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

Added a new lifecycle method componentWillReceiveContext() #5776

Closed
wants to merge 1 commit into from
Closed

Added a new lifecycle method componentWillReceiveContext() #5776

wants to merge 1 commit into from

Conversation

milesj
Copy link
Contributor

@milesj milesj commented Jan 4, 2016

I've been working with contexts a lot lately, and have run into a few issues where the current lifecycle isn't perfect. Specifically, I encounter certain situations where componentWillReceiveProps isn't triggered, even though the context has changed: #5756.

This new lifecycle may help with certain situations regarding contexts, like the following issues: #2517 #3973

…s dealing with changed context easier to manage.
@jimfb
Copy link
Contributor

jimfb commented Jan 4, 2016

I don't see how this would help with #2517 or #3973 - they seem orthogonal.

More to the point, I don't think the solution is to add a new lifecycle here. The solution is to fix https://goo.gl/xWSesX to ensure componentWillReceiveProps gets called if the context changes. This appears to be the conclusion of #5756

@@ -145,6 +145,14 @@ void componentDidUpdate(
)
```

Additionally, a new lifecycle method named `componentWillReceiveContext` is included, which is called after `componentWillReceiveProps`, but before the update occurs. This method will *always* be called if contexts are enabled, unlike `componentWillReceiveProps`, which is only called if props change.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no guarantee that componentWillReceiveProps is called only when props change. The only guarantee is that componentWillReceiveProps will be called if props change.

@milesj
Copy link
Contributor Author

milesj commented Jan 5, 2016

I'm ok with either keeping componentWillReceiveContext or fixing componentWillReceiveProps. However, as it stants, the name of componentWillReceiveProps should probably just be componentWillReceive, since it's reading from 3 different sources.

Regardless, there is definitely a disconnect in how componentWillReceiveProps works, as logged here: #5756 (comment)

@jimfb
Copy link
Contributor

jimfb commented Jan 5, 2016

@milesj My understanding is that the intended behavior is for componentWillReceiveProps to fire when context changes. Let's do that, since that's the spec'd behavior. We can figure out any renaming / reorganizing of lifecycles separately, but that's likely a more involved conversation.

@milesj
Copy link
Contributor Author

milesj commented Jan 5, 2016

Off the top of my head, we might be able to change this line here: https://github.com/facebook/react/blob/master/src/renderers/shared/reconciler/ReactCompositeComponent.js#L657

To do some kind of check that compares the previous and next contexts.

if (prevParentElement === nextParentElement && (nextContext === emptyObject || !this.diff(nextContext, prevContext)) {

Will give it a try.

@milesj
Copy link
Contributor Author

milesj commented Jan 6, 2016

Fixed the other issue in this pull request: #5787

@milesj
Copy link
Contributor Author

milesj commented Jan 6, 2016

Going to close this since #5787 was accepted.

@milesj milesj closed this Jan 6, 2016
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