Update shallowCompare to accept nextContext

[tony99nyr]

Across our application we are using immutable objects as properties and thus using shallowCompare for all our shouldComponentUpdate. Because of this children with contextTypes don't get updates when the context on the parent changes. Adding an additional comparison for context (when it is provided) fixes this problem.

[aweary]

Is the typeof check required? shallowCompare should already work with stateless components so that same behavior should apply to context.

[tony99nyr]

I was thinking it may add execution time to the shallowCompare call so why not avoid calling shallowEqual when we can. I don't think its required though.

[jimfb]

Yeah, I don't think shallowEqual is going to add a meaningful amount of execution time. The undefined check adds bytes, which increases download size; it might be a wash. If we were going to do that check, we should just use nextContext !== undefined instead of doing a typeof and string compare, both of which are slow.

But in the interest of simplicity, let's just remove that undefined check.

[zpao]

Seems reasonable to have this and have it be optional. Any thoughts @spicyj? (in particular, iI'm wondering about potentially changing PureRenderMixin as well)

[sophiebits]

Seems fine to me if we remove the undefined check – let's change PureRenderMixin too.

[jimfb]

Ping @tony99nyr

[tony99nyr]

Apologies, I didn't know you were waiting on me. I made the changes you guys requested.

[ghost]

@tony99nyr updated the pull request.

[jimfb]

Thanks @tony99nyr!

[sophiebits]

I just realized this is a breaking change. If someone calls shallowCompare(this, nextProps, nextState) manually, it may have returned false before but would always return true now. This broke a component at FB that ended up updating endlessly because it would setState to the same state values, expecting it not to rerender.

Bad component design, but maybe we do want the undefined check after all. Can we revert this for now? I have to work around this at FB until we do.

[jimfb]

I'd prefer to fix-forward instead of revert, if possible/practical. Can you just comment out this change on www so we have a little time to fix this up before our next sync?

Checking for undefined seems like a reasonable path forward here. Or we could add a shallowCompareWithContext function.

[sophiebits]

I'd prefer to fix forward instead of revert, if possible/practical.

Why? It doesn't slow us down (except for an artifically-created sense of urgency) and master is unbroken in the meantime.

Can you just comment out this change on www so we have a little time to fix this up before our next sync?

Yes, though it's always preferable that master actually matches what we're using.

[sophiebits]

(There's also little chance of merge conflicts, which would be the typical reason for pushing forward.)

[jimfb]

Why?

Fixing forward makes it easier to reason about what changes are actually in master. It reduces churn, makes the diffs much easier to follow. It reduces the probability of breaking someone who was depending on a merged commit (related to merge conflicts, except harder to detect since it's not caught by git). It reduces the probability of us undoing a feature and then never re-introducing it (#6364). It closes a whole can of worms (for instance, should we revert the wrapping of events because it breaks IE/edge in some cases: #5700, or should we fix forward).

In this particular case, it's basically a two line diff, so it is easy to go forward/back and it probably doesn't matter one way or the other. But in general, it seems better to always fix forward when possible/practical.

[sophiebits]

I disagree that #6364 is an argument in favor of not reverting: rather, I see that if it had stayed in master than master would still be broken. Fixing forward makes it easier to reason about what's in "master" but if we're not running master because it's broken and we need to patch it locally to get it to run, I'm not sure what value that provides. You're right that it helps people who were depending on the merged functionality but somehow weren't affected by the bug.

[jimfb]

Regarding #6364, master is still broken either way. Reverting just traded one bug for a different bug. Unless we go back all the way and revert #4182, master will still be broken. I think the whole #6364 thing is a perfect example of why we should try to always fix forward (when possible/practical).

I'm actually fine with reverting this particular commit, if that's what we decide to do. I wasn't entirely sold on the value of this commit in the first place; I think all the shallowCompare stuff should be moved to userland anyway. But I was not ok with the fact that we reverted #6364 without discussion - I think that was the wrong call.

If we want to revert this commit, that's fine, but it would be preferable if we can find a fix forward.

[tony99nyr]

I don't want to block you guys, let me know if you need me to do anything.

[zpao]

Please revert and followup with the correct fix. That should be the way we operate. It's on the reverter & reviewer to ensure we actually followup.

Further, the change to ReactComponentWithPureRenderMixin is definitely a breaking change and should actually land separately so we can take the shallowCompare change in the stable branch (once it's not breaking).

[sophiebits]

Reverted in #6877.