Context improvements

[bvaughn]

React maintains context using a stack structure (ReactFiberStack). Each time a context-providing component is encountered during the begin-work phase, it is pushed onto the stack. We push early to ensure stack size consistency/predictability. However, at the time we initially push, the context-providing instance may not yet exist. In this case we add a placeholder context object and a default value for whether we performed work at this level and assume that we will update both later.

Previously we initialized the "did perform work at this level" value to false. However that approach can cause problems for context-providing components, making it possible for a component to block its own updates by failing this check in ReactFiberClassComponent. This was initially reported by @edvinerikson via Twitter.

I believe a more correct approach to initializing the "did perform work at this level" bit for a context-providing component is for the component to default to the same value as its parent. We can later update the value using the invalidateContextProvider helper to match what shouldComponentUpdate returned.

I have added some new test cases to the ReactIncremental-test to cover the previous regression case. These new tests may overlap somewhat with the existing ones but I thought it better to be slightly verbose in this case. (I'm open to feedback on this though!)

PS While investigating this issue I also noticed a difference in behavior between stack and fiber DOM renderers. I believe in this case that fiber is behaving correctly and the noop renderer is consistent with fiber's behavior, so I didn't bother adding additional stack-specific tests.

[gaearon]

Wow, cool find.
Have you confirmed this fixes the repro provided by @edvinerikson?

These new tests may overlap somewhat with the existing ones but I thought it better to be slightly verbose in this case.

Could you clarify what case this is testing that wasn’t tested before? In other words, what is special about these new cases that was missing in old ones? I’m trying to understand in which cases the bug would occur but it doesn’t jump out to me.

(I’m cool with adding more tests. Just trying to understand what we missed earlier.)

PS While investigating this issue I also noticed a difference in behavior between stack and fiber DOM renderers.

What difference is it? Can products (internally or externally) be relying on it?

[bvaughn]

Thanks 😄

Have you confirmed this fixes the repro provided by @edvinerikson?

Yes. I rebuilt the react/react-dom bundles locally with this change and confirmed that it resolved the repro case @edvinerikson provided.

Could you clarify what case this is testing that wasn’t tested before? In other words, what is special about these new cases that was missing in old ones? I’m trying to understand in which cases the bug would occur but it doesn’t jump out to me.

The specific test case added that failed previously is "updates descendants with multiple context-providing ancestors with new context values". The others I added to ensure we had extra coverage and that my changes didn't cause a regression.

What difference is it? Can products (internally or externally) be relying on it?

It's reproducible using the public API in test "should update descendants with new context values if setState() is called in the middle of the tree". Specifically the last assertion that verifies the updated output is "count:1, name:not brian". This is true for the noop and fiber renderers but for the stack renderer count remains 0.

It's hard to say whether products could be relying on this behavior, but my intuition says no. The previous behavior is unintuitive and (in my opinion) broken.

[sebmarkbage]

There is also a sCU on functional components but since that's a new thing I guess we could ignore that.

[acdlite]

But as long as it's in the codebase, I think we should keep the implementations in sync.

[sebmarkbage]

That means that we have to push/pop every functional component though, right? That seems like we're severely punishing perf of functional components for this edge case.

[acdlite]

I guess I misunderstood @bvaughn's original description of this change as a bugfix, or improvement to a more correct behavior. Subsequent comments suggest otherwise.

Now that I think about it, this only affects context providers, right? So I guess functional components aren't affected?

[sebmarkbage]

The difference is whether functional components can terminate rendering. They can't with this PR if the context provider rerendered above them. Basically sCU on a functional component becomes useless in many cases.

[sebmarkbage]

Actually I'm not sure what happens... They don't get priority and pending props, so maybe things don't rerender?

[bvaughn]

This change shouldn't be relevant for functional components, since it's specific to context-providers. Functional components can only consume context, since there's no underlying instance. Right?

[gaearon]

One thing to watch out for is whether we make some common case that previously bailed out slower. For example technically we could make context even more "correct" by always traversing deep on every update but this would kill perf. We need to see if the fix that doesn't match Stack behavior also doesn't make some cases pathological.

[sebmarkbage]

I think this makes sense to me.

[acdlite]

I think we should either remove shouldComponentUpdate from functional components, or implement this change there as well. https://github.com/facebook/react/pull/10334/files#r130508835

Never mind, I don't think functional components are relevant here. #10334 (comment)

[acdlite]

Suggested refactor:

if (didChange) {
  const mergedContext = processChildContext(workInProgress, previousContext, true);
  instance.__reactInternalMemoizedMergedChildContext = mergedContext;
  pop(didPerformWorkStackCursor, workInProgress);
  pop(contextStackCursor, workInProgress);
  push(contextStackCursor, mergedContext, workInProgress);
} else {
  pop(didPerformWorkStackCursor, workInProgress);
}

One fewer branch and prevents Flow from complaining.

[bvaughn]

I like this suggestion. Check out the most recent commit. It cool?

[bvaughn]

One thing to watch out for is whether we make some common case that previously bailed out slower. For example technically we could make context even more "correct" by always traversing deep on every update but this would kill perf. We need to see if the fix that doesn't match Stack behavior also doesn't make some cases pathological.

To clarify, the issue initially reported by @edvinerikson was a regression in 16.

[bvaughn]

Lots of discussion on this issue but it seems like @sebmarkbage approves and @acdlite retracted his initial concern, so I'm going to move forward with this! Let's follow up if needed.

[sebmarkbage]

To clarify, the issue initially reported by @edvinerikson was a regression in 16.

That was a behavior regression. I think @gaearon is talking about a performance regression. I don't know if we have any tests to confirm whether this leads us to overrender more than necessary.

[gaearon]

I remember adding some when I was working on memoization logic. So we’re probably good.