Don't skip reconcilation if context differs

[jimfb]

This adds a reconciliation bailout condition asserting that context has not changed, which fixes the other half of @ryanflorence's discovery in #4218.

For those following along, the first half of this bug was fixed in #4221. The assertion that there may be a bug in the reconciler turned out to be due to a bug in jest: jestjs/jest#429 (if you're bored and want to have fun, see if you can identify the 'fix' in this diff that avoids that bug). But I digress...

This diff results in a divergence between dev and prod behavior (specifically, more components will rerender more often in dev) because we no longer bail out of reconciliation in dev mode due to https://github.com/facebook/react/pull/3516/files causing there to always be a context variable change. This means not only that we should start running React unit tests in both dev and prod mode, but also has ramifications for developers who only test in dev before shipping their components to prod.

@sebmarkbage @spicyj

[zpao]

style nit, match where we have &&

[jimfb]

Done.

[jimfb]

@sebmarkbage @spicyj Ping.

[steveorsomethin]

Won't this always evaluate to false if context is used at all (effectively killing the optimization for any application which relies on common libs)? ReactCompositeComponent._processChildContext will replace the reference if getChildContext returns a value. One solution is to detect whether anything changed when merging the new/old contexts and returning the old context when possible, rather than using Object.assign. It would however mean that framework authors who use context would be on the hook to make sure they memoize the values they supply in getChildContext as well as minimizing its surface area to keep merges fast.

[jimfb]

@steveorsomethin I don't remember what @sebmarkbage's reason was, but I vaguely remember there being a reason. The optimization does still apply if the context provider is not in the render path (eg. updating due to a setState). I'm happy to change it if @sebmarkbage (or @zpao / @spicyj) would like it changed. Either way, we should move this diff forward.

[sebmarkbage]

It is not always a new object for the same reason the element isn't always a new object. It is because of subtree state updates where you start deeper in the tree.

[steveorsomethin]

I see, although that would mean that some of the emerging community patterns which result in root state changes will be unable to benefit from memoizing render and getting an automatic, smarter shouldComponentUpdate if props/state are shallow-equal. My interest there over something like PureRenderMixin is detecting otherwise invisible context changes. Even a manual shouldComponentUpdate doesn't really have a clean solution for that scenario.

This change appears necessary, but do you believe there is still time to investigate a more targeted solution before 0.14 ships? I'm currently attempting to use the bail-out optimization + a memoized pure render to simplify a fast top-down render. Something akin to this: https://github.com/reactjs/react-future/blob/master/07%20-%20Returning%20State/01%20-%20Stateful%20Functions.js

I'd be happy to explore and contribute back if you're not aware of any insurmountable hurdles. I'm trying to avoid resorting to observation or imperative actions on refs while maintaining 60fps on a 600mhz arm chip + non-JIT JSC, so I'll keep the context diffing costs in mind.

[jimfb]

@steveorsomethin Nothing insurmountable, except that @sebmarkbage hates anything that smells like a subscription :P, but he is very reasonable and willing to give PRs a fair chance. If I recall correctly, a internally-subscription-based implementation was the only viable alternative that was examined. That was back in #3973. The followup discussion was that, if we were going to do that, we might as well do #3920 and use that to manage the context subscriptions. But then we're not even sure we want to do that, and we'd also need to look at the ramifications on #3398. And then React Conf Europe rolled around and we decided to table all the discussions. In the mean time, we released a 0.14-beta, and this bug needed to be fixed :P.

Anyway, you're welcomed to look into it. #2517 and #3973 would be a good place to start.

[sebmarkbage]

The reason for this PR is to avoid a regression between 0.13 and 0.14. It's not better but at least it's not worse.

#3973 or something like it is the real solution.

[steveorsomethin]

Agreed, with respect to subscription/observation in this setting.

Thanks @jimfb @sebmarkbage, I'll start by integrating #3973 locally and see if anything feels off. I had seen that before but at the time didn't see the utility. With the automatic bail-out, it makes sense.