From 59d51000b9e1355f23440c80759d69d72b70966c Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 9 Jan 2024 12:13:52 -0500 Subject: [PATCH 1/7] Add flag --- packages/shared/ReactFeatureFlags.js | 2 ++ packages/shared/forks/ReactFeatureFlags.native-fb.js | 2 ++ packages/shared/forks/ReactFeatureFlags.native-oss.js | 2 ++ packages/shared/forks/ReactFeatureFlags.test-renderer.js | 2 ++ packages/shared/forks/ReactFeatureFlags.test-renderer.native.js | 2 ++ packages/shared/forks/ReactFeatureFlags.test-renderer.www.js | 2 ++ packages/shared/forks/ReactFeatureFlags.www.js | 2 ++ 7 files changed, 14 insertions(+) diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 8841a7bc6a4b5..5e74b30ef809a 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -15,6 +15,8 @@ export const enableComponentStackLocations = true; +export const enableServerComponentKeys = __EXPERIMENTAL__; + // ----------------------------------------------------------------------------- // Killswitch // diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index eaf68191aa6f5..d01f2197c75b8 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -96,5 +96,7 @@ export const enableFizzExternalRuntime = false; export const enableAsyncActions = false; export const enableUseDeferredValueInitialArg = true; +export const enableServerComponentKeys = true; + // Flow magic to verify the exports of this file match the original version. ((((null: any): ExportsType): FeatureFlagsType): ExportsType); diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index d3ef3621d1dbb..4983bdded5d38 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -87,5 +87,7 @@ export const useMicrotasksForSchedulingInFabric = false; export const passChildrenWhenCloningPersistedNodes = false; export const enableUseDeferredValueInitialArg = __EXPERIMENTAL__; +export const enableServerComponentKeys = true; + // Flow magic to verify the exports of this file match the original version. ((((null: any): ExportsType): FeatureFlagsType): ExportsType); diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index 20fa7bb3ff1d1..8da255a2fd204 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -87,5 +87,7 @@ export const useMicrotasksForSchedulingInFabric = false; export const passChildrenWhenCloningPersistedNodes = false; export const enableUseDeferredValueInitialArg = __EXPERIMENTAL__; +export const enableServerComponentKeys = true; + // Flow magic to verify the exports of this file match the original version. ((((null: any): ExportsType): FeatureFlagsType): ExportsType); diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js index af184d191bd04..d4305560235ac 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js @@ -84,5 +84,7 @@ export const useMicrotasksForSchedulingInFabric = false; export const passChildrenWhenCloningPersistedNodes = false; export const enableUseDeferredValueInitialArg = __EXPERIMENTAL__; +export const enableServerComponentKeys = true; + // Flow magic to verify the exports of this file match the original version. ((((null: any): ExportsType): FeatureFlagsType): ExportsType); diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index 8a31dbca6bcf7..f98285e867dcc 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -87,5 +87,7 @@ export const useMicrotasksForSchedulingInFabric = false; export const passChildrenWhenCloningPersistedNodes = false; export const enableUseDeferredValueInitialArg = true; +export const enableServerComponentKeys = true; + // Flow magic to verify the exports of this file match the original version. ((((null: any): ExportsType): FeatureFlagsType): ExportsType); diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index 4850d8236c661..10e24a6bd996c 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -118,5 +118,7 @@ export const passChildrenWhenCloningPersistedNodes = false; export const enableAsyncDebugInfo = false; +export const enableServerComponentKeys = true; + // Flow magic to verify the exports of this file match the original version. ((((null: any): ExportsType): FeatureFlagsType): ExportsType); From 46991aaebe8735d7566351a53a1ea23d654bb478 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 9 Jan 2024 22:56:52 -0500 Subject: [PATCH 2/7] Test key and identity semantics --- .../src/__tests__/ReactFlight-test.js | 415 ++++++++++++++++++ 1 file changed, 415 insertions(+) diff --git a/packages/react-client/src/__tests__/ReactFlight-test.js b/packages/react-client/src/__tests__/ReactFlight-test.js index fc351132a3135..092aa70990557 100644 --- a/packages/react-client/src/__tests__/ReactFlight-test.js +++ b/packages/react-client/src/__tests__/ReactFlight-test.js @@ -1700,4 +1700,419 @@ describe('ReactFlight', () => { expect(errors).toEqual([]); }); + + // @gate enableServerComponentKeys + it('preserves state when keying a server component', async () => { + function StatefulClient({name}) { + const [state] = React.useState(name.toLowerCase()); + return state; + } + const Stateful = clientReference(StatefulClient); + + function Item({item}) { + return ( +
+ {item} + +
+ ); + } + + function Items({items}) { + return items.map(item => { + return ; + }); + } + + const transport = ReactNoopFlightServer.render( + , + ); + + await act(async () => { + ReactNoop.render(await ReactNoopFlightClient.read(transport)); + }); + + expect(ReactNoop).toMatchRenderedOutput( + <> +
Aa
+
Bb
+
Cc
+ , + ); + + const transport2 = ReactNoopFlightServer.render( + , + ); + + await act(async () => { + ReactNoop.render(await ReactNoopFlightClient.read(transport2)); + }); + + expect(ReactNoop).toMatchRenderedOutput( + <> +
Bb
+
Aa
+
Dd
+
Cc
+ , + ); + }); + + // @gate enableServerComponentKeys + it('does not inherit keys of children inside a server component', async () => { + function StatefulClient({name, initial}) { + const [state] = React.useState(initial); + return state; + } + const Stateful = clientReference(StatefulClient); + + function Item({item, initial}) { + // This key is the key of the single item of this component. + // It's NOT part of the key of the list the parent component is + // in. + return ( +
+ {item} + +
+ ); + } + + function IndirectItem({item, initial}) { + // Even though we render two items with the same child key this key + // should not conflict, because the key belongs to the parent slot. + return ; + } + + // These items don't have their own keys because they're in a fixed set + const transport = ReactNoopFlightServer.render( + <> + + + + + , + ); + + await act(async () => { + ReactNoop.render(await ReactNoopFlightClient.read(transport)); + }); + + expect(ReactNoop).toMatchRenderedOutput( + <> +
A1
+
B2
+
C5
+
C6
+ , + ); + + // This means that they shouldn't swap state when the properties update + const transport2 = ReactNoopFlightServer.render( + <> + + + + + , + ); + + await act(async () => { + ReactNoop.render(await ReactNoopFlightClient.read(transport2)); + }); + + expect(ReactNoop).toMatchRenderedOutput( + <> +
B3
+
A4
+
C5
+
C6
+ , + ); + }); + + // @gate enableServerComponentKeys + it('shares state between single return and array return in a parent', async () => { + function StatefulClient({name, initial}) { + const [state] = React.useState(initial); + return state; + } + const Stateful = clientReference(StatefulClient); + + function Item({item, initial}) { + // This key is the key of the single item of this component. + // It's NOT part of the key of the list the parent component is + // in. + return ( + + {item} + + + ); + } + + function Condition({condition}) { + if (condition) { + return ; + } + // The first item in the fragment is the same as the single item. + return ( + <> + + + + ); + } + + function ConditionPlain({condition}) { + if (condition) { + return ( + + C + + + ); + } + // The first item in the fragment is the same as the single item. + return ( + <> + + C + + + + D + + + + ); + } + + const transport = ReactNoopFlightServer.render( + // This two item wrapper ensures we're already one step inside an array. + // A single item is not the same as a set when it's nested one level. + <> +
+ +
+
+ +
+
+ +
+ , + ); + + await act(async () => { + ReactNoop.render(await ReactNoopFlightClient.read(transport)); + }); + + expect(ReactNoop).toMatchRenderedOutput( + <> +
+ A1 +
+
+ C1 +
+
+ C1 +
+ , + ); + + const transport2 = ReactNoopFlightServer.render( + <> +
+ +
+
+ +
+ {null} +
+ +
+ , + ); + + await act(async () => { + ReactNoop.render(await ReactNoopFlightClient.read(transport2)); + }); + + expect(ReactNoop).toMatchRenderedOutput( + <> +
+ A1 + B3 +
+
+ C1 + D3 +
+
+ C1 + D3 +
+ , + ); + }); + + it('shares state between single return and array return in a set', async () => { + function StatefulClient({name, initial}) { + const [state] = React.useState(initial); + return state; + } + const Stateful = clientReference(StatefulClient); + + function Item({item, initial}) { + // This key is the key of the single item of this component. + // It's NOT part of the key of the list the parent component is + // in. + return ( + + {item} + + + ); + } + + function Condition({condition}) { + if (condition) { + return ; + } + // The first item in the fragment is the same as the single item. + return ( + <> + + + + ); + } + + function ConditionPlain({condition}) { + if (condition) { + return ( + + C + + + ); + } + // The first item in the fragment is the same as the single item. + return ( + <> + + C + + + + D + + + + ); + } + + const transport = ReactNoopFlightServer.render( + // This two item wrapper ensures we're already one step inside an array. + // A single item is not the same as a set when it's nested one level. +
+ + + +
, + ); + + await act(async () => { + ReactNoop.render(await ReactNoopFlightClient.read(transport)); + }); + + expect(ReactNoop).toMatchRenderedOutput( +
+ A1 + C1 + C1 +
, + ); + + const transport2 = ReactNoopFlightServer.render( +
+ + + {null} + +
, + ); + + await act(async () => { + ReactNoop.render(await ReactNoopFlightClient.read(transport2)); + }); + + expect(ReactNoop).toMatchRenderedOutput( +
+ A1 + B3 + C1 + D3 + C1 + D3 +
, + ); + }); + + // @gate enableServerComponentKeys + it('preserves state with keys split across async work', async () => { + let resolve; + const promise = new Promise(r => (resolve = r)); + + function StatefulClient({name}) { + const [state] = React.useState(name.toLowerCase()); + return state; + } + const Stateful = clientReference(StatefulClient); + + function Item({name}) { + if (name === 'A') { + return promise.then(() => ( +
+ {name} + +
+ )); + } + return ( +
+ {name} + +
+ ); + } + + const transport = ReactNoopFlightServer.render([ + , + null, + ]); + + // Create a gap in the stream + await resolve(); + + await act(async () => { + ReactNoop.render(await ReactNoopFlightClient.read(transport)); + }); + + expect(ReactNoop).toMatchRenderedOutput(
Aa
); + + const transport2 = ReactNoopFlightServer.render([ + null, + , + ]); + + await act(async () => { + ReactNoop.render(await ReactNoopFlightClient.read(transport2)); + }); + + expect(ReactNoop).toMatchRenderedOutput(
Ba
); + }); }); From 3f90382542646d12de3ff18b173946f3b367f52d Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Fri, 26 Jan 2024 23:38:06 -0500 Subject: [PATCH 3/7] Encode the broken semantic we're ok with --- .../src/__tests__/ReactFlight-test.js | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/packages/react-client/src/__tests__/ReactFlight-test.js b/packages/react-client/src/__tests__/ReactFlight-test.js index 092aa70990557..8e038bfc0f805 100644 --- a/packages/react-client/src/__tests__/ReactFlight-test.js +++ b/packages/react-client/src/__tests__/ReactFlight-test.js @@ -1941,10 +1941,15 @@ describe('ReactFlight', () => { ReactNoop.render(await ReactNoopFlightClient.read(transport2)); }); + // We're intentionally breaking from the semantics here for efficiency of the protocol. + // In the case a Server Component inside a fragment is itself implicitly keyed but its + // return value has a key, then we need a wrapper fragment. This means they can't + // reconcile. To solve this we would need to add a wrapper fragment to every Server + // Component just in case it returns a fragment later which is a lot. expect(ReactNoop).toMatchRenderedOutput( <>
- A1 + A2{/* This should be A1 ideally */} B3
@@ -2050,13 +2055,18 @@ describe('ReactFlight', () => { ReactNoop.render(await ReactNoopFlightClient.read(transport2)); }); + // We're intentionally breaking from the semantics here for efficiency of the protocol. + // The issue with this test scenario is that when the Server Component is in a set, + // the next slot can't be conditionally a fragment or single. That would require wrapping + // in an additional fragment for every single child just in case it every expands to a + // fragment. expect(ReactNoop).toMatchRenderedOutput(
- A1 + A2{/* Should be A1 */} B3 - C1 + C2{/* Should be C1 */} D3 - C1 + C2{/* Should be C1 */} D3
, ); From b729be032a21a3d7d82736ab9b1fd0dc8d2e8d9d Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Fri, 26 Jan 2024 17:04:56 -0500 Subject: [PATCH 4/7] Pass keyPath and implicitSlot as two new contextual values This keeps track of the keys from parent Server Components that are inherited down. --- .../react-server/src/ReactFlightServer.js | 40 ++++++++++++++++--- 1 file changed, 34 insertions(+), 6 deletions(-) diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index 683cbb9feb6f7..0d1e389fa81c9 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -181,6 +181,8 @@ type Task = { model: ReactClientValue, ping: () => void, toJSON: (key: string, value: ReactClientValue) => ReactJSONValue, + keyPath: null | string, // parent server component keys + implicitSlot: boolean, // true if the root server component of this sequence had a null key context: ContextSnapshot, thenableState: ThenableState | null, }; @@ -314,7 +316,14 @@ export function createRequest( }; request.pendingChunks++; const rootContext = createRootContext(context); - const rootTask = createTask(request, model, rootContext, abortSet); + const rootTask = createTask( + request, + model, + null, + false, + rootContext, + abortSet, + ); pingedTasks.push(rootTask); return request; } @@ -338,12 +347,18 @@ function createRootContext( const POP = {}; -function serializeThenable(request: Request, thenable: Thenable): number { +function serializeThenable( + request: Request, + task: Task, + thenable: Thenable, +): number { request.pendingChunks++; const newTask = createTask( request, null, - getActiveContext(), + task.keyPath, // the server component sequence continues through Promise-as-a-child. + task.implicitSlot, + task.context, request.abortableTasks, ); @@ -647,6 +662,8 @@ function pingTask(request: Request, task: Task): void { function createTask( request: Request, model: ReactClientValue, + keyPath: null | string, + implicitSlot: boolean, context: ContextSnapshot, abortSet: Set, ): Task { @@ -659,6 +676,8 @@ function createTask( id, status: PENDING, model, + keyPath, + implicitSlot, context, ping: () => pingTask(request, task), toJSON: function ( @@ -855,7 +874,9 @@ function outlineModel(request: Request, value: ReactClientValue): number { const newTask = createTask( request, value, - getActiveContext(), + null, // The way we use outlining is for reusing an object. + false, // It makes no sense for that use case to be contextual. + rootContextSnapshot, // Therefore we don't pass any contextual information along. request.abortableTasks, ); retryTask(request, newTask); @@ -1016,7 +1037,9 @@ function renderModel( const newTask = createTask( request, task.model, - getActiveContext(), + task.keyPath, + task.implicitSlot, + task.context, request.abortableTasks, ); const ping = newTask.ping; @@ -1166,7 +1189,7 @@ function renderModelDestructive( } // We assume that any object with a .then property is a "Thenable" type, // or a Promise type. Either of which can be represented by a Promise. - const promiseId = serializeThenable(request, (value: any)); + const promiseId = serializeThenable(request, task, (value: any)); writtenObjects.set(value, promiseId); return serializePromiseID(promiseId); } @@ -1582,6 +1605,7 @@ function retryTask(request: Request, task: Task): void { return; } + const prevContext = getActiveContext(); switchContext(task.context); try { // Track the root so we know that we have to emit this object even though it @@ -1646,6 +1670,10 @@ function retryTask(request: Request, task: Task): void { task.status = ERRORED; const digest = logRecoverableError(request, x); emitErrorChunk(request, task.id, digest, x); + } finally { + if (enableServerContext) { + switchContext(prevContext); + } } } From 8e025ecfe2eae10709cb12955a3d1598c166ec3f Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Fri, 26 Jan 2024 21:12:22 -0500 Subject: [PATCH 5/7] Pass the keyPath context when rendering a Server Component This is then prepended to the key of the terminal client element. --- .../react-server/src/ReactFlightServer.js | 130 ++++++++++++++++-- 1 file changed, 115 insertions(+), 15 deletions(-) diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index 0d1e389fa81c9..504312a6aa423 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -16,6 +16,7 @@ import { enablePostpone, enableTaint, enableServerContext, + enableServerComponentKeys, } from 'shared/ReactFeatureFlags'; import { @@ -515,11 +516,44 @@ function createLazyWrapperAroundWakeable(wakeable: Wakeable) { return lazyType; } +function renderClientElement( + task: Task, + type: any, + key: null | string, + props: any, +): ReactJSONValue { + if (!enableServerComponentKeys) { + return [REACT_ELEMENT_TYPE, type, key, props]; + } + // We prepend the terminal client element that actually gets serialized with + // the keys of any Server Components which are not serialized. + const keyPath = task.keyPath; + if (key === null) { + key = keyPath; + } else if (keyPath !== null) { + key = keyPath + ',' + key; + } + const element = [REACT_ELEMENT_TYPE, type, key, props]; + if (task.implicitSlot && key !== null) { + // The root Server Component had no key so it was in an implicit slot. + // If we had a key lower, it would end up in that slot with an explicit key. + // We wrap the element in a fragment to give it an implicit key slot with + // an inner explicit key. + return [element]; + } + // Since we're yielding here, that implicitly resets the keyPath context on the + // way up. Which is what we want since we've consumed it. If this changes to + // be recursive serialization, we need to reset the keyPath and implicitSlot, + // before recursing here. We also need to reset it once we render into an array + // or anything else too which we also get implicitly. + return element; +} + function renderElement( request: Request, task: Task, type: any, - key: null | React$Key, + key: null | string, ref: mixed, props: any, ): ReactJSONValue { @@ -540,7 +574,7 @@ function renderElement( if (typeof type === 'function') { if (isClientReference(type)) { // This is a reference to a Client Component. - return [REACT_ELEMENT_TYPE, type, key, props]; + return renderClientElement(task, type, key, props); } // This is a server-side component. @@ -567,31 +601,51 @@ function renderElement( // the thenable here. result = createLazyWrapperAroundWakeable(result); } - return renderModelDestructive(request, task, emptyRoot, '', result); + // Track this element's key on the Server Component on the keyPath context.. + const prevKeyPath = task.keyPath; + const prevImplicitSlot = task.implicitSlot; + if (key !== null) { + // Append the key to the path. Technically a null key should really add the child + // index. We don't do that to hold the payload small and implementation simple. + task.keyPath = prevKeyPath === null ? key : prevKeyPath + ',' + key; + } else if (prevKeyPath === null) { + // This sequence of Server Components has no keys. This means that it was rendered + // in a slot that needs to assign an implicit key. Even if children below have + // explicit keys, they should not be used for the outer most key since it might + // collide with other slots in that set. + task.implicitSlot = true; + } + const json = renderModelDestructive(request, task, emptyRoot, '', result); + task.keyPath = prevKeyPath; + task.implicitSlot = prevImplicitSlot; + return json; } else if (typeof type === 'string') { // This is a host element. E.g. HTML. - return [REACT_ELEMENT_TYPE, type, key, props]; + return renderClientElement(task, type, key, props); } else if (typeof type === 'symbol') { - if (type === REACT_FRAGMENT_TYPE) { + if (type === REACT_FRAGMENT_TYPE && key === null) { // For key-less fragments, we add a small optimization to avoid serializing // it as a wrapper. - // TODO: If a key is specified, we should propagate its key to any children. - // Same as if a Server Component has a key. - return renderModelDestructive( + const prevImplicitSlot = task.implicitSlot; + if (task.keyPath === null) { + task.implicitSlot = true; + } + const json = renderModelDestructive( request, task, emptyRoot, '', props.children, ); + task.implicitSlot = prevImplicitSlot; } // This might be a built-in React component. We'll let the client decide. // Any built-in works as long as its props are serializable. - return [REACT_ELEMENT_TYPE, type, key, props]; + return renderClientElement(task, type, key, props); } else if (type != null && typeof type === 'object') { if (isClientReference(type)) { // This is a reference to a Client Component. - return [REACT_ELEMENT_TYPE, type, key, props]; + return renderClientElement(task, type, key, props); } switch (type.$$typeof) { case REACT_LAZY_TYPE: { @@ -611,7 +665,29 @@ function renderElement( prepareToUseHooksForComponent(prevThenableState); const result = render(props, undefined); - return renderModelDestructive(request, task, emptyRoot, '', result); + const prevKeyPath = task.keyPath; + const prevImplicitSlot = task.implicitSlot; + if (key !== null) { + // Append the key to the path. Technically a null key should really add the child + // index. We don't do that to hold the payload small and implementation simple. + task.keyPath = prevKeyPath === null ? key : prevKeyPath + ',' + key; + } else if (prevKeyPath === null) { + // This sequence of Server Components has no keys. This means that it was rendered + // in a slot that needs to assign an implicit key. Even if children below have + // explicit keys, they should not be used for the outer most key since it might + // collide with other slots in that set. + task.implicitSlot = true; + } + const json = renderModelDestructive( + request, + task, + emptyRoot, + '', + result, + ); + task.keyPath = prevKeyPath; + task.implicitSlot = prevImplicitSlot; + return json; } case REACT_MEMO_TYPE: { return renderElement(request, task, type.type, key, ref, props); @@ -633,13 +709,13 @@ function renderElement( ); } } - return [ - REACT_ELEMENT_TYPE, + return renderClientElement( + task, type, key, // Rely on __popProvider being serialized last to pop the provider. {value: props.value, children: props.children, __pop: POP}, - ]; + ); } // Fallthrough } @@ -1009,6 +1085,8 @@ function renderModel( key: string, value: ReactClientValue, ): ReactJSONValue { + const prevKeyPath = task.keyPath; + const prevImplicitSlot = task.implicitSlot; try { return renderModelDestructive(request, task, parent, key, value); } catch (thrownValue) { @@ -1045,6 +1123,12 @@ function renderModel( const ping = newTask.ping; (x: any).then(ping, ping); newTask.thenableState = getThenableStateAfterSuspending(); + + // Restore the context. We assume that this will be restored by the inner + // functions in case nothing throws so we don't use "finally" here. + task.keyPath = prevKeyPath; + task.implicitSlot = prevImplicitSlot; + if (wasReactNode) { return serializeLazyID(newTask.id); } @@ -1057,12 +1141,24 @@ function renderModel( const postponeId = request.nextChunkId++; logPostpone(request, postponeInstance.message); emitPostponeChunk(request, postponeId, postponeInstance); + + // Restore the context. We assume that this will be restored by the inner + // functions in case nothing throws so we don't use "finally" here. + task.keyPath = prevKeyPath; + task.implicitSlot = prevImplicitSlot; + if (wasReactNode) { return serializeLazyID(postponeId); } return serializeByValueID(postponeId); } } + + // Restore the context. We assume that this will be restored by the inner + // functions in case nothing throws so we don't use "finally" here. + task.keyPath = prevKeyPath; + task.implicitSlot = prevImplicitSlot; + if (wasReactNode) { // Something errored. We'll still send everything we have up until this point. // We'll replace this element with a lazy reference that throws on the client @@ -1131,13 +1227,13 @@ function renderModelDestructive( writtenObjects.set(value, -1); } - // TODO: Concatenate keys of parents onto children. const element: React$Element = (value: any); // Attempt to render the Server Component. return renderElement( request, task, element.type, + // $FlowFixMe[incompatible-call] the key of an element is null | string element.key, element.ref, element.props, @@ -1626,6 +1722,10 @@ function retryTask(request: Request, task: Task): void { // Track the root again for the resolved object. modelRoot = resolvedModel; + // The keyPath resets at any terminal child node. + task.keyPath = null; + task.implicitSlot = false; + // If the value is a string, it means it's a terminal value adn we already escaped it // We don't need to escape it again so it's not passed the toJSON replacer. // Object might contain unresolved values like additional elements. From c49a32f5b013cc0f559ec3a8fae1ffcf7df5a935 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Fri, 26 Jan 2024 23:19:39 -0500 Subject: [PATCH 6/7] Unkeyed fragments needs a wrapper if the parent server component was keyed --- .../react-server/src/ReactFlightServer.js | 48 +++++++++++++++++-- 1 file changed, 45 insertions(+), 3 deletions(-) diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index 504312a6aa423..5263d01611c52 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -516,6 +516,48 @@ function createLazyWrapperAroundWakeable(wakeable: Wakeable) { return lazyType; } +function renderFragment( + request: Request, + task: Task, + children: $ReadOnlyArray, +): ReactJSONValue { + if (!enableServerComponentKeys) { + return children; + } + if (task.keyPath !== null) { + // We have a Server Component that specifies a key but we're now splitting + // the tree using a fragment. + const fragment = [ + REACT_ELEMENT_TYPE, + REACT_FRAGMENT_TYPE, + task.keyPath, + {children}, + ]; + if (!task.implicitSlot) { + // If this was keyed inside a set. I.e. the outer Server Component was keyed + // then we need to handle reorders of the whole set. To do this we need to wrap + // this array in a keyed Fragment. + return fragment; + } + // If the outer Server Component was implicit but then an inner one had a key + // we don't actually need to be able to move the whole set around. It'll always be + // in an implicit slot. The key only exists to be able to reset the state of the + // children. We could achieve the same effect by passing on the keyPath to the next + // set of components inside the fragment. This would also allow a keyless fragment + // reconcile against a single child. + // Unfortunately because of JSON.stringify, we can't call the recursive loop for + // each child within this context because we can't return a set with already resolved + // values. E.g. a string would get double encoded. Returning would pop the context. + // So instead, we wrap it with an unkeyed fragment and inner keyed fragment. + return [fragment]; + } + // Since we're yielding here, that implicitly resets the keyPath context on the + // way up. Which is what we want since we've consumed it. If this changes to + // be recursive serialization, we need to reset the keyPath and implicitSlot, + // before recursing here. + return children; +} + function renderClientElement( task: Task, type: any, @@ -638,6 +680,7 @@ function renderElement( props.children, ); task.implicitSlot = prevImplicitSlot; + return json; } // This might be a built-in React component. We'll let the client decide. // Any built-in works as long as its props are serializable. @@ -1334,8 +1377,7 @@ function renderModelDestructive( } if (isArray(value)) { - // $FlowFixMe[incompatible-return] - return value; + return renderFragment(request, task, value); } if (value instanceof Map) { @@ -1401,7 +1443,7 @@ function renderModelDestructive( const iteratorFn = getIteratorFn(value); if (iteratorFn) { - return Array.from((value: any)); + return renderFragment(request, task, Array.from((value: any))); } // Verify that this is a simple plain object. From 893316de12954e5d8e2f36a680450d9c0795ff5c Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Thu, 25 Jan 2024 15:24:04 -0500 Subject: [PATCH 7/7] Don't dedupe Elements if they're in a non-default Context If an element gets wrapped in a different server component then that has a different keyPath context and the element might end up with a different key. So we don't use the deduping mechanism if we're already inside a Server Component parent with a key or otherwise. Only the simple case gets deduped. The props of a client element are still deduped though if they're the same instance. --- .../src/__tests__/ReactFlightDOMEdge-test.js | 51 +++++++++-- .../react-server/src/ReactFlightServer.js | 84 +++++++++++++------ 2 files changed, 103 insertions(+), 32 deletions(-) diff --git a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js index 5d966a16ded59..7c9271fcdcc19 100644 --- a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js +++ b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js @@ -226,20 +226,55 @@ describe('ReactFlightDOMEdge', () => { const [stream1, stream2] = passThrough(stream).tee(); const serializedContent = await readResult(stream1); + expect(serializedContent.length).toBeLessThan(400); expect(timesRendered).toBeLessThan(5); - const result = await ReactServerDOMClient.createFromReadableStream( - stream2, - { - ssrManifest: { - moduleMap: null, - moduleLoading: null, - }, + const model = await ReactServerDOMClient.createFromReadableStream(stream2, { + ssrManifest: { + moduleMap: null, + moduleLoading: null, }, + }); + + // Use the SSR render to resolve any lazy elements + const ssrStream = await ReactDOMServer.renderToReadableStream(model); + // Should still match the result when parsed + const result = await readResult(ssrStream); + expect(result).toEqual(resolvedChildren.join('')); + }); + + it('should execute repeated host components only once', async () => { + const div =
this is a long return value
; + let timesRendered = 0; + function ServerComponent() { + timesRendered++; + return div; + } + const element = ; + const children = new Array(30).fill(element); + const resolvedChildren = new Array(30).fill( + '
this is a long return value
', ); + const stream = ReactServerDOMServer.renderToReadableStream(children); + const [stream1, stream2] = passThrough(stream).tee(); + + const serializedContent = await readResult(stream1); + expect(serializedContent.length).toBeLessThan(400); + expect(timesRendered).toBeLessThan(5); + + const model = await ReactServerDOMClient.createFromReadableStream(stream2, { + ssrManifest: { + moduleMap: null, + moduleLoading: null, + }, + }); + + // Use the SSR render to resolve any lazy elements + const ssrStream = await ReactDOMServer.renderToReadableStream(model); // Should still match the result when parsed - expect(result).toEqual(resolvedChildren); + const result = await readResult(ssrStream); + expect(result).toEqual(resolvedChildren.join('')); }); it('should execute repeated server components in a compact form', async () => { diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index 5263d01611c52..f04642737e167 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -788,8 +788,17 @@ function createTask( ): Task { const id = request.nextChunkId++; if (typeof model === 'object' && model !== null) { - // Register this model as having the ID we're about to write. - request.writtenObjects.set(model, id); + // If we're about to write this into a new task we can assign it an ID early so that + // any other references can refer to the value we're about to write. + if ( + enableServerComponentKeys && + (keyPath !== null || implicitSlot || context !== rootContextSnapshot) + ) { + // If we're in some kind of context we can't necessarily reuse this object depending + // what parent components are used. + } else { + request.writtenObjects.set(model, id); + } } const task: Task = { id, @@ -1251,18 +1260,31 @@ function renderModelDestructive( const writtenObjects = request.writtenObjects; const existingId = writtenObjects.get(value); if (existingId !== undefined) { - if (existingId === -1) { - // Seen but not yet outlined. - const newId = outlineModel(request, value); - return serializeByValueID(newId); + if ( + enableServerComponentKeys && + (task.keyPath !== null || + task.implicitSlot || + task.context !== rootContextSnapshot) + ) { + // If we're in some kind of context we can't reuse the result of this render or + // previous renders of this element. We only reuse elements if they're not wrapped + // by another Server Component. } else if (modelRoot === value) { // This is the ID we're currently emitting so we need to write it // once but if we discover it again, we refer to it by id. modelRoot = null; + } else if (existingId === -1) { + // Seen but not yet outlined. + // TODO: If we throw here we can treat this as suspending which causes an outline + // but that is able to reuse the same task if we're already in one but then that + // will be a lazy future value rather than guaranteed to exist but maybe that's good. + const newId = outlineModel(request, (value: any)); + return serializeLazyID(newId); } else { - // We've already emitted this as an outlined object, so we can - // just refer to that by its existing ID. - return serializeByValueID(existingId); + // We've already emitted this as an outlined object, so we can refer to that by its + // existing ID. We use a lazy reference since, unlike plain objects, elements might + // suspend so it might not have emitted yet even if we have the ID for it. + return serializeLazyID(existingId); } } else { // This is the first time we've seen this object. We may never see it again @@ -1317,7 +1339,18 @@ function renderModelDestructive( // $FlowFixMe[method-unbinding] if (typeof value.then === 'function') { if (existingId !== undefined) { - if (modelRoot === value) { + if ( + enableServerComponentKeys && + (task.keyPath !== null || + task.implicitSlot || + task.context !== rootContextSnapshot) + ) { + // If we're in some kind of context we can't reuse the result of this render or + // previous renders of this element. We only reuse Promises if they're not wrapped + // by another Server Component. + const promiseId = serializeThenable(request, task, (value: any)); + return serializePromiseID(promiseId); + } else if (modelRoot === value) { // This is the ID we're currently emitting so we need to write it // once but if we discover it again, we refer to it by id. modelRoot = null; @@ -1357,14 +1390,14 @@ function renderModelDestructive( } if (existingId !== undefined) { - if (existingId === -1) { - // Seen but not yet outlined. - const newId = outlineModel(request, value); - return serializeByValueID(newId); - } else if (modelRoot === value) { + if (modelRoot === value) { // This is the ID we're currently emitting so we need to write it // once but if we discover it again, we refer to it by id. modelRoot = null; + } else if (existingId === -1) { + // Seen but not yet outlined. + const newId = outlineModel(request, (value: any)); + return serializeByValueID(newId); } else { // We've already emitted this as an outlined object, so we can // just refer to that by its existing ID. @@ -1768,15 +1801,18 @@ function retryTask(request: Request, task: Task): void { task.keyPath = null; task.implicitSlot = false; - // If the value is a string, it means it's a terminal value adn we already escaped it - // We don't need to escape it again so it's not passed the toJSON replacer. - // Object might contain unresolved values like additional elements. - // This is simulating what the JSON loop would do if this was part of it. - // $FlowFixMe[incompatible-type] stringify can return null - const json: string = - typeof resolvedModel === 'string' - ? stringify(resolvedModel) - : stringify(resolvedModel, task.toJSON); + let json: string; + if (typeof resolvedModel === 'object' && resolvedModel !== null) { + // Object might contain unresolved values like additional elements. + // This is simulating what the JSON loop would do if this was part of it. + // $FlowFixMe[incompatible-type] stringify can return null for undefined but we never do + json = stringify(resolvedModel, task.toJSON); + } else { + // If the value is a string, it means it's a terminal value and we already escaped it + // We don't need to escape it again so it's not passed the toJSON replacer. + // $FlowFixMe[incompatible-type] stringify can return null for undefined but we never do + json = stringify(resolvedModel); + } emitModelChunk(request, task.id, json); request.abortableTasks.delete(task);