[Bugfix] Dropped updates inside a suspended tree #18384
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
React Core Team
acdlite
commented
Mar 25, 2020
Comment on lines
1612
to
1614
| // 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.
@sebmarkbage I think I'm going to do this (add back root.lastPendingTime)
There was a problem hiding this comment.
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.
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) |
acdlite
force-pushed
the
bugfix-dropped-updates-in
branch
from
602a821
to
1b3bc39
Compare
acdlite
commented
Mar 25, 2020
| // | ||
| // 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.
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.
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.
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? |
sebmarkbage
approved these changes
Mar 25, 2020
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.
acdlite
force-pushed
the
bugfix-dropped-updates-in
branch
from
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:
|
acdlite
force-pushed
the
bugfix-dropped-updates-in
branch
from
e54aeb9
to
ae27d3a
Compare
|
Added a fallback test case |
acdlite
added a commit
that referenced
this pull request
Mar 27, 2020
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.setStatetraversal skipping over the primary fragment fiber. Still working on that one.Also related to #18357, but likewise only fixes part of it.