Playing nicely with redux, mobx and pure components

[d-leeg]

I would like to open a discussion concerning the way react-router plays together with PureComponent and libraries like redux and mobx.

This topic was briefly touched upon in #4680, but I believe that due to the fact that it influences a huge number of developers, and that the workaround suggested there leaves a lot to be desired; a more thorough discussion is warranted.

I wish to start this discussion here by considering 4 points:

1. Recap of the problem.
2. Arguing why the solution suggested in the guidelines is far from optimal.
3. Analysis of the causes of the problem.
4. Suggested solution.

To recap, the heart of the problem is that react-router uses the context to pass down location and match information. However, changes to this information are
blocked, and not propagated down any sub-tree of the components tree that is rooted at a pure component that has no interest in the react-router context.
Obviously, such blocking completely breaks down the functionality of components such as Route in the sub-tree.

Unfortunately, blocking components are rife: either due to explicit use of React.PureComponent, or by using libraries such as react-redux and mobx; for example, every react-redux component created using a regular call to connect() is pure. As a result, the solution suggested in the guidelines, of punching a hole in every blocking ancestor by obtaining the location and passing it as a prop, is far from being ideal, both from a practical point of view, and from a more conceptual point of view.

More specifically, it suffers from at least the following drawbacks:

1. Punching the holes must be done explicitly (i.e., it is not automatic and transparent).

2. It potentially causes many unnecessary re-renders: a punched component high up the tree will try to re-render the whole sub-tree on every location change.

3. It completely ruins modularity. This is undoubtedly the biggest issue, and for medium sized and larger projects that use redux it is actually a killer. The reason is that
   when an affected component (such as a Route) is added anywhere in the component tree, all its blocking ancestor components must be punched. In other words, a change to
   one component requires a change to multiple other existing components that need not even be aware of its existence: modularity is dead. This is a very big problem for reusing code
   and, in projects where different teams are responsible for different components, it is almost intolerable. The only way to regain modularity is to have all components punched
   in advance. This is clearly unacceptable as it results in a full-scale elimination of the concept of pure components.

4. Other libraries (such as react-redux) do not suffer from such anomalies. E.g., pure components do not block nested connected components from having access to the information they need.

5. From a conceptual point of view, why does react-router use the context? The obvious answer is in order to not have to inject unneeded props to ancestors. The proposed
   solution of punching holes by passing unneeded location props to blocking components obviously negates the whole idea of using the context in the first place.

I will now describe briefly what I see as the source of the problem.
Please forgive me if I am stating the obvious, or making mistakes. As Michael Jackson and Ryan Florence said in their talk at ReactJS 2016, we are all still learning how to use React.

At its core, the problem is that pure components, that do not care about the react-router context, do not pass the new context down when it changes.
This is definitely a conceptual problem within React, and I guess is at least part of the reason why the context is still considered experimental by the React team.
However, this does not pose a problem if the information passed in context never needs to change. For example, in react-redux the redux store (more accurately, a pointer to it)
is passed down the context, and this pointer never changes. Being a mutable object (observe that the store, unlike the state of the store, is mutable) one does not have to change
the pointer to the store in order to pass down the new state of the store. The latter is always accessible by calling store.getState(). On the other hand, react-router passes in the context
immutable data (namely match and location inside route), and there lies the problem.

I suggest the following solution: have any react-router component, like Router or Route, that passes information down in the context never change that information!
Thus, for example, instead of passing down route, which needs to change as the location (and match) change, pass down a constant function getRoute() that returns the most up to date route.
Let's call such a component X. The only missing piece now is to make sure that a client component (like Route) which is a descendant of X, will be notified whenever
X has new route for it. This can be easily done using a subscription mechanism (like in history or react-redux).

More concretely, this will result, for example, in the following changes to Router: remove route from the child context.router, and add a subscribe and getRoute methods instead.

The changes to Route will be as follows: as for Router, remove route from the child context, and add a subscribe and getRoute methods instead. In addition,
in componentWillmount subscribe a listener to context.subscribe (and unsubscribe in componentWillUnmount). This listener will call context.router.getRoute(),
use the returned ancestor route information to set its own location and match,
and then notify all of its listeners of any changes. In addition, anywhere context.router.route is used, it should be replaced with context.router.getRoute().

Alternatively, one can get rid of getRoute altogether, and pass down any route changes to the listener callback, which will then cache that information for future use.

Observe that the above solution will easily traverse any blocking components, with no ill effect, and completely transparently.

The only drawback I can see (apart from the negligible overhead) in a naive implementation of the above, is that it may cause double rendering of nested Route components that are not blocked:. I.e., when the location changes, first the ancestor Route/Router will start a re-render that will cause the descendant Route to re-render, and then the subscription mechanism will kick in triggering another re-render when the nested Route's listener method gets notified. However, This can be easily circumvented (as is usual in React) by shallowly comparing the currently cached route.location and route.match with the ones in the new route in the notification.

Here it is guys, what is your opinion?

[gnoff]

@mjackson I thought at one point near the end of 2016 you said that the team was exploring using a subscription model parallel to what react-redux does. It has probably been rehashed quite a bit before but are there perf reasons this wasn't done?

I agree with @d-leeg that needing to guarantee the location prop on arbitrary ancestor components if they are ever connected is a huge surface area for bugs and to my knowledge there is nothing that can make this failure noisy which means its likely to be found in production more often than not.

I'd love to be educated though, I know there is probably a lot of history on this issue I am ignorant of.

[bkniffler]

I agree with @d-leeg. I'd also like to add that, in my opinion, to improve the interaction with dataflow libraries like redux/mobx the query should be stored as an object. Storing and mutating the search string makes it very difficult if you use different query parameters in different components in your app, since they would all be rerendered if anything changes in the search string. Implementing shouldComponentUpdate is difficult, and manual query string parsing everywhere wastes performance.

The main reason for removing the query string was "We've had many, many requests over the years to modify the way we handle query string parsing and serialization." (#4410), so I propose that we return the query object and introduce parseQuery and stringifyQuery props on the router so people can adapt the mechanisms to their needs and we can have a lean interaction with the queries again.

[etienne-dldc]

@pshrmn have answered to this in #5037 :)

[d-leeg]

@etienne-dldc Thanks for the reference.
However, I do not think that @pshrmn "answered this question".
The question he answered was "why the decision was made to remove the old subscription mechanism form V4", and it assumes an ideal isolated world for react-router.

The question/request raised by this issue is how to nicely co-exist with libraries like redux and mobx. Perhaps a subscription mechanism like I suggested may be offered as an option for users of these libraries.

Now, to directly address that answer: looking at the two reasons given for why subscriptions were abandoned, I have addressed both in my proposed solution: the first (stale context) is completely eliminated since the context never changes, and the second (unnecessary-re renders) can most probably be addressed without a scheduler by simply caching the last location that was received.
In any case, as I argued in my first post, even if the latter is not done, for most projects using react-redux (and mobx) punching through the containers will most probably reduce performance more than the possible re-renders may.

[etienne-dldc]

@d-leeg Um... The way I understand the article is that they were unhappy using "tricks" (subscription mechanism) so they removed it. But I might be wrong ¯_(ツ)_/¯ .

Also I totally agree with you about the drawbacks of current RR4. In fact, to solve this problem I have made a custom version of RR for one of my projects that do use subscriptions and the reason I end up here is that I was searching for why RR4 is not using subscriptions and can we avoid making "yet another routing library" for those who want a subscription mechanism.

BTW if you're curious about it you can find my implementation here : Realytics/react-router-magic.
I've just put the code on github but it's not made to be used (at least not for now).
Note that the way it works is a bit different than what you described because each Route exposes a different context, this allows cascading update when triggered by context and exposing parentMatch which make relative path possible.
A quick note about "multiple render problem" : I can't produce it with my implementation so if someone has an example of subscription mechanism that trigger unnecessary render I'm curious about it !

[d-leeg]

@etienne-dldc it looks like we are basically in agreement (however I will not call the observer pattern "a trick") :)

I will definitely look at your code later this week. Thanks for sharing!

If you look closely at my suggested solution, it also cascades. The context is not necessarily constant along a path from the root of the component hierarchy: every Route is a subscriber of its closest ancestor Route (or Router), and exposes a different, but fixed, context to its descendants. The point is that this is enough to ensure that every component sees a constant context throughout its life, and thus the "stale context" problem disappears.

It would be great if we can get some feedback from the main contributors...

[hnordt]

I stopped using React Router v4 because of these necessary "tricks". But I loved its API, I hope to see a solution soon.

[xavierfuentes]

@hnordt did you go back to v3 or tried a different one like this

[EloB]

I would say version 4 is broken by design. React documentation says don't update the context. Quote from offical React documentation. If you should update context it has to be through "observers".

Don't do it.
React has an API to update context, but it is fundamentally broken and you should not use it.

https://facebook.github.io/react/docs/context.html#updating-context

[brianespinosa]

@timdorr I see that you closed this issue on July 11 but there has been no official comment on the initial proposal by @d-leeg. It's entirely possible this was addressed elsewhere and I missed it. Maybe there are plans to have a static context in RR5 or some other similar solution?

[etienne-dldc]

If anyone is interested, the way I now deal with routing is the following:

• Keep history.location in sync in redux with https://github.com/Realytics/redux-router-location (not prod ready ! PRs welcome)
• Create contants for each routes with https://github.com/Realytics/path-pattern (almost prod ready, PRs welcome)
• Use regular selector to know if a route match or not selectUserMatch = (state) => UserRoute.matchExact(state.location.pathname)
• Connect component with react-redux to get the userMatch props
• Use && operator or if to render the component {props.userMatch && <TheComponent />}

This approach have been quite successful so far, the main advantage is that routing is done in redux so you can do some nice optimisations 😄.