Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Context Provider may block Old context propagation if children are constant #12551

Closed
Jessidhia opened this issue Apr 5, 2018 · 6 comments · Fixed by #12586
Closed

New Context Provider may block Old context propagation if children are constant #12551

Jessidhia opened this issue Apr 5, 2018 · 6 comments · Fixed by #12586

Comments

@Jessidhia
Copy link
Contributor

Jessidhia commented Apr 5, 2018

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

Bug

What is the current behavior?

It seems that, if the children of a new-style React.createContext() context Provider are constant, the Provider can block updates from old-style this.context context providers from propagating to this.context consumers.

This sandbox demonstrates the issue. Clicking the button with a number will correctly increment the Root's state and context, but the update is only propagated to the Child3's context (and its button) when the "Colors!" button is clicked, as it causes an update to the value of the new-style Provider:

https://codesandbox.io/s/ol4lpokpjy

Copy of the source code in the sandbox
import PropTypes from "prop-types";
import React from "react";
import ReactDOM from "react-dom";

class Root extends React.Component {
  constructor(props: {}) {
    super(props);
    this.state = {
      count: 0
    };
    this.countUp = this.countUp.bind(this);
  }

  getChildContext() {
    return {
      ...this.context,
      count: this.state.count,
      countUp: this.countUp
    };
  }

  render() {
    return this.props.children;
  }

  countUp() {
    this.setState(({ count }) => ({ count: count + 1 }));
  }
}

Root.childContextTypes = {
  count: PropTypes.number.isRequired,
  countUp: PropTypes.func.isRequired
};

const ctx = React.createContext();

class Child1 extends React.Component {
  constructor(props: { onClick(): void }) {
    super(props);
    this.state = {
      color: randomHexColor(),
      newColor: this.newColor.bind(this)
    };
  }

  render() {
    return (
      <ctx.Provider value={this.state}>{this.props.children}</ctx.Provider>
    );
  }

  newColor() {
    const color = randomHexColor();
    this.setState(() => ({ color }));
  }
}

function randomHexColor() {
  const colorStr = Math.floor(Math.random() * (Math.pow(2, 24) - 1)).toString(
    16
  );
  return "#000000".slice(0, -colorStr.length) + colorStr;
}

class Child2 extends React.Component {
  render() {
    return (
      <ctx.Consumer>
        {ctx => (
          <React.Fragment>
            <Child3 color={ctx.color} />
            <button onClick={ctx.newColor}>Colors!</button>
          </React.Fragment>
        )}
      </ctx.Consumer>
    );
  }
}

class Child3 extends React.Component {
  render() {
    return (
      <button
        style={{ color: this.props.color }}
        onClick={this.context.countUp}
      >
        {this.context.count}
      </button>
    );
  }
}

Child3.contextTypes = {
  count: PropTypes.number.isRequired,
  countUp: PropTypes.func.isRequired
};

ReactDOM.render(
  <Root>
    <Child1>
      <Child2 />
    </Child1>
  </Root>,
  document.getElementById("root")
);

What is the expected behavior?

Both old-style and new-style context updates should coexist.

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

16.3.1; also broken in 16.3.0.


This seems to only happen if the children of the Provider are constant, which is what happens when the children are provided on the first and only ReactDOM.render call. If Child1 is updated to directly use <Child2/> instead of {this.props.children}, the problem does not happen.

This can also be a problem when using a production optimization that hoists constant elements outside the Component if the specified children are constant, which would even defeat the fix/workaround for the example above.

@Jessidhia Jessidhia changed the title New Context Provider may stop Old context propagation under certain circumstances New Context Provider may block Old context propagation if children are constant Apr 5, 2018
@nmain
Copy link

nmain commented Apr 5, 2018

This is a weakness in the old context API. While you're seeing the problem manifest when combined with the new context API, it's not specific to that. In general, distant children can have updates using the old context API blocked if more immediate children decline to rerender. More about the issue in the old context API docs: https://reactjs.org/docs/legacy-context.html#updating-context

The usual way to solve this was to implement your own registration/observation system and send it through context. The context would contain a callback that consumers could use to register themselves, and then when the parent wanted to push a new update, it would send a signal to all registered consumers, which could then force the rerendering of their own children with forceUpdate or setState. This is how popular libraries like react-redux implemented their connect HOCs; you can see an example here: https://github.com/reactjs/react-redux/blob/master/src/components/connectAdvanced.js

Simply returning different values to getChildContext() is and has always been unreliable. If it worked for you, then it was because your components were already rerendering for some other reason.

@Jessidhia
Copy link
Contributor Author

This was actually encountered when using react-router; Routes were not updating anymore after I updated one of my custom context providers that sit between Router and the app core to the new API.

@gaearon
Copy link
Collaborator

gaearon commented Apr 5, 2018

I'll tag as a bug although I'm not sure. Can you create a test case in ReactNewContext-test.internal.js?

My guess is that maybe a condition like this is also needed before bailouts here, here, and here.

But I’m not sure.

@acdlite

thysultan added a commit to dyo/dyo that referenced this issue Apr 5, 2018
- Revert to previous context implementation that does not rely on the
old context API to ensure both API’s can co-exist, related:
facebook/react#12551 (comment)

- Support `defaultValue` on select elements.

- Fix getChildContext’s update phase invocation to happen after state
has updated but before “render”.
@Jessidhia
Copy link
Contributor Author

I am just guessing, but I assume that this is using a similar bailout logic as PureComponent that also bails out if children is constant.

Maybe this actually will be the right thing to do once the old context API is gone.

@gaearon
Copy link
Collaborator

gaearon commented Apr 6, 2018

I suggested a fix in the comment above. This probably won't be a priority for us but if you send a PR we with tests can take it.

@gaearon
Copy link
Collaborator

gaearon commented May 24, 2018

Should be fixed in React 16.4.
https://reactjs.org/blog/2018/05/23/react-v-16-4.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants