[Flight] Use lazy ID if referenced model chunk was not emitted yet #28620
Conversation
unstubbable
changed the title
unstubbable
changed the title
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.
There was a problem hiding this comment.
Perf wise this isn't the best solution and there's other techniques we can use.
However, the general idea is that this shouldn't matter because the Client is supposed to be resilient to this kind of formatting. I believe the only case it's not is when it's at the root and it might be solvable on the client instead.
Are you suggesting
When what is at the root? The async server component? Or the suspense boundary with the current/next promises? The issue is also reproducible with this: const {pipe} = ReactServerDOMServer.renderToPipeableStream(
<main>{suspendedChunk.row}</main>,
webpackMap,
);...or by nesting the server component into any other elements. I guess you mean something else entirely, with "at the root"? |
|
d8984de <- That's not what you meant, is it? |
|
This should be able to lazily resolve a reference that hasn't resolved yet when the parent is. This can happen for other reasons too like if a dependency on a Client Reference hasn't resolve yet. https://github.com/facebook/react/blob/main/packages/react-client/src/ReactFlightClient.js#L586 However, this works by mutating the parent object. The parent object might be the root object that |
|
Hm, ok, I think I understand. This change in the Flight Client would also make the test pass, and works with the root object. But not using
And in these tests the affected chunks are either I've also checked the Flight Fixture, but If you have a very specific fix in mind, feel free to go ahead and create a PR for that, if you prefer that. I don't want to steal your time and attention from more important things, by having to answer here. I'm just doing this because it's a fun challenge trying to solve the bug I stumbled upon. Final thought: Regardless of whether the Flight Client should be resilient to the order of the chunks, I think it still makes sense to emit references to not emitted models with a lazy ID in the Flight Server. As I've already written in the issue, you also mentioned this in the TODO comment:
I would also be interested in what other technique we could use that would be better performing than using the Thanks for reading this! |
Maybe I misinterpreted this sentence. Do you plan to (eventually) get rid of the lazy IDs completely, and handle all those cases in the client instead (without creating lazy wrappers)? |
I've debugged this now extensively, and I believe that there's more issues with this lazy resolving technique than the root object issue. When I wrap the 
...the root object issue is prevented, but the lazy resolution still does not work (failing with a different error now). It seems that 
Using chunk (And when dealing with the root object, I am still thinking about how the problem can be solved without resorting to returning lazy chunk wrappers for pending chunks in the default case of The added unit test also shows another potential issue here: In production mode, the async server component elements are emitted twice: Whereas in dev mode, due to the Footnotes |
|
After having written all of this, this potential (and maybe naïve) solution materialised in front of my eyes: #28669 |
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
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)
|
Closing for now, as the bug has been fixed with #28669. Using lazy IDs for these model references might still be something we would want to do, but maybe there's a better technique to implement this. |
…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
Summary
With this change, we keep track of emitted model chunk IDs. This allows us to properly decide whether to use a value ID or a lazy ID for a referenced model.
fixes #28595
How did you test this change?
yarn test -r=stable packages/react-server-dom-webpack/src/__tests__/ReactFlightDOM-test.jsyarn test -r=stable --env=production packages/react-server-dom-webpack/src/__tests__/ReactFlightDOM-test.js