fix(react): Change Renderer from createRoot.render to renderToReadableStream & hydrateRoot
#30419
Conversation
NEKOYASAN
force-pushed
the
fix/30317-migrate-renderer-to-readable-stream-hydrate
branch
3 times, most recently
from
7983529 to
6b4230a
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile
| export const WithAsyncFunction = async ({ children }: { children: any }) => { | ||
| await new Promise((resolve) => setTimeout(resolve, 1000)); |
There was a problem hiding this comment.
style: children prop should be properly typed instead of using 'any'
| export const WithAsyncFunction = async ({ children }: { children: any }) => { | |
| await new Promise((resolve) => setTimeout(resolve, 1000)); | |
| export const WithAsyncFunction = async ({ children }: { children: React.ReactNode }) => { | |
| await new Promise((resolve) => setTimeout(resolve, 1000)); |
| const stream = await ReactDOMServer.renderToReadableStream(node, { | ||
| // @ts-expect-error onCaughtError in hydrationRoot and createRoot (React 19 only) | ||
| onError: rootOptions?.onCaughtError, | ||
| }); |
There was a problem hiding this comment.
logic: The onError handler is marked as React 19 only but no version check is performed. This could cause runtime errors in React 18.
| let root = nodes.get(el); | ||
|
|
||
| if (root) { | ||
| root.unmount(); | ||
| } |
There was a problem hiding this comment.
style: Unmounting the existing root before the stream is ready could cause a flash of empty content.
There was a problem hiding this comment.
LGTM
4 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
|
Test logs``` > yarn playwright testRunning 152 tests using 4 workers
3 failed |
valentinpalkovic
added
the
ci:daily
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 108 | 108 | 0 |
| Self size | 17 KB | 17 KB | 0 B |
| Dependency size | 42.63 MB | 42.61 MB | 🎉 -18 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-dom-shim
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 0 | 0 |
| Self size | 10 KB | 11 KB | 🚨 +1 KB 🚨 |
| Dependency size | 792 B | 792 B | 0 B |
| Bundle Size Analyzer | Link | Link |
|
What's blocking this PR? Any way to help? |
|
@FezVrasta I was too busy with my day job to work on it. The first thing I need to fix with this PR is that this e2e test fails, and I'm having trouble figuring out how to fix the |
|
Thanks for the write up. Yes the double decorator call is definitely because it's first called during rendering and then during hydration. I think it would make sense to update the test to account for this? Alternatively rather than checking if the decorator is called, the test could check if some client-side only code is executed (for example a spy called in a useEffect). |
|
Thank you very much. I'm also a little concerned about the |
NEKOYASAN
force-pushed
the
fix/30317-migrate-renderer-to-readable-stream-hydrate
branch
from
0d5b5f9 to
5d08f1d
Compare
|
@FezVrasta Thank you so much for your advice. I have three questions. If anyone knows anything about this, please let me know.
|
|
I'm testing your patch on a large project and many stories are failing with This error is explained quite clearly on this blog post https://dev.to/codewithvick/how-to-fix-the-detected-multiple-renderers-concurrently-rendering-the-same-context-provider-4o09 I'm not sure how to solve it though. |
|
I had this happen before with Sandbox and fix it... My storybook environment is not working in |
|
Can you share some context on how did you fix it earlier? I don't quite understand why this error is happening, your code looks good to me, you take the react node, render it "server side", put its result in the node html, and then hydrate it. |
|
There was due to the fact that multiple renderers were created by not unmounting correctly when re-rendering. Since |
|
Do you think a queue could help? Where we wait for the previous |
|
I tried a quick and dirty queue approach but it doesn't solve the issue 🤔 diff --git a/code/lib/react-dom-shim/src/react-18.tsx b/code/lib/react-dom-shim/src/react-18.tsx
index e7a131fa40..d4712cbe59 100644
--- a/code/lib/react-dom-shim/src/react-18.tsx
+++ b/code/lib/react-dom-shim/src/react-18.tsx
@@ -5,6 +5,50 @@ import type { Root as ReactRoot, RootOptions } from 'react-dom/client';
import * as ReactDOM from 'react-dom/client';
import * as ReactDOMServer from 'react-dom/server';
+class PromiseQueue {
+ private queue: (() => Promise<any>)[] = [];
+
+ private isProcessing: boolean = false;
+
+ // Method to add a new task to the queue
+ public add(task: () => Promise<any>): Promise<any> {
+ return new Promise((resolve, reject) => {
+ this.queue.push(() => task().then(resolve).catch(reject));
+ this.processQueue();
+ });
+ }
+
+ // Method to process the queue
+ private async processQueue(): Promise<void> {
+ if (this.isProcessing) {
+ return;
+ } // Prevent multiple processing // Prevent multiple processing
+
+ this.isProcessing = true;
+
+ while (this.queue.length > 0) {
+ const task = this.queue.shift(); // Get the next task
+ if (task) {
+ try {
+ await task(); // Execute the task
+ } catch (error) {
+ console.error('Task failed:', error);
+ }
+ }
+ }
+
+ this.isProcessing = false; // Reset processing state
+ }
+}
+
+// Create an instance of the queue
+const queue = new PromiseQueue();
+
+// Function to run tasks in the queue
+async function runInQueue(task: () => Promise<any>): Promise<any> {
+ return queue.add(task);
+}
+
// A map of all rendered React 18 nodes
const nodes = new Map<Element, ReactRoot>();
@@ -53,7 +97,7 @@ export const renderElement = async (
experimentalRSCRenderer?: boolean
) => {
if (experimentalRSCRenderer) {
- await renderElementExperimentalRSC(node, el, rootOptions);
+ await runInQueue(() => renderElementExperimentalRSC(node, el, rootOptions));
return;
} |
Closes #30317
What I did
Rendering asynchronous RSC with Suspense in Client Render causes a re-rendering loop.
Change from
createRoot.renderto usingrenderToReadableStreamandhydrateRootto avoid re-rendering loop.This change only applies to those with
parameters.react.rsc === true.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
nextjs/default-tsstories/frameworks/nextjs/rsc/Defaultstories/frameworks/nextjs/rsc/WithAsyncFunctionRSC<- Impacted step in this PRDocumentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>Greptile Summary
This PR modifies the React Server Components (RSC) rendering approach to fix infinite re-rendering issues with async components.
renderElementExperimentalRSCincode/lib/react-dom-shim/src/react-18.tsxto userenderToReadableStreamandhydrateRootinstead ofcreateRoot.renderrenderToCanvasto conditionally use RSC rendering based onstoryContext?.parameters?.react?.rscWithAsyncFunctionRSCinRSC.stories.tsxwith 1-second delay to validate fixWithAsyncFunctioncomponent inRSC.tsxto demonstrate async RSC functionalityThe changes look appropriate and focused on solving the infinite re-rendering issue while maintaining proper error handling and state management. The test coverage with both sync and async RSC components helps validate the fix.
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!