-
Notifications
You must be signed in to change notification settings - Fork 46.3k
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
[Bugfix] Dropped updates inside a suspended tree #18384
Conversation
// TODO: We used to track the lowest pending level for the whole root, but | ||
// we removed it to simplify the implementation. It might be worth adding | ||
// it back for this use case, to avoid redundant passes. |
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.
@sebmarkbage I think I'm going to do this (add back root.lastPendingTime
)
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.
Instead of tracking it globally, why won't we just track it on this suspense boundary in the state?
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'm going to track the "suspended time" locally but to track the "last pending time" locally I'd have to add stuff to the setState
path. Doesn't seem worth it since the bitmask version will be much simpler.
Details of bundled changes.Comparing: e0ab1a4...ae27d3a react-test-renderer
react-dom
react-art
react-native-renderer
react-reconciler
Size changes (experimental) |
Details of bundled changes.Comparing: e0ab1a4...ae27d3a react-dom
react-native-renderer
react-art
react-test-renderer
react-reconciler
Size changes (stable) |
602a821
to
1b3bc39
Compare
// | ||
// As a workaround, we need to recompute the `childExpirationTime` | ||
// by bubbling it up from the next level of children. This is | ||
// based on similar logic in `resetChildExpirationTime`. |
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.
An alternative fix would be to detect this during setState
. I thought it might be better to localize the fix here since it's relatively rare but I'm having second thoughts.
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 think this is the right call to keep it in the rare path.
I think we should just bite the bullet have have Suspense boundaries always have these fragments even when not suspended. It's too much code to manage (buggy and byte size). Especially with enter/exit animations it's going to get even trickier.
It adds a bit more memory but there's not that many suspense boundaries and the ones that do exist fairly often transition through this state anyway.
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.
That would definitely simplify things a lot
Is it worth adding tests for similar scenarios but with updates originating inside the fallback just in case? |
Adds a `resolveText` method as an alternative to using timers. Also removes dependency on react-cache (for just this one test file; can do the others later). Timer option is still there if you provide a `ms` prop.
When there are multiple updates at different priority levels inside a suspended subtree, all but the highest priority one is dropped after the highest one suspends. We do have tests that cover this for updates that originate outside of the Suspense boundary, but not for updates that originate inside. I'm surprised it's taken us this long to find this issue, but it makes sense in that transition updates usually originate outside the boundary or "seam" of the part of the UI that is transitioning.
Fixes a bug where updates inside a suspended tree are dropped because the fragment fiber we insert to wrap the hidden children is not part of the return path, so it doesn't get marked during setState. As a workaround, I recompute `childExpirationTime` right before deciding to bail out by bubbling it up from the next level of children. This is something we should consider addressing when we refactor the Fiber data structure.
This reverts commit 9a54113. I want to use this so we can check if there might be any lower priority updates in a suspended tree. We can remove it again during the expiration times refactor.
We don't currently have an mechanism to check if there are lower priority updates in a subtree, but we can check if there are any in the whole root. This still isn't perfect but it's better than using Idle, which frequently leads to redundant re-renders. When we refactor `expirationTime` to be a bitmask, this will no longer be necessary because we'll know exactly which "task bits" remain.
1b3bc39
to
4ffc49a
Compare
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 ae27d3a:
|
e54aeb9
to
ae27d3a
Compare
Added a fallback test case |
This assignment should have been deleted in #18384. It was deleted in the other branches, but I missed this one. About to open a PR that includes a test that covers this branch.
Fixes #18353.
When there are multiple updates at different priority levels inside a suspended subtree, all but the highest priority one is dropped after the highest one suspends.
We do have tests that cover this for updates that originate outside of the Suspense boundary, but not for updates that originate inside.
I'm surprised it's taken us this long to find this issue, but it makes sense in that transition updates usually originate outside the boundary or "seam" of the part of the UI that is transitioning.
This is related to #18353 but it only fixes part of it. There's another bug, which I believe is related toUpdate: yep, that was it. Pushed another commit that and confirmed that it fixes the sandbox in #18353. CodeSandbox CI is currently down, but when it's back up I'll post an updated sandbox.setState
traversal skipping over the primary fragment fiber. Still working on that one.Also related to #18357, but likewise only fixes part of it.