-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix(ai/rsc): New createStreamableUI
implementation
#1825
Conversation
createStreamableUI
implcreateStreamableUI
implementation
packages/core/rsc/streamable.tsx
Outdated
currentPatchValue = undefined; | ||
currentValue = value; | ||
} else { | ||
throw new Error( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have introduced dedicated errors so we can have error pages for troubleshooting, e.g. https://sdk.vercel.ai/docs/troubleshooting/ai-sdk-errors/ai-invalid-data-content-error
do you think it's worth adding dedicated errors here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! We should convert all the common errors to dedicated error codes, with clear example and explanation.
packages/core/rsc/streamable.tsx
Outdated
} else if (isValidElement(currentValue)) { | ||
if (typeof value === 'string' || isValidElement(value)) { | ||
currentPatchValue = [1, value]; | ||
(currentValue as ReactElement) = ( | ||
<> | ||
{currentValue} | ||
{value} | ||
</> | ||
); | ||
} else { | ||
throw new Error( | ||
`.append(): The value type can't be appended to the stream. Received: ${typeof value}`, | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the check for !(string | validelement)
is duplicated 3 times. you could invert the if-else and pull it out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah part of it is to bypass TS checks as it's not that intelligent to handle the branching if typeof value
is stored as a variable.
export async function streamableUI() { | ||
const streamable = createStreamableUI(); | ||
(async () => { | ||
await sleep(); | ||
await sleep(10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how stable is the test with this low timeout?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite stable. Ran this many times on local/CI and the result is consistent. Here it could be sleep(0)
and it will still work, but decided to add a small delay just in case one layer (Next.js, RSC, React, browser) batches the chunks.
@gaetansnl That sounds like a bug to me and I'll take a look! |
Perfect timing ! I need it for my RSC / gen ui library 🚀 |
Instead of using nested Suspense wrappers, I realized that we can just pass a streamable value of the UI into a Client Component and do the update on the client. This has a lot of advantages, and the major one is that we no longer need to express the UI update logic declaratively via JSX, and React will be able to correctly reconcile the component updates.
Since
createStreamableUI
also powersstreamUI
, that API will benefit from this too.Also moved unit tests of
streamUI
to E2E tests.Closes #1850.