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

Failing test for Context bug in ReactPartialRenderer #12974

Closed
wants to merge 1 commit into from

Conversation

aweary
Copy link
Contributor

@aweary aweary commented Jun 3, 2018

A failing test for the bug reported in #12968

A summary of the issue is that rendering this combination of consumers and providers for multiple contexts causes a bug where the value from one context is being used in the other in subsequent renders.

This is possible because context is stored on the Context element itself, so it's mutated as the components are rendered. In this case, the wrong value is being set on a provider when its being popped:

if (this.providerIndex < 0) {
context._currentValue = context._defaultValue;
} else {
// We assume this type is correct because of the index check above.
const previousProvider: ReactProvider<any> = (this.providerStack[
this.providerIndex
]: any);
context._currentValue = previousProvider.props.value;
}

previousProvider ends up being the provider for the other context, so the next time it renders it has the wrong value. The assumption that the context type is correct because of the index check isn't accurate. It's also strange that it's always setting the context value to previousProvider.props.value, even if the provider doesn't have a value.

Not sure what the right fix is, but I'd like to figure it out 😄

cc @acdlite @gaearon

@gaearon
Copy link
Collaborator

gaearon commented Jun 5, 2018

It's also strange that it's always setting the context value to previousProvider.props.value, even if the provider doesn't have a value.

What do you mean by "doesn't have a value"?

@aweary
Copy link
Contributor Author

aweary commented Jun 5, 2018

What do you mean by "doesn't have a value"?

@gaearon I mean when a Provider doesn't have a value prop and is relying on the default value passed in to React.createContext

@gaearon
Copy link
Collaborator

gaearon commented Jun 5, 2018

If Provider doesn't have a value prop then you'll get undefined. The defaultValue is only used when there's no matching Provider.

@aweary
Copy link
Contributor Author

aweary commented Jun 5, 2018

@gaearon sure, but it sets _currentValue not _defaultValue

context._currentValue = previousProvider.props.value;

The only place I see _defaultValue being used is when the provider stack is empty

if (this.providerIndex < 0) {
context._currentValue = context._defaultValue;
} else {

So I'm not sure how setting it to undefined is doing anything

@aweary
Copy link
Contributor Author

aweary commented Jun 7, 2018

Closing in favor of #12985

@aweary aweary closed this Jun 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants