Skip to content
New issue

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

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

Already on GitHub? # to your account

Context providers and consumers should bailout on already finished work #12254

Merged
merged 7 commits into from
Mar 12, 2018

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Feb 20, 2018

Fixes bug where a consumer would re-render even if its props and context had not changed.

@acdlite acdlite requested a review from gaearon February 20, 2018 18:14
@acdlite acdlite changed the title Context providers and consumers should bail-out on already finished work Context providers and consumers should bailout on already finished work Feb 20, 2018
@acdlite
Copy link
Collaborator Author

acdlite commented Mar 6, 2018

@gaearon Ready for review

@bvaughn bvaughn self-requested a review March 12, 2018 17:46
Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

Verified it fixes my issue. Code seems to make sense.

} else if (oldProps === newProps) {
return bailoutOnAlreadyFinishedWork(current, workInProgress);
}
} else if (changedBits !== 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could just be else

@bvaughn
Copy link
Contributor

bvaughn commented Mar 12, 2018

This change seems to make sense to me. 👍

@acdlite
Copy link
Collaborator Author

acdlite commented Mar 12, 2018

@sebmarkbage I'm gonna merge this as-is so I can start the sync. If you have additional feedback, I'll fix in a follow up.

@acdlite acdlite merged commit c7f364d into facebook:master Mar 12, 2018
'A context consumer expects a single child that is a function. ' +
'If you did pass a function, make sure there is no trailing or leading whitespace around it.',
if (__DEV__) {
warning(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we move this to context creation then? Or do we already warn there?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Context creation is too early. Needs to check on every render. Like a prop type.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean createElement when we know type is a context consumer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah I see. Idk, not sure what the advantages are. I thought about using propTypes but if the plan is to remove those eventually? Although maybe this is an indication that would shouldn't...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Idk, not sure what the advantages are.

You get a JS stack trace pointing where it happened.

LeonYuAng3NT pushed a commit to LeonYuAng3NT/react that referenced this pull request Mar 22, 2018
…rk (facebook#12254)

* Context providers and consumers should bail-out on already finished work

Fixes bug where a consumer would re-render even if its props and context
had not changed.

* Encode output as JSON string

* Add weights to random action generator

* Add context to triangle fuzz tester

* Move bailouts to as early as possible

* Bailout if neither context value nor children haven't changed (sCU)

* Change prop type invariant to a DEV-only warning
rhagigi pushed a commit to rhagigi/react that referenced this pull request Apr 19, 2018
…rk (facebook#12254)

* Context providers and consumers should bail-out on already finished work

Fixes bug where a consumer would re-render even if its props and context
had not changed.

* Encode output as JSON string

* Add weights to random action generator

* Add context to triangle fuzz tester

* Move bailouts to as early as possible

* Bailout if neither context value nor children haven't changed (sCU)

* Change prop type invariant to a DEV-only warning
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants