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.

Sign up for GitHub

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

Jump to bottom

Spurious context warning when getChildContext isn't pure #3709

Closed
sophiebits opened this issue Apr 21, 2015 · 8 comments
Closed

Spurious context warning when getChildContext isn't pure #3709

sophiebits opened this issue Apr 21, 2015 · 8 comments
Assignees
@jimfb
Labels
Component: Core Utilities

Comments

@sophiebits
Copy link
Collaborator

@sahrens found a case where we had a component that was creating a new object each time getChildContext was called. This isn't optimal for performance because getChildContext is called for each render, but it looks like we're actually calling it twice for each render and warning if the two results aren't the same. This code

var Parent = React.createClass({
  childContextTypes: {
    x: React.PropTypes.number
  },
  getChildContext: function() {
    return {x: Math.random()};
  },
  render: function() {
    return <Child />;
  }
});

var Child = React.createClass({
  contextTypes: {
    x: React.PropTypes.number
  },
  render: function() {
    return <div>x: {this.context.x}</div>;
  }
});

React.render(<Parent />, document.body);

produces warnings like

Warning: owner-based and parent-based contexts differ (values: `0.9166603954508901` vs `0.3767729678656906`) for key (x) while mounting Child (see: http://fb.me/react-context-by-parent)
@jimfb
Copy link
Contributor

jimfb commented Apr 21, 2015

Fixed by https://github.com/facebook/react/pull/3615/files which removes the warning entirely as part of the switch to parent context.

@jimfb jimfb closed this as completed Apr 21, 2015
@sophiebits
Copy link
Collaborator Author

Going to leave this open in case we want to do a patch release for 0.13.

@sophiebits sophiebits reopened this Apr 21, 2015
@zpao
Copy link
Member

zpao commented Apr 21, 2015

We won't take #3615 in 0.13 so it would need to be something different.

@sebmarkbage
Copy link
Collaborator

getChildContext is supposed to be idempotent because it is recalled in the render phase but referential equality might screw up the warning even though they're idempotent - assuming value semantics.

@sophiebits
Copy link
Collaborator Author

Yes, sorry for being unclear – the random() call was just to demonstrate; the original case here instantiated a value-like object each time with the same arguments.

@jimfb
Copy link
Contributor

jimfb commented Apr 21, 2015

The issue is that one context comes from the owner hierarchy and one comes from the parent hierarchy. With value semantics, two distinct objects could represent the same "value" if the auxiliary data doesn't contribute to the user's interpretation of value (eg. one of the values on an object might be a logger or something). We could add some hacks to attempt to be smarter about our definition of equality, but ultimately it is going to be a futile effort to catch everything. We could also add a hack to find the component that provided the variable and bail if it was the same component, but this also has potential issues and might lead to this warning popping up in even more subtle situations.

In terms of prioritization, this is a non fatal warning that only occurs in rare cases (thus we haven't seen it until now). There isn't really a great solution, and any fix would only be useful for a point release before the code is removed entirely. My intuition is that (unless a surprising number of users are hitting this), we might just let people know what's going on and teach them about making their code more idempotent. Either way, I'll update the Gist so people know what's going on. @sebmarkbage, seem sufficient, or should we patch this for a point release?

@jimfb
Copy link
Contributor

jimfb commented Apr 21, 2015

Gist updated.

sophiebits added a commit to sophiebits/react that referenced this issue Apr 21, 2015
@sophiebits
Fix context warning with non-pure getChildContext
f74e40b 
Fixes facebook#3709.
@sophiebits sophiebits mentioned this issue Apr 21, 2015
Closed
sophiebits added a commit to sophiebits/react that referenced this issue Apr 21, 2015
@sophiebits
Fix context warning with non-pure getChildContext
a8c292a 
Fixes facebook#3709.
zpao pushed a commit to zpao/react that referenced this issue May 8, 2015
@sophiebits @zpao
Fix context warning with non-pure getChildContext
80ea01d 
Fixes facebook#3709.
@sophiebits
Copy link
Collaborator Author

Fix is in 0.13.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jimfb jimfb

Labels
Component: Core Utilities
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@sophiebits @zpao @sebmarkbage @jimfb