Investigate when a context from another library causes us to opt out of perf optimizations

[gaearon]

Related to #99 and the follow-up issues and pull requests.
I’d like to understand better in which cases we have the behavior described in #365 (comment):

I dug into why this happens, and it seems like returning a cached element in render() is not enough to prevent a re-render in some cases when there is an additional context between <Provider> and the connect()ed component because of this check in ReactReconciler.

Is just having another component providing context enough to trigger this? Is there any way we can work around this?

[foiseworth]

I did some investigating into this and the issue seems to be caused by the following:

1. There is a component with getChildContext above the connected component (one will usually be Provider).
2. The cached element has getChildContext or an element above that re-renders has getChildContext.
3. React merges the contexts in _processChildContext by assigning to an empty object thus creating a new context object
4. New context object fails equality check thus stopping early bail out in ReactReconciler

Not sure how you can work around this. Changing the equality check in ReactReconciler to a shallowEquals check would work.

[markerikson]

Thanks for looking into this! Does the rewrite in #416 happen to eliminate this issue?

[jimbolla]

416 doesn't cache the element, only the final props, since more aggressive shouldComponentUpdate() means render will only fire if it actually needs to rerender, so I would guess it does.?

[foiseworth]

Did some testing and and 416 fixes the issue and it does indeed seem to be down to shouldComponentUpdate. Some unscientific testing shows a massive perf increase as well.

[DouglasLivingstone]

A similar situation is when the Provider itself gets re-rendered, as in:

service.onUpdate(() => ReactDOM.render(<Provider>....</Provider>, element))

This causes the context from Provider to be regenerated, so the context === internalInstance._context check fails. I'd guess that #416 fixes this too.

[foiseworth]

I did some further testing can confirm that this can be quite an issue if you're using context:

What you would expect

1. connect a component
2. change the store state
3. it re-renders and either it or something in between it and the next component has getChildContext
4. somewhere down the tree a connected sub component is rendered
5. the sub component connect's shouldComponentUpdate will return true (because store state has changed)
6. the sub component connect's render method returns the cached element (because its calculated props haven't changed)
7. React uses cached element, perf win!

What happens

1. connect a component
2. change the store state
3. it re-renders and either it or something in between it and the next component has getChildContext
4. the return of getChildContext is merged with the previous getChildContext into an empty object
5. somewhere down the tree a connected sub component is rendered
6. the sub component connect's shouldComponentUpdate will return true (because store state has changed)
7. the sub component connect's render method returns the cached element (because its calculated props haven't changed)
8. React detects that context is a different object and discards cached element
9. Element (and therefore tree) is re-rendered

To combat it, in the short term I'm going to try to move all my getChildContext above my connected components.

In the long term I guess:

• we advise people to avoid using context with react-redux
• we switch to another method (should as shouldComponentUpdate) for not re-rendering child such as in Rewrite connect() for better performance and extensibility #416
• we think about the behaviour of React and submit an PR to change it (I'm still thinking it should be a shallow compare rather than an equality check on context in React Reconciler).

[foiseworth]

One great example of this bug is if you wrap your top level component in connect. As it will wrap the Provider component which has getChildContext, anytime it re-renders (due to a store update which leads to a change in the output of mapStateToProps) the entire tree will re-render unless you have some non-connect shouldComponentUpdate logic.