Move createElement/JSX Warnings into the Renderer #29088
Merged
+874
−384
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
sebmarkbage
force-pushed
the
elementstacks
branch
from
5a71659
to
7f19699
Compare
sebmarkbage
force-pushed
the
elementstacks
branch
2 times, most recently
from
c8e1421
to
168b54e
Compare
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
sebmarkbage
force-pushed
the
elementstacks
branch
2 times, most recently
from
4a5b219
to
516edd9
Compare
sebmarkbage
force-pushed
the
elementstacks
branch
from
516edd9
to
fe72cfc
Compare
eps1lon
reviewed
May 21, 2024
| 'See https://react.dev/link/warning-keys for more information.', | ||
| gate(flags => flags.enableOwnerStacks) | ||
| ? 'Each child in a list should have a unique "key" prop.' + | ||
| '\n\nCheck the top-level render call using <ParentClient>. ' + |
There was a problem hiding this comment.
I got confused by this for a bit. I thought it suggested I should look inside ParentClient.
What do you think about
Suggested change
| '\n\nCheck the top-level render call using <ParentClient>. ' + | |
| '\n\nCheck the root.render() call using <ParentClient>. ' + |
?
There was a problem hiding this comment.
I want to remove this whole contextual thing once we ship the owner stacks. It makes no sense and is kind of an artifact of trying to explain the actual cause which we don't have. I just didn't want to change it yet to keep parity in the incremental steps. Mainly because of all the tests. The current PR I'm working on I'm manually editing hundreds of tests. I'm actually doing a lot of incremental stuff I wouldn't do just because editing our tests is too much work.
| [root] | ||
| ▾ <Example> ✕ | ||
| ▾ <Example> |
There was a problem hiding this comment.
Any chance we can get these back? It was part of the feature that you could jump to the element in devtools to find the offending element quickly.
There was a problem hiding this comment.
Yea. So this is directly related to this TODO:
react/packages/react-reconciler/src/ReactChildFiber.js
Lines 178 to 180 in fe72cfc
Previously it was the "owner" of the parent which sometimes isn't related to where these elements are created at all. Now it's the actual child that should have the key specified on it but since that's a fake fiber it is not persistent in DevTools. We need to refactor ChildFiber to allow this warning to happen when we have access to the real fiber and it'll be better for context but I'm not planning on doing that yet because the rest of it is too much work already.
Comment on lines
+168
to
+169
| // This only applies to the warning during construction. | ||
| // We do warn if it's actually rendered. |
There was a problem hiding this comment.
Wasn't this just a bug in the test? Component doesn't even accept children here. A more realistic case would probably implement Component to return <div>{children}</div> i.e. the test would end up rendering <div>{[[0, <Component />], [1, <Component />], [2, <Component />]]}</div> instead. Then we always warned because <Component /> was missing a key and i as well.
Feels like we should just add error assertions here and update the test description: #29192
There was a problem hiding this comment.
Well, I think it's still correct that it shouldn't error early in either case - which we have code to ensure in the JSX runtime. So this test is still valuable to assert that - at least until we remove that code all together. We already have tests for testing that maps warn when rendered.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
eps1lon
approved these changes
May 21, 2024
rickhanlonii
approved these changes
May 23, 2024
sebmarkbage
force-pushed
the
elementstacks
branch
from
fe72cfc
to
28c8b7d
Compare
Eventually we should be able to delete describeUnknownElementTypeFrameInDEV since it only has this one callsite.
We validate these in the renderers inherently anyway and the renderers are the only ones that really knows what a valid type for that renderer is. For example the Client Reference check isn't valid for all kinds of configs in Flight. Also, a lot of the checks for valid elements are not valid in Flight e.g. class components. The only reason to do this validation early is because it provides a better stack trace of the callsite for context. However, that's a general problem and the whole point of enableOwnerStacks that it solves this for all kinds of late errors. For now I keep the early validation when enableOwnerStacks is off because in that case we don't have a good stack (but to be fair we don't for any other error neither). Once it is further rolled out we can delete these fully.
We have the same validation in the reconciler and with enableOwnerStacks we'll have the ability to provide a stack trace with the callsite of the element. The way the validation works is that if something is passed through a valid static slot, an element gets marked as valid. If it has been logged as errored before, it gets logged as valid so that future logs don't happen. This change logs for strictly fewer cases - specifically if an element doesn't get rendered/mounted at all. I updated some tests to start rendering where it didn't before.
When React.Children assigns an implicit key we should still warn when the original element should've had a key since if was in an array. This marks such elements as having failed validation even if they have a key so that the error can later be logged in the renderer.
This is just additional for running warnings in the server logs in addition to the ones that happen during hydration but it's good to have both for parity and being able to fail SSR tests early. This also demonstrates per request warning deduping so that they don't just happen once when the server remains alive.
Elements that render Server Components are validated inside Flight. Others pass the validated flag to the client. Instead of running validation in certain cases (block list) we're marking all the cases that don't need keys (allow list) which is tricky since we need to cover all cases. This might lead to false warnings.
Key warnings are supposed to be associated with the child missing the key. It was previously associated with the owner of the parent element which is not really as useful but now it's unfortunately associated with a fake fiber which disappears.
sebmarkbage
force-pushed
the
elementstacks
branch
from
28c8b7d
to
43ceacd
Compare
github-actions bot
pushed a commit
that referenced
this pull request
May 23, 2024
This is necessary to simplify the component stack handling to make way for owner stacks. It also solves some hacks that we used to have but don't quite make sense. It also solves the problem where things like key warnings get silenced in RSC because they get deduped. It also surfaces areas where we were missing key warnings to begin with. Almost every type of warning is issued from the renderer. React Elements are really not anything special themselves. They're just lazily invoked functions and its really the renderer that determines there semantics. We have three types of warnings that previously fired in JSX/createElement: - Fragment props validation. - Type validation. - Key warning. It's nice to be able to do some validation in the JSX/createElement because it has a more specific stack frame at the callsite. However, that's the case for every type of component and validation. That's the whole point of enableOwnerStacks. It's also not sufficient to do it in JSX/createElement so we also have validation in the renderers too. So this validation is really just an eager validation but also happens again later. The problem with these is that we don't really know what types are valid until we get to the renderer. Additionally, by placing it in the isomorphic code it becomes harder to do deduping of warnings in a way that makes sense for that renderer. It also means we can't reuse logic for managing stacks etc. Fragment props validation really should just be part of the renderer like any other component type. This also matters once we add Fragment refs and other fragment features. So I moved this into Fiber. However, since some Fragments don't have Fibers, I do the validation in ChildFiber instead of beginWork where it would normally happen. For `type` validation we already do validation when rendering. By leaving it to the renderer we don't have to hard code an extra list. This list also varies by context. E.g. class components aren't allowed in RSC but client references are but we don't have an isomorphic way to identify client references because they're defined by the host config so the current logic is flawed anyway. I kept the early validation for now without the `enableOwnerStacks` since it does provide a nicer stack frame but with that flag on it'll be handled with nice stacks anyway. I normalized some of the errors to ensure tests pass. For `key` validation it's the same principle. The mechanism for the heuristic is still the same - if it passes statically through a parent JSX/createElement call then it's considered validated. We already did print the error later from the renderer so this also disables the early log in the `enableOwnerStacks` flag. I also added logging to Fizz so that key warnings can print in SSR logs. Flight is a bit more complex. For elements that end up on the client we just pass the `validated` flag along to the client and let the client renderer print the error once rendered. For server components we log the error from Flight with the server component as the owner on the stack which will allow us to print the right stack for context. The factoring of this is a little tricky because we only want to warn if it's in an array parent but we want to log the error later to get the right debug info. Fiber/Fizz has a similar factoring problem that causes us to create a fake Fiber for the owner which means the logs won't be associated with the right place in DevTools. DiffTrain build for commit 84239da.
github-actions bot
pushed a commit
that referenced
this pull request
May 23, 2024
This is necessary to simplify the component stack handling to make way for owner stacks. It also solves some hacks that we used to have but don't quite make sense. It also solves the problem where things like key warnings get silenced in RSC because they get deduped. It also surfaces areas where we were missing key warnings to begin with. Almost every type of warning is issued from the renderer. React Elements are really not anything special themselves. They're just lazily invoked functions and its really the renderer that determines there semantics. We have three types of warnings that previously fired in JSX/createElement: - Fragment props validation. - Type validation. - Key warning. It's nice to be able to do some validation in the JSX/createElement because it has a more specific stack frame at the callsite. However, that's the case for every type of component and validation. That's the whole point of enableOwnerStacks. It's also not sufficient to do it in JSX/createElement so we also have validation in the renderers too. So this validation is really just an eager validation but also happens again later. The problem with these is that we don't really know what types are valid until we get to the renderer. Additionally, by placing it in the isomorphic code it becomes harder to do deduping of warnings in a way that makes sense for that renderer. It also means we can't reuse logic for managing stacks etc. Fragment props validation really should just be part of the renderer like any other component type. This also matters once we add Fragment refs and other fragment features. So I moved this into Fiber. However, since some Fragments don't have Fibers, I do the validation in ChildFiber instead of beginWork where it would normally happen. For `type` validation we already do validation when rendering. By leaving it to the renderer we don't have to hard code an extra list. This list also varies by context. E.g. class components aren't allowed in RSC but client references are but we don't have an isomorphic way to identify client references because they're defined by the host config so the current logic is flawed anyway. I kept the early validation for now without the `enableOwnerStacks` since it does provide a nicer stack frame but with that flag on it'll be handled with nice stacks anyway. I normalized some of the errors to ensure tests pass. For `key` validation it's the same principle. The mechanism for the heuristic is still the same - if it passes statically through a parent JSX/createElement call then it's considered validated. We already did print the error later from the renderer so this also disables the early log in the `enableOwnerStacks` flag. I also added logging to Fizz so that key warnings can print in SSR logs. Flight is a bit more complex. For elements that end up on the client we just pass the `validated` flag along to the client and let the client renderer print the error once rendered. For server components we log the error from Flight with the server component as the owner on the stack which will allow us to print the right stack for context. The factoring of this is a little tricky because we only want to warn if it's in an array parent but we want to log the error later to get the right debug info. Fiber/Fizz has a similar factoring problem that causes us to create a fake Fiber for the owner which means the logs won't be associated with the right place in DevTools. DiffTrain build for [84239da](84239da)
This was referenced May 30, 2024
yungsters
added a commit
that referenced
this pull request
May 30, 2024
## Summary #29088 introduced a regression triggering this warning when rendering flattened positional children: > Each child in a list should have a unique "key" prop. The specific scenario that triggers this is when rendering multiple positional children (which do not require unique `key` props) after flattening them with one of the `React.Children` utilities (e.g. `React.Children.toArray`). The refactored logic in `React.Children` incorrectly drops the `element._store.validated` property in `__DEV__`. This diff fixes the bug and introduces a unit test to prevent future regressions. ## How did you test this change? ``` $ yarn test ReactChildren-test.js ```
github-actions bot
pushed a commit
that referenced
this pull request
May 30, 2024
## Summary #29088 introduced a regression triggering this warning when rendering flattened positional children: > Each child in a list should have a unique "key" prop. The specific scenario that triggers this is when rendering multiple positional children (which do not require unique `key` props) after flattening them with one of the `React.Children` utilities (e.g. `React.Children.toArray`). The refactored logic in `React.Children` incorrectly drops the `element._store.validated` property in `__DEV__`. This diff fixes the bug and introduces a unit test to prevent future regressions. ## How did you test this change? ``` $ yarn test ReactChildren-test.js ``` DiffTrain build for commit 72644ef.
github-actions bot
pushed a commit
that referenced
this pull request
May 30, 2024
## Summary #29088 introduced a regression triggering this warning when rendering flattened positional children: > Each child in a list should have a unique "key" prop. The specific scenario that triggers this is when rendering multiple positional children (which do not require unique `key` props) after flattening them with one of the `React.Children` utilities (e.g. `React.Children.toArray`). The refactored logic in `React.Children` incorrectly drops the `element._store.validated` property in `__DEV__`. This diff fixes the bug and introduces a unit test to prevent future regressions. ## How did you test this change? ``` $ yarn test ReactChildren-test.js ``` DiffTrain build for [72644ef](72644ef)
yungsters
added a commit
that referenced
this pull request
May 31, 2024
## Summary In #29088, the validation logic for `React.Children` inspected whether `mappedChild` — the return value of the map callback — has a valid `key`. However, this deviates from existing behavior which only warns if the original `child` is missing a required `key`. This fixes false positive `key` validation warnings when using `React.Children`, by validating the original `child` instead of `mappedChild`. This is a more general fix that expands upon my previous fix in #29662. ## How did you test this change? ``` $ yarn test ReactChildren-test.js ```
github-actions bot
pushed a commit
that referenced
this pull request
May 31, 2024
## Summary In #29088, the validation logic for `React.Children` inspected whether `mappedChild` — the return value of the map callback — has a valid `key`. However, this deviates from existing behavior which only warns if the original `child` is missing a required `key`. This fixes false positive `key` validation warnings when using `React.Children`, by validating the original `child` instead of `mappedChild`. This is a more general fix that expands upon my previous fix in #29662. ## How did you test this change? ``` $ yarn test ReactChildren-test.js ``` DiffTrain build for commit 8fd963a.
github-actions bot
pushed a commit
that referenced
this pull request
May 31, 2024
## Summary In #29088, the validation logic for `React.Children` inspected whether `mappedChild` — the return value of the map callback — has a valid `key`. However, this deviates from existing behavior which only warns if the original `child` is missing a required `key`. This fixes false positive `key` validation warnings when using `React.Children`, by validating the original `child` instead of `mappedChild`. This is a more general fix that expands upon my previous fix in #29662. ## How did you test this change? ``` $ yarn test ReactChildren-test.js ``` DiffTrain build for [8fd963a](8fd963a)
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 is necessary to simplify the component stack handling to make way for owner stacks. It also solves some hacks that we used to have but don't quite make sense. It also solves the problem where things like key warnings get silenced in RSC because they get deduped. It also surfaces areas where we were missing key warnings to begin with.
Almost every type of warning is issued from the renderer. React Elements are really not anything special themselves. They're just lazily invoked functions and its really the renderer that determines there semantics.
We have three types of warnings that previously fired in JSX/createElement:
It's nice to be able to do some validation in the JSX/createElement because it has a more specific stack frame at the callsite. However, that's the case for every type of component and validation. That's the whole point of enableOwnerStacks. It's also not sufficient to do it in JSX/createElement so we also have validation in the renderers too. So this validation is really just an eager validation but also happens again later.
The problem with these is that we don't really know what types are valid until we get to the renderer. Additionally, by placing it in the isomorphic code it becomes harder to do deduping of warnings in a way that makes sense for that renderer. It also means we can't reuse logic for managing stacks etc.
Fragment props validation really should just be part of the renderer like any other component type. This also matters once we add Fragment refs and other fragment features. So I moved this into Fiber. However, since some Fragments don't have Fibers, I do the validation in ChildFiber instead of beginWork where it would normally happen.
For
typevalidation we already do validation when rendering. By leaving it to the renderer we don't have to hard code an extra list. This list also varies by context. E.g. class components aren't allowed in RSC but client references are but we don't have an isomorphic way to identify client references because they're defined by the host config so the current logic is flawed anyway. I kept the early validation for now without theenableOwnerStackssince it does provide a nicer stack frame but with that flag on it'll be handled with nice stacks anyway. I normalized some of the errors to ensure tests pass.For
keyvalidation it's the same principle. The mechanism for the heuristic is still the same - if it passes statically through a parent JSX/createElement call then it's considered validated. We already did print the error later from the renderer so this also disables the early log in theenableOwnerStacksflag.I also added logging to Fizz so that key warnings can print in SSR logs.
Flight is a bit more complex. For elements that end up on the client we just pass the
validatedflag along to the client and let the client renderer print the error once rendered. For server components we log the error from Flight with the server component as the owner on the stack which will allow us to print the right stack for context. The factoring of this is a little tricky because we only want to warn if it's in an array parent but we want to log the error later to get the right debug info.Fiber/Fizz has a similar factoring problem that causes us to create a fake Fiber for the owner which means the logs won't be associated with the right place in DevTools.