remove subscriptions #4598

ryanflorence · 2017-02-24T22:39:33Z

Why?

They are hard to understand
They don’t feel like React
They were causing lots of re-rendering on popstate that we’d have to start doing our own scheduling/batching to get around D: With every new nested Route you'd get another round of rendering.

But what about `shouldComponentUpdate`?

Pass location to the PureComponent (or whatever). If a component as a Route or Switch in it then location needs to be a part of the
sCU diffing.

<Route render={({ location }) => (
  <SomePureComponent location={location}/>
)}/>

Also, withRouter continues to subscribe directly to history if somebody would rather jump into the subscription world instead of passing a prop down.

What about Redux?

We implemented subscriptions so that deep updates work in Redux. But, Redux users want more than just that.

They want to:

get deep updates that survive redux’s automatic sCU
read routing state from the store, not components
navigate with dispatch
sync the url and store so the devtools time-travel/upload debugging tools work

Subscriptions only solved (1). To get the rest requires the exact same touch points whether we do subscriptions or not.

In summary

Since the Redux integration story is identical whether we use subscriptions or not, and normal sCU can be solved by just passing the location to the optimized components, there’s no reason we need to keep using subscriptions.

We’re bowing out of a fight against React where we implement our own scheduling of updates. Also, we still have withRouter if people feel like stepping into the ring.

ryanflorence · 2017-02-24T22:42:15Z

packages/react-router/modules/Route.js

@@ -2,17 +2,13 @@ import React, { PropTypes } from 'react'
 import warning from 'warning'
 import matchPath from './matchPath'



Moved computeMatch cause it felt cleaner as an instance method, only had to check context.router.location v. props.location in one spot. Also, it's our doImperativeStuff method of this component. We do it initially, and then when we get new props.

I actually like the method up here, and I like that it's just 1 line up here instead of 4 down below. No need to create the function for every instance. Once will do :)

JavaScript prototypes use only one function per instance.

Also, I was having to do the same logic to figure out the arguments to pass to computeMatch, so complexity was just moving up and repeated, now its only in one place.

ryanflorence · 2017-02-24T22:45:31Z

@pshrmn this will effect the relative routes/links work since Route doesn't put anything on context anymore, you won't know the whole hierarchy of match.url anymore. I imagine we'll just have route shadow context.router again and put match on it in the patch that adds relativity.

pshrmn · 2017-02-24T23:05:16Z

I'll take a closer look at this later. I believe that a <Route> should add its own router object. If you check out #4589, you can see that this is necessary for its children to know which location they are "in" (there is a modal example there that relies on that).

pshrmn · 2017-02-24T23:08:38Z

Also, it might not hurt to add a <Listener> component for people who would prefer to compose instead of use withRouter to get around sCU blocks. I wrote one when I was messing with removing listeners and I didn't test it extensively, but it seemed to do the job.

timdorr · 2017-02-25T03:13:06Z

BTW, the most common case of someone running into this seems to be using a redux connect'ed Route component. Luckily, we're getting rid of sCU soon. So, in the not too distant future (somewhere in time and space), this won't be a bug on either side of the aisle.

mjackson

Overall, this looks great. I'm happy to ditch subscriptions.

mjackson · 2017-02-25T05:24:17Z

packages/react-router/modules/Route.js

@@ -29,37 +25,13 @@ class Route extends React.Component {
    location: PropTypes.object
  }

-  static childContextTypes = {
-    router: PropTypes.object.isRequired


I believe that match should live on context.router for the sake of nested <Link>s and <Route>s that need to be relative. Can we keep it there for now and just remove the subscriptions in this PR?

## Why remove subscriptions? 1. They are hard to understand 2. They fight against React 3. They were causing *lots of cascading re-renders on popstate* that we’d have to start doing our own scheduling/batching to get around D: ## What about shouldComponentUpdate? Pass `location` to the `PureComponent` (or whatever). If a component as a Route or Switch in it then `location` needs to be a part of the sCU diffing. ```jsx <Route render={({ location }) => ( <SomePureComponent location={location}/> )}/> ``` We implemented subscriptions so that deep updates would work in Redux out of the box (no react-router-redux needed). But, Redux users want more than just that. They want to: 1. get deep updates that survive redux’s automatic sCU 2. read routing state from the store, not components 3. navigate with dispatch 4. sync the url and store so the devtools time-travel/upload debugging tools work Subscriptions only solved (1). To get the rest requires the exact same touch points whether we do subscriptions or not. ## Why remove withRouter? It has the same cascading render problem (because it used subscriptions), we don’t want to deal with that. `withRouter` has been about getting access to the imperative router API. It can easily be added to app manually in two lines: ``` const withRouter = (Comp) => (props) => <Route render={(router) => <Comp {…props} {…router}/> ``` ## In Summary Since the Redux integration story is identical whether we use subscriptions or not, and normal sCU can be solved by just passing the `location` to the optimized components, there’s no reason we need to keep using subscriptions and fighting against React.

ryanflorence · 2017-02-26T04:49:45Z

packages/react-router/modules/Route.js

+      router: {
+        ...this.context.router,
+        match: this.state.match
+      }


^ added match back in

pshrmn · 2017-02-26T05:01:36Z

packages/react-router/modules/Router.js

-      Object.assign(this.router, history)
-      Object.assign(this.router.match, {
-        isExact: history.location.pathname === '/'
-      })


The above code was just added in #4581 (although I now see that I missed adding this.unlisten and calling that in cWU).

mjackson

Getting there, but it still feels like this PR is doing too much.

mjackson · 2017-02-26T17:12:50Z

packages/react-router/modules/Route.js

+  }
+
+  static childContextTypes = {
+    router: PropTypes.object.isRequired


I like keeping childContextTypes right next to getChildContext so I can see the two together easily.

mjackson · 2017-02-26T17:24:45Z

packages/react-router/modules/index.js

@@ -6,4 +6,3 @@ export Router from './Router'
 export StaticRouter from './StaticRouter'
 export Switch from './Switch'
 export matchPath from './matchPath'
-export withRouter from './withRouter'


Can we discuss what to do with withRouter in a separate PR? I think we may want to keep it, but just simplify the implementation.

There's a lot of subtle things I'm sure I can't explain well enough here, I'll wait till you recover from Sweden and let's do a quick brain dump.

mjackson · 2017-02-26T17:33:17Z

packages/react-router/modules/Route.js

-  componentWillMount() {
-    const parentRouter = this.context.router
-
-    this.router = {


If we're going to remove this.router, we should at least have a few tests that ensure correct behavior. Also, Redux users are only 1 group that uses shouldComponentUpdate. Anyone else who's using it may still benefit from us mutating context.router.

@ryanflorence

This commit builds upon (and supersedes) the work done by @ryanflorence in #4598 to remove subscriptions from the codebase. Instead of subscribing to location changes and using forceUpdate inside every <Route>, we better follow React's model by using setState in <Router> and letting changes trickle down to descendants using React's built-in state propagation mechanism. This also means that we don't mutate context.router anymore (no more Object.assign). Instead, context.router was split into two parts: context.history and context.route (the <Route> props + match). This allows us to leave context.history untouched and change context.route only according to the props + match state of each <Route>. withRouter was also updated to provide everything in a "router" prop instead of spreading all props to the component.

mjackson · 2017-02-28T02:16:40Z

Superseded by 1f01079. Thanks for your work here, @ryanflorence. You were right on track, as always.

ryanflorence · 2017-02-28T16:12:01Z

Except for that time I brought subscriptions in!

benwiley4000 · 2017-06-07T05:13:58Z

First can I say - I really agree that subscriptions and deep-mutable context does seem sort of wrong/un-React to me, so I understand the reasoning behind this change.

Though I'm somewhat unconvinced react-redux is ditching shouldComponentUpdate based on discussions over here. Someone please correct me if you know something I don't! 🙂

So essentially until that happens (if it does) devs will need to wrap withRouter not only around the component that needs it, but also every connect all the way up the hierarchy, until we reach the Router. Which is okay, on a surface level! This isn't that hard to do.

But I can definitely see this being confusing to devs, particularly on a large team, who see a higher order component wrapping another one even though its props aren't being used. A logical thing might be to delete this under the assumption it's obsolete code, and you don't hear about it until a regression surfaces later.

I haven't run into this yet on our app at work but I imagine we will once we start creating route dependencies deeper in the app.

The other reason I'm following this discussion is that I'm trying to solve the context problem with a library I'm building on my own time - in my case, I think I'll stick with subscriptions at least until an updated context API is announced. Besides the HOC-wrapping thing being a lot to ask of users, it would certainly eliminate any performance gain from connect's shouldComponentUpdate as my child context updates very frequently. This typically shouldn't be a problem with routes, so I guess that's a remaining argument in favor of the decision made here.

ryanflorence commented Feb 24, 2017

View reviewed changes

ryanflorence force-pushed the v4-remove-subscriptions branch from 50cca14 to d60450f Compare February 24, 2017 22:55

mjackson requested changes Feb 25, 2017

View reviewed changes

ryanflorence force-pushed the v4-remove-subscriptions branch from d60450f to 04fd041 Compare February 26, 2017 04:48

ryanflorence commented Feb 26, 2017

View reviewed changes

pshrmn reviewed Feb 26, 2017

View reviewed changes

pshrmn mentioned this pull request Feb 26, 2017

Add context.route.location #4589

Merged

mjackson requested changes Feb 26, 2017

View reviewed changes

mjackson closed this Feb 28, 2017

ryanflorence deleted the v4-remove-subscriptions branch February 28, 2017 16:20

timdorr mentioned this pull request Sep 22, 2017

Proposal: non-blocking updates refactor #5534

Closed

lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019

		@@ -2,17 +2,13 @@ import React, { PropTypes } from 'react'
		import warning from 'warning'
		import matchPath from './matchPath'

Uh oh!

remove subscriptions #4598

remove subscriptions #4598

Uh oh!

Conversation

ryanflorence commented Feb 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

But what about shouldComponentUpdate?

What about Redux?

In summary

Uh oh!

ryanflorence Feb 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanflorence commented Feb 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pshrmn commented Feb 24, 2017

Uh oh!

pshrmn commented Feb 24, 2017

Uh oh!

timdorr commented Feb 25, 2017

Uh oh!

mjackson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjackson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjackson commented Feb 28, 2017

Uh oh!

ryanflorence commented Feb 28, 2017

Uh oh!

benwiley4000 commented Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ryanflorence commented Feb 24, 2017 •

edited

Loading

But what about `shouldComponentUpdate`?

ryanflorence Feb 24, 2017 •

edited

Loading

ryanflorence commented Feb 24, 2017 •

edited

Loading

benwiley4000 commented Jun 7, 2017 •

edited

Loading