[Fiber][Float] Error when a host fiber changes "flavor" #29693
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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
CLA Signed
React Core Team
gnoff
force-pushed
the
error-on-hoistable-change
branch
from
1ad9eae
to
9543a7d
Compare
|
Comparing: adbec0c...82a61ff 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
|
sebmarkbage
reviewed
May 31, 2024
sebmarkbage
reviewed
May 31, 2024
| (resource === null && current.memoizedState) | ||
| ) { | ||
| throw new Error( | ||
| 'Expected HostHoistable to not change between Instance and Resource type. Try adding a key to this component so that when its props change a new instance is mounted.', |
There was a problem hiding this comment.
This is a public error message likely to be hit by an end user yet it includes three Fiber internal terminologies.
There was a problem hiding this comment.
Yeah so I would like to make it more public friendly but its a reconciler error and it feels odd to be too descriptive about the DOM here. I could implement the error in the host config but then I need to add it to all the others and it still is only going to be used by DOM presumably until the error goes away.
I was thinking of adding a warning that contextualizes the error better inside getResource since that's already in the host config and then this error can be generic but i'm having trouble with non fiber-internal language in the message that is still not host specific
There was a problem hiding this comment.
We should still try to express the message in terms of the possible mistakes that might cause it. It can link to a docs page if the explanation is too verbose.
There was a problem hiding this comment.
Re: host-specific language, it's annoying but the right thing to do is probably a fiber config method.
sebmarkbage
reviewed
May 31, 2024
| if (nextResource === currentResource) { | ||
| workInProgress.flags &= ~MaySuspendCommit; | ||
| } else { | ||
| // This is an update. |
There was a problem hiding this comment.
What all this refactoring about? Does it do anything?
There was a problem hiding this comment.
We no longer support transitioning between types (we error in begin work) so we can simplify the update logic. If we have a resource now then we must have had one before. If we don't have one now we must have not had one before.
this removes a duplicate check of nextResource !== currentResource.
gnoff
force-pushed
the
error-on-hoistable-change
branch
3 times, most recently
from
623d8a6
to
2fea326
Compare
Host Components can exist as four semantic types 1. regular Components (Vanilla) 2. singleton Components 2. hoistable components 3. resources Each of these component types have their own rules related to mounting and reconciliation however they are not direclty modeled as their own unique fiber type. This is partly for code size but also because reconciling the inner type of these components would be in a very hot path in fiber creation and reconciliation and it's just not practical to do this logic check here. Right now we have three Fiber types used to implement these 4 concepts but we probably need to reconsider the model and think of Host Components as a single fiber type with an inner implementation. Once we do this we can regularize things like transitioning between a resource and a regular component or a singleton and a hoistable instance. The cases where these transitions happen today aren't particularly common but they can be observed and currently the handling of these transitions is incmplete at best and buggy at worst. The most egregious case is the link type. This can be a regular component (stylesheet without precedence) a hositable component (non stylesheet link tags) or a resource (stylesheet with a precedence) and if you have a single jsx slot that tries to reconcile transitions between these types it just doesn't work well. This commit adds an error for when a Hoistable goes from Instance to Resource. This is the most urgent because it is the easiest to hit but doesn't add much overhead in hot paths detecting type shifting to and from regular components is harder to do efficiently because we don't want to reevaluate the type on every update for host components which is currently not required and would add overhead to a very hot path singletons can't really type shift in their one practical implementation (DOM) so they are only a problem in theroy not practice
gnoff
force-pushed
the
error-on-hoistable-change
branch
from
2fea326
to
82a61ff
Compare
sebmarkbage
approved these changes
Jun 3, 2024
unstubbable
reviewed
Jun 3, 2024
| + ${describeLinkForResourceErrorDEV(pendingProps)}`; | ||
| } | ||
| throw new Error( | ||
| 'Expected <link> not to update to be updated to a stylehsheet with precedence.' + |
There was a problem hiding this comment.
Suggested change
| 'Expected <link> not to update to be updated to a stylehsheet with precedence.' + | |
| 'Expected <link> to not be updated to a stylesheet with precedence.' + |
| await waitForAll([]); | ||
| if (__DEV__) { | ||
| expect(errors).toEqual([ | ||
| `Expected <link> not to update to be updated to a stylehsheet with precedence. Check the \`rel\`, \`href\`, and \`precedence\` props of this component. Alternatively, check whether two different <link> components render in the same slot or share the same key. |
There was a problem hiding this comment.
Suggested change
| `Expected <link> not to update to be updated to a stylehsheet with precedence. Check the \`rel\`, \`href\`, and \`precedence\` props of this component. Alternatively, check whether two different <link> components render in the same slot or share the same key. | |
| `Expected <link> to not be updated to a stylesheet with precedence. Check the \`rel\`, \`href\`, and \`precedence\` props of this component. Alternatively, check whether two different <link> components render in the same slot or share the same key. |
| ]); | ||
| } else { | ||
| expect(errors).toEqual([ | ||
| 'Expected <link> not to update to be updated to a stylehsheet with precedence. Check the `rel`, `href`, and `precedence` props of this component. Alternatively, check whether two different <link> components render in the same slot or share the same key.', |
There was a problem hiding this comment.
Suggested change
| 'Expected <link> not to update to be updated to a stylehsheet with precedence. Check the `rel`, `href`, and `precedence` props of this component. Alternatively, check whether two different <link> components render in the same slot or share the same key.', | |
| 'Expected <link> to not be updated to a stylesheet with precedence. Check the `rel`, `href`, and `precedence` props of this component. Alternatively, check whether two different <link> components render in the same slot or share the same key.', |
| @@ -512,5 +512,7 @@ | |||
| "524": "Values cannot be passed to next() of AsyncIterables passed to Client Components.", | |||
| "525": "A React Element from an older version of React was rendered. This is not supported. It can happen if:\n- Multiple copies of the \"react\" package is used.\n- A library pre-bundled an old copy of \"react\" or \"react/jsx-runtime\".\n- A compiler tries to \"inline\" JSX instead of using the runtime.", | |||
| "526": "Could not reference an opaque temporary reference. This is likely due to misconfiguring the temporaryReferences options on the server.", | |||
| "527": "Incompatible React versions: The \"react\" and \"react-dom\" packages must have the exact same version. Instead got:\n - react: %s\n - react-dom: %s\nLearn more: https://react.dev/warnings/version-mismatch" | |||
| "527": "Incompatible React versions: The \"react\" and \"react-dom\" packages must have the exact same version. Instead got:\n - react: %s\n - react-dom: %s\nLearn more: https://react.dev/warnings/version-mismatch", | |||
| "528": "Expected <link> not to update to be updated to a stylehsheet with precedence. Check the `rel`, `href`, and `precedence` props of this component. Alternatively, check whether two different <link> components render in the same slot or share the same key.%s", | |||
There was a problem hiding this comment.
Suggested change
| "528": "Expected <link> not to update to be updated to a stylehsheet with precedence. Check the `rel`, `href`, and `precedence` props of this component. Alternatively, check whether two different <link> components render in the same slot or share the same key.%s", | |
| "528": "Expected <link> to not be updated to a stylesheet with precedence. Check the `rel`, `href`, and `precedence` props of this component. Alternatively, check whether two different <link> components render in the same slot or share the same key.%s", |
|
resolved typo in #29732 |
github-actions bot
pushed a commit
that referenced
this pull request
Jun 3, 2024
Host Components can exist as four semantic types 1. regular Components (Vanilla obv) 2. singleton Components 2. hoistable components 3. resources Each of these component types have their own rules related to mounting and reconciliation however they are not direclty modeled as their own unique fiber type. This is partly for code size but also because reconciling the inner type of these components would be in a very hot path in fiber creation and reconciliation and it's just not practical to do this logic check here. Right now we have three Fiber types used to implement these 4 concepts but we probably need to reconsider the model and think of Host Components as a single fiber type with an inner implementation. Once we do this we can regularize things like transitioning between a resource and a regular component or a singleton and a hoistable instance. The cases where these transitions happen today aren't particularly common but they can be observed and currently the handling of these transitions is incomplete at best and buggy at worst. The most egregious case is the link type. This can be a regular component (stylesheet without precedence) a hoistable component (non stylesheet link tags) or a resource (stylesheet with a precedence) and if you have a single jsx slot that tries to reconcile transitions between these types it just doesn't work well. This commit adds an error for when a Hoistable goes from Instance to Resource. Currently this is only possible for `<link>` elements going to and from stylesheets with precedence. Hopefully we'll be able to remove this error and implement as an inner type before we encounter new categories for the Hoistable types detecting type shifting to and from regular components is harder to do efficiently because we don't want to reevaluate the type on every update for host components which is currently not required and would add overhead to a very hot path singletons can't really type shift in their one practical implementation (DOM) so they are only a problem in theroy not practice DiffTrain build for commit 47d0c30.
github-actions bot
pushed a commit
that referenced
this pull request
Jun 3, 2024
Host Components can exist as four semantic types 1. regular Components (Vanilla obv) 2. singleton Components 2. hoistable components 3. resources Each of these component types have their own rules related to mounting and reconciliation however they are not direclty modeled as their own unique fiber type. This is partly for code size but also because reconciling the inner type of these components would be in a very hot path in fiber creation and reconciliation and it's just not practical to do this logic check here. Right now we have three Fiber types used to implement these 4 concepts but we probably need to reconsider the model and think of Host Components as a single fiber type with an inner implementation. Once we do this we can regularize things like transitioning between a resource and a regular component or a singleton and a hoistable instance. The cases where these transitions happen today aren't particularly common but they can be observed and currently the handling of these transitions is incomplete at best and buggy at worst. The most egregious case is the link type. This can be a regular component (stylesheet without precedence) a hoistable component (non stylesheet link tags) or a resource (stylesheet with a precedence) and if you have a single jsx slot that tries to reconcile transitions between these types it just doesn't work well. This commit adds an error for when a Hoistable goes from Instance to Resource. Currently this is only possible for `<link>` elements going to and from stylesheets with precedence. Hopefully we'll be able to remove this error and implement as an inner type before we encounter new categories for the Hoistable types detecting type shifting to and from regular components is harder to do efficiently because we don't want to reevaluate the type on every update for host components which is currently not required and would add overhead to a very hot path singletons can't really type shift in their one practical implementation (DOM) so they are only a problem in theroy not practice DiffTrain build for [47d0c30](47d0c30)
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.
Host Components can exist as four semantic types
Each of these component types have their own rules related to mounting and reconciliation however they are not direclty modeled as their own unique fiber type. This is partly for code size but also because reconciling the inner type of these components would be in a very hot path in fiber creation and reconciliation and it's just not practical to do this logic check here.
Right now we have three Fiber types used to implement these 4 concepts but we probably need to reconsider the model and think of Host Components as a single fiber type with an inner implementation. Once we do this we can regularize things like transitioning between a resource and a regular component or a singleton and a hoistable instance. The cases where these transitions happen today aren't particularly common but they can be observed and currently the handling of these transitions is incomplete at best and buggy at worst. The most egregious case is the link type. This can be a regular component (stylesheet without precedence) a hoistable component (non stylesheet link tags) or a resource (stylesheet with a precedence) and if you have a single jsx slot that tries to reconcile transitions between these types it just doesn't work well.
This commit adds an error for when a Hoistable goes from Instance to Resource. Currently this is only possible for
<link>elements going to and from stylesheets with precedence. Hopefully we'll be able to remove this error and implement as an inner type before we encounter new categories for the Hoistable typesdetecting type shifting to and from regular components is harder to do efficiently because we don't want to reevaluate the type on every update for host components which is currently not required and would add overhead to a very hot path
singletons can't really type shift in their one practical implementation (DOM) so they are only a problem in theroy not practice