Update shouldComponentUpdate docs with advice about closures

[thysultan]

Do you want to request a feature or report a bug?

Possible bug depending on my understanding of the new context feature.

What is the current behavior?

The presence of an <Indirection> component with shouldComponentUpdate:false is blocking updates to context consumers below it after the first click of "Toggle Top Data": https://codesandbox.io/s/v87rp187r7

What is the expected behavior?

For consumers below a shouldComponentUpdate component to continue updating after the first "Toggle Top Data" click.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

React 16.3.0-alpha.0

[TrySound]

@thysultan That's the case of shouldComponentUpdate. React just don't react on changing props. The case of new context api is not relating to shouldComponentUpdate of component which renders consumer itself like here

class Indirection extends React.Component {
  shouldComponentUpdate() {
    return false;
  }
  render() {
    return (
      <DataConsumer>
        {nestedController => (
           <div>
           </div>
          )}
       </DataConsumer>
    );
  }
}

[thysultan]

@TrySound My assumption that this is a bug is based on this test case https://github.com/facebook/react/blob/master/packages/react-reconciler/src/tests/ReactNewContext-test.internal.js#L72-L138. where updates propagate through a shouldComponentUpdate component that is between the provider and leaf consumer.

[TrySound]

@thysultan It's not a bug. Indirection component is not rerendered when any prop is changed. Consider children as any prop.

[thysultan]

@TrySound I'm not sure i understand, <Indirection> is updated when you click "Toggle Top Data" the first time but stops updating on future events. The test details that the new context API should be able to propagate updates through any* number of nested components that could have otherwise prevented the update with shouldComponentUpdate:false in the alternate/older context API.

[bvaughn]

I think the test you linked to specifically confirms that the behavior works correctly. (The Indirection component is not rendered again, since it returns false from shouldComponentUpdate, but the Context.Consumer is rerendered when the Context.Provider value changes.)

In this case, your test shows that changes to topController don't propagating through the Indirection component to the inner consumer, but I think that's expected, because the Indirection is inside of the top-level consumer. However, changes to the nested-provider do propagate.

[thysultan]

The behavior works for the first click, but subsequent clicks are not registered(every click should alternate between true and false).

[bvaughn]

I don't know. This test is hard for me to read. There's several levels of indirection and stuff.

I've tried to simplify it a little (so I can better understand it) here:
https://codesandbox.io/s/7wl94qq5m0

I see the behavior I would expect from this.

[thysultan]

The expected result based on what i understand from the test specifications would be identical to https://codesandbox.io/s/321kq30kx1.

[bvaughn]

Please take a look at the CodeSandbox I linked, and let me know what's unexpected.

Your CodeSandbox is a bit odd to me. It has the following structure:

Context.Consumer
  Context.Provider
    Context.Consumer
      Context.Consumer
        Context.Provider
          Context.Consumer

I'm not sure this level of nesting is indicative of a valid use-case.

[thysultan]

The CodeSandbox you linked to doesn't reveal the same behavior/bug.

Yes there's a lot of Indirection, i think that was intentional for that particular example, the original was by @kentcdodds, i added the Indirection component to get a better sense of what the context API is spec'd to support.

Does the new Context API have restrictions on the amount of indirection a context update can propagate through?

[bvaughn]

I'm just not convinced it is a reasonable bug. What does it even mean for a context's Consumer to wrap its own Provider (as it does in your example)? It looks like a mistake, like you're using the consumer and provider in exactly the opposite way as they're intended. This is why I rewrote in an attempt to simplify.

[bvaughn]

Can you point me to the example by Kent that you're referring to?

[bvaughn]

I assume you're referring to this blog post?

It does not demonstrate the odd wrapping/nesting behavior your CodeSandbox does.

I think this is kind of a version of Kent's that shows the indirection you mention (and it also seems to work):

https://codesandbox.io/s/mz5m6zx2zx

[thysultan]

The CodeSandBox is a fork of Kent's CodeSandBox – my fork added the <Indirection> component to test how React would handle shouldComponentUpdate within that level of nesting.

[bvaughn]

Can you acknowledge the strange nesting structure and explain why it's important? I believe it's the cause of the behavior you're reporting as unexpected, and I don't understand why it's necessary.

For instance, if you remove the double layer of Context.Consumer in the middle, the pen behaves like you want it to:
https://codesandbox.io/s/47vq1pzx0

[jquense]

@bvaughn a "Consumer wrapping its own Provider" is a pattern I ran into migrating code to the new API, as a way for having components map/alter context as it passes through. It does seem though like the problem is not that but Consumer -> Consumer or at least that pattern mixed around the nested Providers. Still I can think of cases where that might happen naturally? e.g nested ThemedComponents, each consumer the context?

[bvaughn]

To be clear, context supports nesting the same provider multiple times, and there are tests for this, so it's okay for a provider to be an ancestor of a consumer. The code snippet provided above just looked a bit unnecessarily/arbitrarily complex and so I was trying to better understand why it had to be that way.

I'm not sure yet what the specific bit that's causing a problem is. Maybe I'll try to pair it down in a unit test this morning.

[gaearon]

In any case, whether there is a bug in React and whether the code is strange are two different discussions.

[jquense]

sure, just trying to help out i've also not been able to simply the example effectively :/

[gaearon]

Yeah, I just mean that

I'm not sure this level of nesting is indicative of a valid use-case

doesn't mean we shouldn't fix it. 🙂

[bvaughn]

In any case, whether there is a bug in React and whether the code is strange are two different discussions.

I didn't mean to imply otherwise, although re-reading the comment you quoted above, I can see where it gave that impression.

[gaearon]

I thought so, just wanted to make sure the original author also knows this. 🙃

[bvaughn]

Good call.

I'm going to pick this up now and see if I can catch it in a local test. (This was my intent anyway, I just wanted to eat breakfast first) 🤪

[gaearon]

Here's a reduced case. https://codesandbox.io/s/k967n8x5v

I'm not convinced it's a bug. I think it might be an artifact of reading topController.data from a closure (from an old render). Not sure.

[bvaughn]

Here's a more reduced case: https://codesandbox.io/s/mq29590rjy

If you remove the Indirection component (or comment out its sCU method) the "bug" doesn't manifest.

I was thinking along the lines of what you're saying though, wrt stale data.

[gaearon]

Apparently I gave a wrong link :-( I had a smaller example but CodeSandbox didn't save it.

[bvaughn]

Adding logging to the click handler (same sandbox) shows that it is calling setState with stale data:

sandbox.16fdafda.js:5036 onClickHandler() {
"topLevel": true
}
08:14:22.920 sandbox.16fdafda.js:5036 onClickHandler() {
"topLevel": true
}

[gaearon]

Here's an example with components combined https://codesandbox.io/s/kyrj9jq93

[bvaughn]

I think the problem is that the inner Indirection prevents the button from re-rendering with new data. (So each time it's clicked it re-applies stale data)

[bvaughn]

The consumer is being re-rendered each time though (as my added logging shows)

[gaearon]

That makes sense to me (and sounds as expected behavior).

The tree is:

- Provider
  - Consumer
    - Indirection that blocks update
      - Component that doesn't re-render but uses values from consumer

The fix would be to move indirection above the consumer.

There is no way React could know that the leaf component (button) uses the value from above.

[bvaughn]

Yup, yup. Agreed.

[gaearon]

I guess it's worth documenting as a gotcha.

[gaearon]

The original example still confuses me though. There, Indirection appears to be inside of the consumer..

[gaearon]

Ahh I get it now. It's inside a nested consumer. It's trying to read a value from the top-level consumer though.

This is exactly why "toggle nested" keeps working. It only uses context values "below" the indirection.

[bvaughn]

Yup!

[gaearon]

This is why I hate render props 😂

Well, not "hate". But I do find that they present their own pitfalls compared to HOCs, and people too readily ignore them. Anyway, I'm fine with the new context API. Just ranting a bit.

[bvaughn]

😆

cc @acdlite too (who is writing the new context API docs)

[acdlite]

There are a few things going on here. One is that closures are confusing. It's really easy to accidentally depend on values from an outer scope without realizing it. @gaearon is right that render props, because they rely on closures, can exacerbate this confusion.

But in my view the core problem with all these examples is that shouldComponentUpdate always returns false. The solution is... not to do that :D (So it's worth pointing out that you could have this same bug with an HOC or with a normal component, too.)

By returning return false from Indirection's shouldComponentUpdate, you're signaling to React that the children are the same. So, it never re-renders the children, so the click event handler never gets updated with the new closure values, so the topController.data is stale the next time you click it.

The unit tests for context happen always return false in shouldComponentUpdate in a few places, but that's my bad. I should have chosen a more realistic scenario. You should never do that unless the children are completely static.

I believe the actionable items for React here are:

• Add additional documentation for shouldComponentUpdate with advice to never treat two closures or two children props as equal unless they are referentially identical, since it's impossible to know what values they close over.
• In the documentation for the new context API, when advertising its ability to bypass shouldComponentUpdate bailouts, don't mislead people into thinking that it can reach into JavaScript closures and swap out those values, too 😆

[thysultan]

From my understanding of the new API this looked like a bug since it manifested the same limitation of the old context API – that is i expected DataConsumer to re-render regardless of shouldComponentUpdate:false, and in doing so also update the inline onClick function.

[gaearon]

i expected DataConsumer to re-render regardless of shouldComponentUpdate:false, and in doing so also update the inline onClick function.

DataConsumer did re-render. There's no reason why it would update the onClick function though because Indirection says "I never update to new children" so it ignores the updated <button>.

Maybe a simpler way to explain this is imagine you had code like this:

<ContextConsumer>
  {value => (
    <Indirection> {/* always returns false from shouldComponentUpdate */}
      {value}
    </Indirection>
  )}
</ContextConsumer>

I hope you can see that there's no way value would update, with either old or new context.

It wouldn't even update if there was no context usage at all:

function MyLabel(props) {
  return (
    <Indirection> {/* always returns false from shouldComponentUpdate */}
      {props.value}
    </Indirection>
  );
}

Does this make more sense now?

New context doesn't force every single component below to update, just in case some of those components happen to pass that context value down and later render it.

New context just fixes an issue where context consumers wouldn't get updated. From them and down, everything flow like normal React, subject to normal React rules.

Another way to explain this is that your example would work if you changed Indirection to shallowly compare props (e.g. by inheriting from PureComponent). Then it would "notice" the children element has changed. With old context, PureComponent wouldn't work there, but with the new one, it does.

[acdlite]

@thysultan An easy way to confirm that the consumer re-renders is to put a log statement inside its render prop.

[thysultan]

DataConsumer did re-render. There's no reason why it would update the onClick function though because Indirection says "I never update to new children".

I'm assuming that if it re-renders then it also reconciles the difference...

<Indirection>
  <DataConsumer>
    {nestedController => (...)}

That is to say, it calls the render prop function and reconciles that part of the tree using the new return value.

I hope you can see that there's no way value would update, with either old or new context.

The Indirection is outside of DataConsumer so i'm not sure that example would apply in this case.

[gaearon]

It applies in your case, it's just not very obvious why.

The root of the issue is you're referencing top-level topController value from that nested closure at the bottom. But Indirection doesn't update its children. The context consumer in it re-rendered, but because Indirection refused to update, the topControlled value in the closure is stale. So it just uses the previous value again.

You'd have the exact same problem if you just closed over some variable at the top of the render method, and then have that "blocking" indirection in the middle. Even if a component deeper than the indirection re-rendered, it would still have the "stale" top-level value from a stale closure.

[gaearon]

To anyone reading this later, to avoid this problem always make sure that you either don't write shouldComponentUpdate by hand, or, if you do, that you return true when this.props.children !== prevProps.children. In this example, Indirection "swallowed" the new closure.

[thysultan]

Thanks, that makes sense.

[tkh44]

For anyone that wants to play around with this concept, I made a sandbox that might be useful.

https://codesandbox.io/s/k0lmr9xn57

[Nirvananirvan]

Lets me

[gaearon]

Closing since it doesn't seem to come up often.
Also we track docs issues in https://github.com/reactjs/reactjs.org now.