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
Don't bailout after Suspending in Legacy Mode #19216
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 77ec289:
|
Details of bundled changes.Comparing: 47ff31a...77ec289 react-dom
react-native-renderer
react-art
react-test-renderer
react-reconciler
Size changes (experimental) |
Details of bundled changes.Comparing: 47ff31a...77ec289 react-dom
react-art
react-test-renderer
react-native-renderer
react-reconciler
ReactDOM: size: 0.0%, gzip: -0.0% Size changes (stable) |
Seems like I'll need a broader fix with the effect tag so that it works for other Fiber types too. |
438537d
to
ea251a6
Compare
Upd. |
// consumer produces a changed value, it will set this to true. Otherwise, | ||
// the component will assume the children have not changed and bail out. | ||
didReceiveUpdate = false; | ||
if ((current.effectTag & ForceUpdateForLegacySuspense) !== NoEffect) { |
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.
I wouldn't expect this check to be necessary, since whenever you hit this block, you'll always fall into the updateSimpleMemoComponent
path, so you can do everything in there.
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.
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.
This isn't specific to SimpleMemo though. This block is to fix the issue for all other cases (e.g. normal memo, or a function component). See the rest of the tests.
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.
Ah. God Legacy Mode is annoying. OK that makes sense then.
Upd2. I think the generic check is still needed because this is a fix for more than SimpleMemo path. #18844 was about functions in general, with the referential equality bailout as the problem. |
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.
OK I'm cool with this
Fixes #17356.
Fixes #18844.
Has regression tests + verified sandboxes work.
Builds on the test case in #18572 and approach from #18572 (comment).