Fix: Stylesheet in error UI suspends indefinitely #27265
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
CLA Signed
React Core Team
acdlite
commented
Aug 22, 2023
| ); | ||
| } | ||
| } | ||
| // We only attach ping listeners in concurrent mode. Legacy Suspense always |
There was a problem hiding this comment.
I moved this into the branches above to avoid attaching a redundant listener to the "noop" thenable. That way we can treat any listener that does get attached as a mistake and warn in dev.
acdlite
commented
Aug 22, 2023
| // NOTE: We do this even for sync updates, for lack of any better option. In | ||
| // the future, we may change how we handle this, like by putting the whole | ||
| // root into a "detached" mode. | ||
| return true; |
There was a problem hiding this comment.
This is the actual fix. It used to return false in this case, now it returns true.
|
Comparing: e76a5ac...b7cf5ba Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
gnoff
approved these changes
Aug 22, 2023
This fixes the regression test added in the previous commit. The "Suspensey commit" implementation relies on the `shouldRemainOnPreviousScreen` function to determine whether to 1) suspend the commit 2) activate a parent fallback and schedule a retry. The issue was that we were sometimes attempting option 2 even when there was no parent fallback. Part of the reason this bug landed is due to how `throwException` is structured. In the case of Suspensey commits, we pass a special "noop" thenable to `throwException` as a way to trigger the Suspense path. This special thenable must never have a listener attached to it. This is not a great way to structure the logic, it's just a consequence of how the code evolved over time. We should refactor it into multiple functions so we can trigger a fallback directly without having to check the type. In the meantime, I added an internal warning to help detect similar mistakes in the future.
acdlite
force-pushed
the
fix-stylesheet-in-error-ui
branch
from
00090a8 to
b7cf5ba
Compare
github-actions bot
pushed a commit
that referenced
this pull request
Aug 22, 2023
This fixes the regression test added in the previous commit. The "Suspensey commit" implementation relies on the `shouldRemainOnPreviousScreen` function to determine whether to 1) suspend the commit 2) activate a parent fallback and schedule a retry. The issue was that we were sometimes attempting option 2 even when there was no parent fallback. Part of the reason this bug landed is due to how `throwException` is structured. In the case of Suspensey commits, we pass a special "noop" thenable to `throwException` as a way to trigger the Suspense path. This special thenable must never have a listener attached to it. This is not a great way to structure the logic, it's just a consequence of how the code evolved over time. We should refactor it into multiple functions so we can trigger a fallback directly without having to check the type. In the meantime, I added an internal warning to help detect similar mistakes in the future. DiffTrain build for [dd480ef](dd480ef)
kodiakhq bot
pushed a commit
to vercel/next.js
that referenced
this pull request
Aug 22, 2023
### React upstream changes - facebook/react#27265 - facebook/react#27259 - facebook/react#27264 - facebook/react#27257 - facebook/react#27258 - facebook/react#27187 - facebook/react#27243 - facebook/react#27205 - facebook/react#27220 - facebook/react#27238 - facebook/react#27234 - facebook/react#27224 - facebook/react#27223 - facebook/react#27222 This will help unblock #53906
kodiakhq bot
pushed a commit
to vercel/next.js
that referenced
this pull request
Aug 23, 2023
### Problem One style of `not-found` has `precendence` property with "undefined" value, which can't be handled by React Float, then during navigation the style could not load properly, lead to the style missing in issue #53210. ### Solution Always enable `precendence` for all links, so all the css styles of page and convention components can be hoist by react properly. Float will decide which one should be handled. Previously this change only applies to template, actually we can apply it to all components so that they can all be handled properly especially during client navigation. Related react change: facebook/react#27265 Fixes #53210
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
This fixes the regression test added in the previous commit. The "Suspensey commit" implementation relies on the `shouldRemainOnPreviousScreen` function to determine whether to 1) suspend the commit 2) activate a parent fallback and schedule a retry. The issue was that we were sometimes attempting option 2 even when there was no parent fallback. Part of the reason this bug landed is due to how `throwException` is structured. In the case of Suspensey commits, we pass a special "noop" thenable to `throwException` as a way to trigger the Suspense path. This special thenable must never have a listener attached to it. This is not a great way to structure the logic, it's just a consequence of how the code evolved over time. We should refactor it into multiple functions so we can trigger a fallback directly without having to check the type. In the meantime, I added an internal warning to help detect similar mistakes in the future.
bigfootjon
pushed a commit
that referenced
this pull request
Apr 18, 2024
This fixes the regression test added in the previous commit. The "Suspensey commit" implementation relies on the `shouldRemainOnPreviousScreen` function to determine whether to 1) suspend the commit 2) activate a parent fallback and schedule a retry. The issue was that we were sometimes attempting option 2 even when there was no parent fallback. Part of the reason this bug landed is due to how `throwException` is structured. In the case of Suspensey commits, we pass a special "noop" thenable to `throwException` as a way to trigger the Suspense path. This special thenable must never have a listener attached to it. This is not a great way to structure the logic, it's just a consequence of how the code evolved over time. We should refactor it into multiple functions so we can trigger a fallback directly without having to check the type. In the meantime, I added an internal warning to help detect similar mistakes in the future. DiffTrain build for commit dd480ef.
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.
This fixes the regression test added in the previous commit. The "Suspensey commit" implementation relies on the
shouldRemainOnPreviousScreenfunction to determine whether to 1) suspend the commit 2) activate a parent fallback and schedule a retry. The issue was that we were sometimes attempting option 2 even when there was no parent fallback.Part of the reason this bug landed is due to how
throwExceptionis structured. In the case of Suspensey commits, we pass a special "noop" thenable tothrowExceptionas a way to trigger the Suspense path. This special thenable must never have a listener attached to it. This is not a great way to structure the logic, it's just a consequence of how the code evolved over time. We should refactor it into multiple functions so we can trigger a fallback directly without having to check the type. In the meantime, I added an internal warning to help detect similar mistakes in the future.