From e26781eacc120f0e526f1be9f45b324e1f03a23b Mon Sep 17 00:00:00 2001 From: eps1lon Date: Tue, 30 Jul 2024 14:56:23 +0200 Subject: [PATCH 1/3] Failing test for deduped props of elements in fragments --- .../__tests__/ReactFlightDOMBrowser-test.js | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js index def347e83a18b..0e44c574413a5 100644 --- a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js +++ b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js @@ -525,6 +525,53 @@ describe('ReactFlightDOMBrowser', () => { expect(container.innerHTML).toBe('{"foo":1}{"foo":1}'); }); + it('should handle deduped props of elements in fragments', async () => { + let resolveFooClientComponentChunk; + + const FooClient = clientExports( + function Foo({children, item}) { + return children; + }, + '1', + '/foo.js', + new Promise(resolve => (resolveFooClientComponentChunk = resolve)), + ); + + const shared =
; + + function Server() { + return ( + + {shared} + + ); + } + + const stream = await serverAct(() => + ReactServerDOMServer.renderToReadableStream(, webpackMap), + ); + + function ClientRoot({response}) { + return use(response); + } + + const response = ReactServerDOMClient.createFromReadableStream(stream); + const container = document.createElement('div'); + const root = ReactDOMClient.createRoot(container); + + await act(() => { + root.render(); + }); + + expect(container.innerHTML).toBe(''); + + await act(() => { + resolveFooClientComponentChunk(); + }); + + expect(container.innerHTML).toBe('
'); + }); + it('should progressively reveal server components', async () => { let reportedErrors = []; From 1e4159e5ac05e2499b80a3c011565e321d4d984c Mon Sep 17 00:00:00 2001 From: Hendrik Liebau Date: Tue, 30 Jul 2024 16:03:00 +0200 Subject: [PATCH 2/3] Assign resolved outlined props to element object (and not only tuple) When an element is used multiple times as shown in #30526, its props might be deduped. When resolving the reference to the deduped props, we were only updating the React element tuple, which at this point has already been parsed into a React element object, using `null` as placeholder props. Therefore, updating the element tuple doesn't help much, and we need to make sure that the element object's props are updated as well. This is a similar fix as #28669, see the code lines above, and thus feels similarly hacky. Maybe there's a better way to fix this? @eps1lon was mentioning offline that solving [this TODO](https://github.com/facebook/react/blob/33e54fa252b9dbe7553ef42a2287c3dbbd4f035d/packages/react-client/src/ReactFlightClient.js#L1327) would probably fix it properly, since we wouldn't need to deal with stale tuples then. But that's a way heavier lift. --- packages/react-client/src/ReactFlightClient.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/packages/react-client/src/ReactFlightClient.js b/packages/react-client/src/ReactFlightClient.js index ade358b4186a2..9c3efc544c063 100644 --- a/packages/react-client/src/ReactFlightClient.js +++ b/packages/react-client/src/ReactFlightClient.js @@ -902,6 +902,20 @@ function waitForReference( handler.value = parentObject[key]; } + // If the parent object is an unparsed React element tuple and its outlined + // props have now been resolved, we also need to update the props of the + // parsed element object (i.e. handler.value). + if ( + parentObject[0] === REACT_ELEMENT_TYPE && + key === '3' && + typeof handler.value === 'object' && + handler.value !== null && + handler.value.$$typeof === REACT_ELEMENT_TYPE && + handler.value.props === null + ) { + handler.value.props = parentObject[key]; + } + handler.deps--; if (handler.deps === 0) { From 1d38832fc04b3b0bf96bc8ef9f8c6bdddfc2ceb7 Mon Sep 17 00:00:00 2001 From: Hendrik Liebau Date: Tue, 30 Jul 2024 17:36:36 +0200 Subject: [PATCH 3/3] Add another test for resolving of deduped props --- .../__tests__/ReactFlightDOMBrowser-test.js | 55 ++++++++++++++++++- 1 file changed, 53 insertions(+), 2 deletions(-) diff --git a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js index 0e44c574413a5..db8edf7ad6831 100644 --- a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js +++ b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js @@ -525,7 +525,7 @@ describe('ReactFlightDOMBrowser', () => { expect(container.innerHTML).toBe('{"foo":1}{"foo":1}'); }); - it('should handle deduped props of elements in fragments', async () => { + it('should handle deduped props of re-used elements in fragments (same-chunk reference)', async () => { let resolveFooClientComponentChunk; const FooClient = clientExports( @@ -542,7 +542,58 @@ describe('ReactFlightDOMBrowser', () => { function Server() { return ( - {shared} + <>{shared} + + ); + } + + const stream = await serverAct(() => + ReactServerDOMServer.renderToReadableStream(, webpackMap), + ); + + function ClientRoot({response}) { + return use(response); + } + + const response = ReactServerDOMClient.createFromReadableStream(stream); + const container = document.createElement('div'); + const root = ReactDOMClient.createRoot(container); + + await act(() => { + root.render(); + }); + + expect(container.innerHTML).toBe(''); + + await act(() => { + resolveFooClientComponentChunk(); + }); + + expect(container.innerHTML).toBe('
'); + }); + + it('should handle deduped props of re-used elements in server components (cross-chunk reference)', async () => { + let resolveFooClientComponentChunk; + + function PassthroughServerComponent({children}) { + return children; + } + + const FooClient = clientExports( + function Foo({children, item}) { + return children; + }, + '1', + '/foo.js', + new Promise(resolve => (resolveFooClientComponentChunk = resolve)), + ); + + const shared =
; + + function Server() { + return ( + + {shared} ); }