Skip to content

Move data fetching into FlightCard#1

Open
unstubbable wants to merge 3 commits intomainfrom
colocate-data-fetching
Open

Move data fetching into FlightCard#1
unstubbable wants to merge 3 commits intomainfrom
colocate-data-fetching

Conversation

@unstubbable
Copy link
Copy Markdown
Owner

@unstubbable unstubbable commented Mar 19, 2024

How we got here (commits on main)

Reproducing the issue (this PR)

When moving the data fetching from render into the FlightCard component, a race condition is triggered where, under certain circumstances, e.g. when starting the server with a production build, or when delaying the data fetching, the rendered element is unmounted again.

Here's how it should behave:

dev.mov

And here it's failing:

prod.mov

This is the RSC response for the successful run:

0:["$@1",["development",null]]
3:"$Sreact.suspense"
4:D{"name":"","env":"Server"}
1:["$@2",{"id":1710917980696,"display":["$","$3",null,{"fallback":"$undefined","children":"$L4"}]}]
2:{"0":[{"role":"user","content":"flight 42"}],"1":[{"role":"function","name":"get_flight_info","content":"TODO"}],"_t":"a"}
5:D{"name":"FlightCard","env":"Server"}
6:D{"name":"","env":"Server"}
4:["$","$3",null,{"fallback":"$L5","children":"$L6"}]
5:["$","div",null,{"children":[["$","h2",null,{"children":"Flight Information"}],["$","p",null,{"children":["Flight Number: ","42"]}],["$","p",null,{"children":["Departure: ","New York"]}],["$","p",null,{"children":["Arrival: ","San Francisco"]}]]}]
6:"$5"

And this is the RSC response for the failed run:

0:["$@1",["development",null]]
3:"$Sreact.suspense"
4:D{"name":"","env":"Server"}
1:["$@2",{"id":1710918908284,"display":["$","$3",null,{"fallback":"$undefined","children":"$L4"}]}]
2:{"0":[{"role":"user","content":"flight 42"}],"1":[{"role":"function","name":"get_flight_info","content":"TODO"}],"_t":"a"}
5:D{"name":"FlightCard","env":"Server"}
6:D{"name":"","env":"Server"}
4:["$","$3",null,{"fallback":"$L5","children":"$L6"}]
6:"$5"
5:["$","div",null,{"children":[["$","h2",null,{"children":"Flight Information"}],["$","p",null,{"children":["Flight Number: ","42"]}],["$","p",null,{"children":["Departure: ","New York"]}],["$","p",null,{"children":["Arrival: ","San Francisco"]}]]}]

Note that the last two RSC rows are switched here. When I add a breakpoint to setMessages, or delay the setter with a timeout, this response is also handled correctly, though.

Comment thread app/action.tsx
role: "function",
name: "get_flight_info",
content: JSON.stringify(flightInfo),
content: "TODO",
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm aware that this is not trivial to solve (one approach here), so there's a chance that this may be an anti-pattern.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But giving up colocation of data fetching in server components, when using them in an AI app (as shown in https://sdk.vercel.ai/docs/concepts/ai-rsc#nested-ui-streaming), would also be unfortunate.

@unstubbable unstubbable force-pushed the colocate-data-fetching branch from c1a0fc5 to 4229a53 Compare March 20, 2024 08:24
@unstubbable
Copy link
Copy Markdown
Owner Author

unstubbable commented Mar 20, 2024

Hm, interesting, this might be a bug in ReactFlightClient. For some reason, in the failed case, the last chunk ($5) is resolved as fulfilled, but having no value, and instead the rejectListeners as reason.

Failure:

Screenshot 2024-03-20 at 09 54 58

Success:

Screenshot 2024-03-20 at 09 53 42

@unstubbable
Copy link
Copy Markdown
Owner Author

Wait, shouldn't it be 6:"$L5" instead of 6:"$5", because that's really a lazy node?

@unstubbable
Copy link
Copy Markdown
Owner Author

Wait, shouldn't it be 6:"$L5" instead of 6:"$5", because that's really a lazy node?

Indeed, if I manually patch the response, the bug is fixed.

Screenshot 2024-03-20 at 13 15 19

After having looked into https://github.com/vercel/ai/blob/main/packages/core/rsc/streamable.tsx and https://github.com/vercel/ai/blob/main/packages/core/rsc/utils.tsx, I'm not sure this can be fixed in ai/rsc.

It might instead be a bug in ReactFlightServer, maybe?

@unstubbable
Copy link
Copy Markdown
Owner Author

unstubbable commented Mar 20, 2024

Ok, I think I've figured it out. The bug was introduced by facebook/react#28283. If I revert this change locally, the bug is fixed.

Reading the PR description and the added code comments, there doesn't appear to be a simple fix, as it would be required to track which rows have been emitted already to decide whether to use serializeLazyID or serializeByValueID.

sebmarkbage pushed a commit to facebook/react 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
github-actions Bot pushed a commit to facebook/react 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)
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant