Use ReactDOM.unstable_batchedUpdates in useRouterHistory?

[taion]

From reactjs/react-router-redux#197.

This issue arises when using a Flux integration for React Router. The issue is that, if the integration updates e.g. Flux stores when history updates, this will lead to a separate setState (in e.g. smart component containers, from the store update) from the setState in <Router> (from the transition manager update directly), which will lead to the page rendering twice.

We could in principle resolve this by using ReactDOM.unstable_batchedUpdates in useRouterHistory. We can't really do this in history because that library is not aware of React. I think it'd make more sense to do it here rather than in every React Router integration, because otherwise they'd all need the same logic.

The downsides of course are:

• We now call into an unstable API
• We introduce an explicit react-dom dependency (for now) when currently there isn't one

cc @gaearon, in case this is a horrible anti-pattern if used in conjunction with Redux

[timdorr]

Until it's sanctioned, I would avoid using these kinds of APIs. React already does batching internally and I believe improvements to negate the need for these kinds of extra APIs are coming. The side effects of using them might be pretty negative.

Also, I'm getting $scope.apply angular flashbacks :P

[taion]

React only does batching right now inside React lifecycle hooks. setState calls outside of those aren't batched, which is what was causing the reported RSR issue.

[timdorr]

Yeah, sorry, I should have said "does batching to a certain extent".

But I'm wondering if this is something we should be concerning ourselves with. Who is connecting location state and for what reason? The router already provides that for you. So, you're getting the same information in two different ways. It's no wonder that you're getting rendered twice.

The state that redux-simple-router provides is primarily useful outside of your components. Such as in action creators or middleware. So this shouldn't be a common problem. It's mainly a documentation of best practices by the binding libraries.

[taion]

Is Redux connect smart enough to only re-render when a specific portion of the store has changed? At a glance, it looks like it re-renders whenever anything in the store changes.

[timdorr]

It has a shouldComponentUpdate check that does a shallow compare of the props. So it will only render when whatever it has selected changes.

[taion]

I see https://github.com/rackt/react-redux/blob/master/src/components/connect.js#L169-L181 and https://github.com/rackt/react-redux/blob/master/src/components/connect.js#L78-L80. Won't this always set hasStoreStateChanged to true after a store update?

[timdorr]

Ah, I think you're right. It must be up to you to provide your own shouldComponentUpdate. I never did a deep dive on react-redux, I just normally focus on the main module.

[taion]

Or use reselect or something.

But if we make React batch the updates in the history callback, then we avoid this issue.

[ryanflorence]

We might have been the first project outside of facebook to use context, so we're not afraid of unstable API (it's projects like ours that often justify them).

At the least, it could be an opt-in option to Router.

That said, I'd rather have a different solution. Is there a decent workaround?

[taion]

I don't think this should be an opt-in (or even opt-out) feature. It ought not break anyone or meaningfully change behavior except for preventing the double-render thing – it'd purely be an implementation detail on our side.

The issue is basically:

1. history fires listener
2. Flux integration picks up on update, updates stores, leads to subscribed containers calling setState and re-rendering
3. <Router> picks up on update, calls setState, re-renders

Unless I'm misunderstanding, this issue is most prominent with Redux, since there's only a single store, so it causes every container to re-render.

It's really a question of whether we're okay with calling into an unstable ReactDOM API and taking an explicit react-dom dependency. You can see from e.g. facebook/relay#713 that the RN equivalent is actually a separate ReactNative.unstable_batchedUpdates (although we don't meaningfully support RN yet, so this isn't going to break anyone in practice).

You can see in the Relay case that unstable_batchedUpdates is used there for much the same purpose.

[ryanflorence]

I still kind of expect a redux router integration that uses RouterContext and sets up its own history listener and dispatches when locations come through.

[timdorr]

That's what redux-simple-router does. However, there are two listeners, and therefore two setState/renders.

[taion]

You'd actually have to do with redux-router does, which is to replace <Router> entirely and have everything be driven by the store state rather than the router state. It's a pretty different approach to RSR, I think.

[taion]

Just tested it – it doesn't actually help.

Better probably to wait for React to update context propagation such that people no longer need to use { pure: false } for their connected components.

[mjrussell]

@ryanflorence @taion I actually tried using the RouterContext during an attempt to simplify/upgrade redux-router to ReactRouter v2.

https://github.com/mjrussell/redux-router/blob/rr-v2/src/ReduxRouterContext.js

This approach felt more like react-router was the single point of truth for the routing data and the store.router more of just a mirror of that data. The problem I found was that it became very difficult to update the routingReducer and expect it to fire off a change to React-Router without retriggering another dispatch.

Once I saw RSR had the full location data I hopped on that bandwagon though 😄

[gaearon]

Is Redux connect smart enough to only re-render when a specific portion of the store has changed? At a glance, it looks like it re-renders whenever anything in the store changes.

No, it does not re-render everything, that would have a terrible performance. React Redux has a bunch of performance optimizations, from implementing shouldComponentUpdate(), to calling mapStateToProps and mapDispatchToProps only when necessary, to caching render()ed element (which has the same effect as shouldComponentUpdate() but allows us to delay the child prop calculation until render() to guarantee there are no stale props when you nest containers).

Some perf optimizations in React Redux:

• https://github.com/rackt/react-redux/blob/4fca3cbc736b7462de65f589d3b0fdab0cb7a495/src/components/connect.js#L78-L80
• https://github.com/rackt/react-redux/blob/4fca3cbc736b7462de65f589d3b0fdab0cb7a495/src/components/connect.js#L101-L103
• https://github.com/rackt/react-redux/blob/4fca3cbc736b7462de65f589d3b0fdab0cb7a495/src/components/connect.js#L111-L113
• https://github.com/rackt/react-redux/blob/4fca3cbc736b7462de65f589d3b0fdab0cb7a495/src/components/connect.js#L150-L152
• https://github.com/rackt/react-redux/blob/4fca3cbc736b7462de65f589d3b0fdab0cb7a495/src/components/connect.js#L204-L210
• https://github.com/rackt/react-redux/blob/4fca3cbc736b7462de65f589d3b0fdab0cb7a495/src/components/connect.js#L214-L219
• https://github.com/rackt/react-redux/blob/4fca3cbc736b7462de65f589d3b0fdab0cb7a495/src/components/connect.js#L228-L234

Some tests proving they work as intended:

• https://github.com/rackt/react-redux/blob/4fca3cbc736b7462de65f589d3b0fdab0cb7a495/test/components/connect.spec.js#L462-L756
• https://github.com/rackt/react-redux/blob/4fca3cbc736b7462de65f589d3b0fdab0cb7a495/test/components/connect.spec.js#L862-L983
• https://github.com/rackt/react-redux/blob/4fca3cbc736b7462de65f589d3b0fdab0cb7a495/test/components/connect.spec.js#L1411-L1507

[gaearon]

Won't this always set hasStoreStateChanged to true after a store update?

To clarify—this is correct but we're using a different technique to avoid rendering. We memoize the return value of previous render(). If we return the same element, React will bail out of reconciliation so it's equivalent to us implementing shouldComponentUpdate() but gives us the ability to delay calling map*ToProps() until the actual rendering so we invoke those functions with the same props we are rendering with.

[taion]

@gaearon To clarify, I'm talking about the case with { pure: false }, due to #470. I believe most of the render-related optimizations then get disabled, including the SCU check and the memoization in render.

[gaearon]

Oh, I see. This is correct, with { pure: false } we don't enable any optimizations at all.

[taion]

Well, we can't fix it just by using batched updates anyway. And mostly people can get by with pure Redux containers anyway.

[timdorr]

This only applies if you map location to props in redux-simple-router (a bad pattern, the router already provides this to your components) or if you have pure set to false (fairly uncommon). I think most commonly people are running into the first case and that's more an issue of our documentation.

Edit: redux-simple-router's documentation, that is.

[taion]

@timdorr You need to set pure to false for context updates for e.g. isActive to propagate through to update link active status.