Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move data fetching into FlightCard #1

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

unstubbable
Copy link
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.

aiState.done([
...aiState.get(),
{
role: "function",
name: "get_flight_info",
content: JSON.stringify(flightInfo),
content: "TODO",
Copy link
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
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
Copy link
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
Owner Author

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

@unstubbable
Copy link
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
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