[Flight] Deduplicate suspended elements #28748
Conversation
unstubbable
force-pushed
the
deduplicate-suspended-elements
branch
from
06bb3fb to
01f0a40
Compare
| @@ -1237,6 +1237,9 @@ function renderModel( | |||
| task.implicitSlot, | |||
| request.abortableTasks, | |||
| ); | |||
| // The suspended element was outlined, so we're using the same ID for the | |||
| // original value, thus ensuring that it's deduplicated if it's referenced again. | |||
| request.writtenObjects.set((value: any), newTask.id); | |||
There was a problem hiding this comment.
There's a couple of things to consider here.
- This value might not be one of the kinds of objects that we stash in this map or even an object at all necessarily. So that would need to be confirmed.
- The thing being executed in the task is really the
model, notvalue. The value could be something we've already rendered past. It would be safer to do this with justmodel. - It's possible for the same element to be rendered inside two different "contexts". We removed Server Context but
keyPathandimplicitSlotis still a contextual thing. You could render the same element in two different key paths.
E.g. this would be safer:
if (wasReactNode && task.keyPath === null && !task.implicitSlot) {
request.writtenObjects.set(model, newTask.id);
}
There was a problem hiding this comment.
See
react/packages/react-server/src/ReactFlightServer.js
Lines 879 to 880 in fd0da3e
There was a problem hiding this comment.
While investigating whether I could avoid the cast to any for value, I also thought I could maybe just use model instead (which, I later noticed, I would also need to cast). But it turns out that these two are not the same because of createLazyWrapperAroundWakeable, and then the deduplication does not work:
{
value: {
'$$typeof': Symbol(react.element),
type: [AsyncFunction: Bar],
key: null,
ref: null,
props: { text: 'bar' }
},
model: {
'$$typeof': Symbol(react.lazy),
_payload: Promise { 'BAR', status: 'pending' },
_init: [Function: readThenable]
}
}There was a problem hiding this comment.
So maybe this is good enough as a heuristic then?
if (wasReactNode && task.keyPath === null && !task.implicitSlot) {
request.writtenObjects.set(value, newTask.id);
}I'm not sure whether wasReactNode always covers value transitively. If not, we could do the same check for value, I guess?
There was a problem hiding this comment.
It's possible for the same element to be rendered inside two different "contexts". We removed Server Context but keyPath and implicitSlot is still a contextual thing. You could render the same element in two different key paths.
Can you expand on this a bit? Using the unit test as an example, implicitSlot is indeed set to true here for <Bar /> because it does not have a key path. Which means, using the proposed condition would only work if we move the code a couple lines further down, after the context has been restored. Would that still be correct?
unstubbable
force-pushed
the
deduplicate-suspended-elements
branch
from
01f0a40 to
718e73a
Compare
|
superseded by #28996 🥳 |
This is a follow-up from the finding I've documented in #28620 (comment):
In #27537 the following statement was made:
So it seems that the deduplication is indeed supposed to work for such cases (as opposed to references to other elements, where only "detriplication" is applied).
This PR ensures that this works if suspended elements are referenced twice within the same model root. This is usually the case when streaming async server components with
ai/rsc, where the element is used in a suspense fallback, and also referenced in its children, see https://github.com/vercel/ai/blob/5e00440e574f78b4129848deb56da8b354d04dc2/packages/core/rsc/utils.tsx#L42-L44.In addition to the unit tests, I've also tested this end-to-end with https://github.com/unstubbable/mfng-ai-demo.