[Flight] Update stale blocked values in createModelResolver
#28669
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 added test, intended to fail and reproduce the [reported issue](facebook#28595), unexpectedly passes in its current state. I see three possible reasons: 1. The bug report could be invalid. 2. How I've structured the test might be insufficient to replicate what `ai/rsc` is doing. 3. Something in the test setup could be masking the actual error. (Maybe related to fake timers?) If the problem lies in reason 2 or 3, this test could possibly serve as a foundation for further investigation.
unstubbable
force-pushed
the
fix-28595-client-3
branch
from
4e45f45 to
c578cff
Compare
sebmarkbage
reviewed
Mar 30, 2024
|
|
||
| // If this is the root object for a model reference, where `blocked.value` | ||
| // is a stale `null`, the resolved value can be used directly. | ||
| if (key === '' && blocked.value === null) { |
There was a problem hiding this comment.
This is clever. I think this hold true because if the root is blocked, there can't be any other objects that are also blocked. At least for now. Might not hold in the future but should be ok for now at least.
| // were resolved (4th item, key '3'), the props of `blocked.value` must be | ||
| // updated. | ||
| if ( | ||
| Array.isArray(parentObject) && |
There was a problem hiding this comment.
This is an interesting observation. For any parentObject where we later might replace it with another object, this problem occurs. I guess that can really just be React Elements for now.
It seems like in principle it could happen to any of the keys inside of it though. Not just the props.
Also, it seems like in theory it should be able to happen even even if the element is not at the root. I think it's problematic to assume that if the parent object is representing the object that's blocked at the root. If the blocked object is an element that itself has further objects below which itself happens to have an array as the parent object then this would incorrectly match that case. So this doesn't seem safe.
At first, I was confused how this could happen but it's really an artifact of the other problem that the props get outlined separately from the element because when we start outlining an object we've seen everything inside of it before which causes us to outline everything inside of it too which is inefficient because we might not ever see them again outside of this particular object. Which happens to props and elements.
This should ideally have some more clever strategy.
However, one thing we could do is mark props objects inside elements in the outlining as never outlined. E.g. give it id -2 or something. That way they're never outline independently from the element that they're apart of. That way this particular scenario doesn't happen. (Although ideally the parser would be resilient to it.)
There was a problem hiding this comment.
I like the idea of not outlining the props. The condition for updating them in the blocked element felt brittle to me. I've implemented the suggested change in 7d5ad2e.
There was a problem hiding this comment.
What this doesn't do (yet), is prevent the objects inside of the props from being outlined. Is this something you would want to see being done too? To fix the issue, this wouldn't be needed. It might maybe be useful to keep outlining them when the same object is passed to different elements as part of the props. Although it would of course be even better to detect exactly this case, and only then outline them. Not sure if we want to add more complexity for this though.
This avoids having to handle stale blocked elements (with `null` props) in the Flight Client.
Because of the `$$typeof` check above, we know that this is a react element.
|
This is a good start so I'll merge the fix. I think ideally we'd have three things:
|
github-actions bot
pushed a commit
that referenced
this pull request
Apr 1, 2024
Alternative to #28620. Instead of emitting lazy references to not-yet-emitted models in the Flight Server, this fixes the observed issue in unstubbable/ai-rsc-test#1 by adjusting the lazy model resolution in the Flight Client to update stale blocked root models, before assigning them as chunk values. In addition, the element props are not outlined anymore in the Flight Server to avoid having to also handle their staleness in blocked elements. fixes #28595 DiffTrain build for [93f9179](93f9179)
unstubbable
added a commit
to unstubbable/mfng-ai-demo
that referenced
this pull request
Apr 4, 2024
sebmarkbage
added a commit
that referenced
this pull request
Apr 8, 2024
We used to assume that outlined models are emitted before the reference (which was true before Blobs). However, it still wasn't safe to assume that all the data will be available because an "import" (client reference) can be async and therefore if it's directly a child of an outlined model, it won't be able to update in place. This is a similar problem as the one hit by @unstubbable in #28669 with elements, but a little different since these don't follow the same way of wrapping. I don't love the structuring of this code which now needs to pass a first class mapper instead of just being known code. It also shares the host path which is just an identity function. It wouldn't necessarily pass my own review but I don't have a better one for now. I'd really prefer if this was done at a "row" level but that ends up creating even more code. Add test for Blob in FormData and async modules in Maps.
github-actions bot
pushed a commit
that referenced
this pull request
Apr 8, 2024
We used to assume that outlined models are emitted before the reference (which was true before Blobs). However, it still wasn't safe to assume that all the data will be available because an "import" (client reference) can be async and therefore if it's directly a child of an outlined model, it won't be able to update in place. This is a similar problem as the one hit by @unstubbable in #28669 with elements, but a little different since these don't follow the same way of wrapping. I don't love the structuring of this code which now needs to pass a first class mapper instead of just being known code. It also shares the host path which is just an identity function. It wouldn't necessarily pass my own review but I don't have a better one for now. I'd really prefer if this was done at a "row" level but that ends up creating even more code. Add test for Blob in FormData and async modules in Maps. DiffTrain build for [14f50ad](14f50ad)
rickhanlonii
pushed a commit
that referenced
this pull request
Apr 11, 2024
We used to assume that outlined models are emitted before the reference (which was true before Blobs). However, it still wasn't safe to assume that all the data will be available because an "import" (client reference) can be async and therefore if it's directly a child of an outlined model, it won't be able to update in place. This is a similar problem as the one hit by @unstubbable in #28669 with elements, but a little different since these don't follow the same way of wrapping. I don't love the structuring of this code which now needs to pass a first class mapper instead of just being known code. It also shares the host path which is just an identity function. It wouldn't necessarily pass my own review but I don't have a better one for now. I'd really prefer if this was done at a "row" level but that ends up creating even more code. Add test for Blob in FormData and async modules in Maps.
rickhanlonii
pushed a commit
that referenced
this pull request
Apr 11, 2024
We used to assume that outlined models are emitted before the reference (which was true before Blobs). However, it still wasn't safe to assume that all the data will be available because an "import" (client reference) can be async and therefore if it's directly a child of an outlined model, it won't be able to update in place. This is a similar problem as the one hit by @unstubbable in #28669 with elements, but a little different since these don't follow the same way of wrapping. I don't love the structuring of this code which now needs to pass a first class mapper instead of just being known code. It also shares the host path which is just an identity function. It wouldn't necessarily pass my own review but I don't have a better one for now. I'd really prefer if this was done at a "row" level but that ends up creating even more code. Add test for Blob in FormData and async modules in Maps.
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
…ok#28669) Alternative to facebook#28620. Instead of emitting lazy references to not-yet-emitted models in the Flight Server, this fixes the observed issue in unstubbable/ai-rsc-test#1 by adjusting the lazy model resolution in the Flight Client to update stale blocked root models, before assigning them as chunk values. In addition, the element props are not outlined anymore in the Flight Server to avoid having to also handle their staleness in blocked elements. fixes facebook#28595
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
We used to assume that outlined models are emitted before the reference (which was true before Blobs). However, it still wasn't safe to assume that all the data will be available because an "import" (client reference) can be async and therefore if it's directly a child of an outlined model, it won't be able to update in place. This is a similar problem as the one hit by @unstubbable in facebook#28669 with elements, but a little different since these don't follow the same way of wrapping. I don't love the structuring of this code which now needs to pass a first class mapper instead of just being known code. It also shares the host path which is just an identity function. It wouldn't necessarily pass my own review but I don't have a better one for now. I'd really prefer if this was done at a "row" level but that ends up creating even more code. Add test for Blob in FormData and async modules in Maps.
ztanner
added a commit
to vercel/next.js
that referenced
this pull request
Apr 17, 2024
…64271) ### What When rendering a parallel slot multiple times in a single layout, in conjunction with using an error boundary, the following TypeError is thrown: > Cannot destructure property 'parallelRouterKey' of 'param' as it is null ### Why I'm not 100% sure of the reason, but I believe this is because of how React attempts to dededupe (more specifically, "detriplficate") objects that it sees getting passed across the RSC -> client component boundary (and an error boundary is necessarily a client component). When React sees the same object twice, it'll create a reference to that object and then use that reference in future places where it sees the object. My assumption is that there's a bug somewhere here, as the `LayoutRouter` component for the subsequent duplicated parallel slots (after the first one) have no props, hence the TypeError. ### How Rather than passing the error component as a prop to `LayoutRouter`, this puts it as part of the `CacheNodeSeedData` data structure. This is more aligned with other properties anyway (such as `loading` and `rsc` for each segment), and seems to work around this bug as the `initialSeedData` prop is only passed from RSC->client once. EDIT: Confirmed this is also fixed after syncing the latest React, due to facebook/react#28669 Fixes #58485 Closes NEXT-2095
This was referenced Apr 24, 2024
This was referenced May 21, 2024
unstubbable
added a commit
to unstubbable/react
that referenced
this pull request
Jul 30, 2024
When an element is used multiple times as shown in facebook#30526, its props might be deduped. When resolving the reference to the deduped props, we were only updating the React element tuple, which at this point has already been parsed into a React element object, using `null` as placeholder props. Therefore, updating the element tuple doesn't help much, and we need to make sure that the element object's props are updated as well. This is a similar fix as facebook#28669, see the code lines above, and thus feels similarly hacky. Maybe there's a better way to fix this? @eps1lon was mentioning offline that solving [this TODO](https://github.com/facebook/react/blob/33e54fa252b9dbe7553ef42a2287c3dbbd4f035d/packages/react-client/src/ReactFlightClient.js#L1327) would probably fix it properly, since we wouldn't need to deal with stale tuples then. But that's a way heavier lift.
unstubbable
added a commit
to unstubbable/react
that referenced
this pull request
Jul 30, 2024
When an element is used multiple times as shown in facebook#30526, its props might be deduped. When resolving the reference to the deduped props, we were only updating the React element tuple, which at this point has already been parsed into a React element object, using `null` as placeholder props. Therefore, updating the element tuple doesn't help much, and we need to make sure that the element object's props are updated as well. This is a similar fix as facebook#28669, see the code lines above, and thus feels similarly hacky. Maybe there's a better way to fix this? @eps1lon was mentioning offline that solving [this TODO](https://github.com/facebook/react/blob/33e54fa252b9dbe7553ef42a2287c3dbbd4f035d/packages/react-client/src/ReactFlightClient.js#L1327) would probably fix it properly, since we wouldn't need to deal with stale tuples then. But that's a way heavier lift.
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.
Alternative to #28620.
Instead of emitting lazy references to not-yet-emitted models in the Flight Server, this fixes the observed issue in unstubbable/ai-rsc-test#1 by adjusting the lazy model resolution in the Flight Client to update stale blocked root models, before assigning them as chunk values. In addition, the element props are not outlined anymore in the Flight Server to avoid having to also handle their staleness in blocked elements.
fixes #28595