Fix: Synchronous popstate transitions #30759
Merged
Conversation
This file contains hidden or 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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: b57d282369b3b3232d8fed537f5aaf0156430d63...7dd999a5a8852f3c40e9522b00602afae658f6a2 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show |
eps1lon
reviewed
Aug 22, 2024
This is a refactor of the fix in facebook#27505. When a transition update is scheduled by a popstate event, (i.e. a back/ forward navigation) we attempt to render it synchronously even though it's a transition, since it's likely the previous page's data is cached. In facebook#27505, I changed the implementation so that it only "upgrades" the priority of the transition for a single attempt. If the attempt suspends, say because the data is not cached after all, from then on it should be treated as a normal transition. But it turns out facebook#27505 did not work as intended, because it relied on marking the root with pending synchronous work (root.pendingLanes), which was never cleared until the popstate update completed. The test scenarios I wrote accidentally worked for a different reason related to suspending the work loop, which I'm currently in the middle of refactoring.
acdlite
force-pushed
the
fix-synchronous-popstate
branch
from
990acf1 to
ac17c69
Compare
eps1lon
approved these changes
Aug 23, 2024
Comment on lines
+590
to
+596
| function getLanesOfEqualOrHigherPriority(lanes: Lane | Lanes): Lanes { | ||
| // Create a mask with all bits to the right or same as the highest bit. | ||
| // So if lanes is 0b100, the result would be 0b111. | ||
| // If lanes is 0b101, the result would be 0b111. | ||
| const lowestPriorityLaneIndex = 31 - clz32(lanes); | ||
| return (1 << (lowestPriorityLaneIndex + 1)) - 1; | ||
| } |
Collaborator
There was a problem hiding this comment.
That makes sense to me now 👍🏻
github-actions bot
pushed a commit
that referenced
this pull request
Aug 23, 2024
This is a refactor of the fix in #27505. When a transition update is scheduled by a popstate event, (i.e. a back/ forward navigation) we attempt to render it synchronously even though it's a transition, since it's likely the previous page's data is cached. In #27505, I changed the implementation so that it only "upgrades" the priority of the transition for a single attempt. If the attempt suspends, say because the data is not cached after all, from then on it should be treated as a normal transition. But it turns out #27505 did not work as intended, because it relied on marking the root with pending synchronous work (root.pendingLanes), which was never cleared until the popstate update completed. The test scenarios I wrote accidentally worked for a different reason related to suspending the work loop, which I'm currently in the middle of refactoring. DiffTrain build for commit ee7f675.
github-actions bot
pushed a commit
that referenced
this pull request
Aug 23, 2024
This is a refactor of the fix in #27505. When a transition update is scheduled by a popstate event, (i.e. a back/ forward navigation) we attempt to render it synchronously even though it's a transition, since it's likely the previous page's data is cached. In #27505, I changed the implementation so that it only "upgrades" the priority of the transition for a single attempt. If the attempt suspends, say because the data is not cached after all, from then on it should be treated as a normal transition. But it turns out #27505 did not work as intended, because it relied on marking the root with pending synchronous work (root.pendingLanes), which was never cleared until the popstate update completed. The test scenarios I wrote accidentally worked for a different reason related to suspending the work loop, which I'm currently in the middle of refactoring. DiffTrain build for [ee7f675](ee7f675)
This was referenced Aug 27, 2024
acdlite
added a commit
that referenced
this pull request
Sep 4, 2024
### Based on - #30761 - #30759 --- `use` has an optimization where in some cases it can suspend the work loop during the render phase until the data has resolved, rather than unwind the stack and lose context. However, the current implementation is not compatible with sibling prerendering. So I've temporarily disabled it until the sibling prerendering has been refactored. We will add it back in a later step.
github-actions bot
pushed a commit
that referenced
this pull request
Sep 4, 2024
### Based on - #30761 - #30759 --- `use` has an optimization where in some cases it can suspend the work loop during the render phase until the data has resolved, rather than unwind the stack and lose context. However, the current implementation is not compatible with sibling prerendering. So I've temporarily disabled it until the sibling prerendering has been refactored. We will add it back in a later step. DiffTrain build for [8b4c54c](8b4c54c)
github-actions bot
pushed a commit
that referenced
this pull request
Sep 4, 2024
### Based on - #30761 - #30759 --- `use` has an optimization where in some cases it can suspend the work loop during the render phase until the data has resolved, rather than unwind the stack and lose context. However, the current implementation is not compatible with sibling prerendering. So I've temporarily disabled it until the sibling prerendering has been refactored. We will add it back in a later step. DiffTrain build for commit 8b4c54c.
mofeiZ
added a commit
that referenced
this pull request
Oct 11, 2024
### Based on - #30761 - #30759 --- `use` has an optimization where in some cases it can suspend the work loop during the render phase until the data has resolved, rather than unwind the stack and lose context. However, the current implementation is not compatible with sibling prerendering. So I've temporarily disabled it until the sibling prerendering has been refactored. We will add it back in a later step. [ghstack-poisoned]
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
This is a refactor of the fix in #27505.
When a transition update is scheduled by a popstate event, (i.e. a back/ forward navigation) we attempt to render it synchronously even though it's a transition, since it's likely the previous page's data is cached.
In #27505, I changed the implementation so that it only "upgrades" the priority of the transition for a single attempt. If the attempt suspends, say because the data is not cached after all, from then on it should be treated as a normal transition.
But it turns out #27505 did not work as intended, because it relied on marking the root with pending synchronous work (root.pendingLanes), which was never cleared until the popstate update completed.
The test scenarios I wrote accidentally worked for a different reason related to suspending the work loop, which I'm currently in the middle of refactoring.