-
Notifications
You must be signed in to change notification settings - Fork 46.8k
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
Fix propagation of legacy context when used with memo #31273
base: 18-3-1
Are you sure you want to change the base?
Fix propagation of legacy context when used with memo #31273
Conversation
At work our 18.2.0 app has a few remaining uses of legacy context, and @MarshallOfSound and I noticed a couple bugs: * [too many rerenders] memo does not always prevent its descendant function components from rerendering if legacy context has changed above * [too few rerenders] memo can incorrectly prevent its descendant legacy context consumers from receiving legacy context updates The added test demonstrates both issues. Prior to my changes to ReactFiberBeginWork, `'Second'` is never received by the consumer, and `Intermediate` _sometimes_ rerenders unnecessarily (iff SimpleMemoComponent is not active). Previously, if hasLegacyContextChanged() then we would always consider didUpdateWork = true, even if oldProps === newProps. This was necessary in order to make sure that we don't go down the bailoutOnAlreadyFinishedWork path which would prevent descendants from receiving updates. I changed it so that bailoutOnAlreadyFinishedWork doesn't reuse the child fibers if hasLegacyContextChanged(), much like if there are updates scheduled lower in the subtree or modern context changes, and I changed beginWork so that we bailout unless the particular component we're on is one that can consume legacy context (or provide it, since that's necessary for merging). Ideally we wouldn't even run legacy context consumers that are subscribed to different context keys than the ones that changed but I don't care that much. Tests all pass except two in ReactIncremental-test that are trying to make sure that sCU returning false can block descendants from receiving a legacy context update. That seems ridiculous to me so IMO it's more correct that these fail now. I don't actually expect we'll land or release this but wanted to put it up for posterity (and a check of my work, if anyone is feeling generous).
@sophiebits is attempting to deploy a commit to the Meta Open Source Team on Vercel. A member of the Team first needs to authorize it. |
(workInProgress.tag === ClassComponent | ||
? !!workInProgress.type.contextTypes || | ||
!!workInProgress.type.childContextTypes | ||
: workInProgress.tag === FunctionComponent || | ||
workInProgress.tag === SimpleMemoComponent | ||
? !!workInProgress.type.contextTypes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sloppy but who carez
// TODO: it's unfortunate that we can't reuse work on | ||
// these components even though they don't depend on context. | ||
'IndirectionFn {}', | ||
'IndirectionClass {}', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed :)
sharedTests('normal, force no simplememo', (component, compare) => { | ||
const Memo = React.memo(component, compare ?? shallowEqual); | ||
function Indirection(props) { | ||
return <Memo {...props} />; | ||
} | ||
return React.lazy(() => fakeImport(Indirection)); | ||
}); | ||
sharedTests('lazy, force no simplememo', (component, compare) => { | ||
const Memo = React.memo(component, compare ?? shallowEqual); | ||
return React.lazy(() => fakeImport(Memo)); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe I should land this regardless (existing tests do all pass, thankfully)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Legacy context should be deleted and it only exists for Meta's internal usage until they can delete it. "Fixing" the backwards compatible behavior pre-Fiber could break something in their code which is the only reason this code still exists.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
At work our 18.2.0 app has a few remaining uses of legacy context, and @MarshallOfSound and I noticed a couple bugs:
The added test demonstrates both issues. Prior to my changes to ReactFiberBeginWork,
'Second'
is never received by the consumer, andIntermediate
sometimes rerenders unnecessarily (iff SimpleMemoComponent is not active).Previously, if hasLegacyContextChanged() then we would always consider didUpdateWork = true, even if oldProps === newProps. This was necessary in order to make sure that we don't go down the bailoutOnAlreadyFinishedWork path which would prevent descendants from receiving updates.
I changed it so that bailoutOnAlreadyFinishedWork doesn't reuse the child fibers if hasLegacyContextChanged(), much like if there are updates scheduled lower in the subtree or modern context changes, and I changed beginWork so that we bailout unless the particular component we're on is one that can consume legacy context (or provide it, since that's necessary for merging).
Ideally we wouldn't even run legacy context consumers that are subscribed to different context keys than the ones that changed but I don't care that much.
Tests all pass except two in ReactIncremental-test that are trying to make sure that sCU returning false can block descendants from receiving a legacy context update. That seems ridiculous to me so IMO it's more correct that these fail now. I don't actually expect we'll land or release this but wanted to put it up for posterity (and a check of my work, if anyone is feeling generous).