From 1b7aaab6f6c3225fa22959da4dbf2b5e45cd1de9 Mon Sep 17 00:00:00 2001 From: Josh Story Date: Sun, 27 Mar 2022 00:17:30 -0700 Subject: [PATCH 1/6] Implements useId hook for Flight server. The approach for ids for Flight is different from Fizz/Client where there is a need for determinancy. Flight rendered elements will not be rendered on the client and as such the ids generated in a request only need to be unique. However since FLight does support refetching subtrees it is possible a client will need to patch up a part of the tree rather than replacing the entire thing so it is not safe to use a simple incrementing counter. To solve for this we allow the caller to specify a prefix. On an initial fetch it is likely this will be empty but on refetches or subtrees we expect to have a client `useId` provide the prefix since it will guaranteed be unique for that subtree and thus for the entire tree. It is also possible that we will automatically provide prefixes based on a client/Fizz useId on refetches in addition to the core change I also modified the structure of options for renderToReadableStream where `onError`, `context`, and the new `identifierPrefix` are properties of an Options object argument to avoid the clumsiness of a growing list of optional function arguments. --- .../src/__tests__/ReactFlight-test.js | 60 +++++++++++++++++-- .../src/ReactNoopFlightServer.js | 11 ++-- .../src/ReactFlightDOMRelayServer.js | 3 + .../src/ReactFlightDOMServerBrowser.js | 6 +- .../src/ReactFlightDOMServerNode.js | 5 +- packages/react-server/src/ReactFlightHooks.js | 28 ++++++++- .../react-server/src/ReactFlightServer.js | 19 ++++-- 7 files changed, 113 insertions(+), 19 deletions(-) diff --git a/packages/react-client/src/__tests__/ReactFlight-test.js b/packages/react-client/src/__tests__/ReactFlight-test.js index 6213f0b72e08..a04198a28445 100644 --- a/packages/react-client/src/__tests__/ReactFlight-test.js +++ b/packages/react-client/src/__tests__/ReactFlight-test.js @@ -512,6 +512,59 @@ describe('ReactFlight', () => { ); }); + describe('Hooks', () => { + function DivWithId() { + const id = React.useId(); + return
; + } + + it('should support useId', () => { + function App() { + return ( + <> + + + + ); + } + + const transport = ReactNoopFlightServer.render(); + act(() => { + ReactNoop.render(ReactNoopFlightClient.read(transport)); + }); + expect(ReactNoop).toMatchRenderedOutput( + <> +
+
+ , + ); + }); + + it('accepts an identifier prefix that prefixes generated ids', () => { + function App() { + return ( + <> + + + + ); + } + + const transport = ReactNoopFlightServer.render(, { + identifierPrefix: 'foo', + }); + act(() => { + ReactNoop.render(ReactNoopFlightClient.read(transport)); + }); + expect(ReactNoop).toMatchRenderedOutput( + <> +
+
+ , + ); + }); + }); + describe('ServerContext', () => { // @gate enableServerContext it('supports basic createServerContext usage', () => { @@ -759,15 +812,14 @@ describe('ReactFlight', () => { function Bar() { return {React.useContext(ServerContext)}; } - const transport = ReactNoopFlightServer.render(, {}, [ - ['ServerContext', 'Override'], - ]); + const transport = ReactNoopFlightServer.render(, { + context: [['ServerContext', 'Override']], + }); act(() => { const flightModel = ReactNoopFlightClient.read(transport); ReactNoop.render(flightModel); }); - expect(ReactNoop).toMatchRenderedOutput(Override); }); diff --git a/packages/react-noop-renderer/src/ReactNoopFlightServer.js b/packages/react-noop-renderer/src/ReactNoopFlightServer.js index 1c607befe74b..3a84a06d0843 100644 --- a/packages/react-noop-renderer/src/ReactNoopFlightServer.js +++ b/packages/react-noop-renderer/src/ReactNoopFlightServer.js @@ -61,20 +61,19 @@ const ReactNoopFlightServer = ReactFlightServer({ type Options = { onError?: (error: mixed) => void, + context?: Array<[string, ServerContextJSONValue]>, + identifierPrefix?: string, }; -function render( - model: ReactModel, - options?: Options, - context?: Array<[string, ServerContextJSONValue]>, -): Destination { +function render(model: ReactModel, options?: Options): Destination { const destination: Destination = []; const bundlerConfig = undefined; const request = ReactNoopFlightServer.createRequest( model, bundlerConfig, options ? options.onError : undefined, - context, + options ? options.context : undefined, + options ? options.identifierPrefix : undefined, ); ReactNoopFlightServer.startWork(request); ReactNoopFlightServer.startFlowing(request, destination); diff --git a/packages/react-server-dom-relay/src/ReactFlightDOMRelayServer.js b/packages/react-server-dom-relay/src/ReactFlightDOMRelayServer.js index 7feb6e3f0eff..5e65921193ba 100644 --- a/packages/react-server-dom-relay/src/ReactFlightDOMRelayServer.js +++ b/packages/react-server-dom-relay/src/ReactFlightDOMRelayServer.js @@ -21,6 +21,7 @@ import { type Options = { onError?: (error: mixed) => void, + identifierPrefix?: string, }; function render( @@ -33,6 +34,8 @@ function render( model, config, options ? options.onError : undefined, + undefined, // not currently set up to supply context overrides + options ? options.identifierPrefix : undefined, ); startWork(request); startFlowing(request, destination); diff --git a/packages/react-server-dom-webpack/src/ReactFlightDOMServerBrowser.js b/packages/react-server-dom-webpack/src/ReactFlightDOMServerBrowser.js index a00eb8c44106..bfd47cc40a69 100644 --- a/packages/react-server-dom-webpack/src/ReactFlightDOMServerBrowser.js +++ b/packages/react-server-dom-webpack/src/ReactFlightDOMServerBrowser.js @@ -19,19 +19,21 @@ import { type Options = { onError?: (error: mixed) => void, + context?: Array<[string, ServerContextJSONValue]>, + identifierPrefix?: string, }; function renderToReadableStream( model: ReactModel, webpackMap: BundlerConfig, options?: Options, - context?: Array<[string, ServerContextJSONValue]>, ): ReadableStream { const request = createRequest( model, webpackMap, options ? options.onError : undefined, - context, + options ? options.context : undefined, + options ? options.identifierPrefix : undefined, ); const stream = new ReadableStream( { diff --git a/packages/react-server-dom-webpack/src/ReactFlightDOMServerNode.js b/packages/react-server-dom-webpack/src/ReactFlightDOMServerNode.js index 76952b6d5425..b89a91a456b8 100644 --- a/packages/react-server-dom-webpack/src/ReactFlightDOMServerNode.js +++ b/packages/react-server-dom-webpack/src/ReactFlightDOMServerNode.js @@ -24,6 +24,8 @@ function createDrainHandler(destination, request) { type Options = { onError?: (error: mixed) => void, + context?: Array<[string, ServerContextJSONValue]>, + identifierPrefix?: string, }; type PipeableStream = {| @@ -40,7 +42,8 @@ function renderToPipeableStream( model, webpackMap, options ? options.onError : undefined, - context, + options ? options.context : undefined, + options ? options.identifierPrefix : undefined, ); let hasStartedFlowing = false; startWork(request); diff --git a/packages/react-server/src/ReactFlightHooks.js b/packages/react-server/src/ReactFlightHooks.js index 88a2eac86ca2..ae7666d19ecf 100644 --- a/packages/react-server/src/ReactFlightHooks.js +++ b/packages/react-server/src/ReactFlightHooks.js @@ -8,10 +8,27 @@ */ import type {Dispatcher as DispatcherType} from 'react-reconciler/src/ReactInternalTypes'; +import type {Request} from './ReactFlightServer'; import type {ReactServerContext} from 'shared/ReactTypes'; import {REACT_SERVER_CONTEXT_TYPE} from 'shared/ReactSymbols'; import {readContext as readContextImpl} from './ReactFlightNewContext'; +let currentIndentifierPrefix = '$'; +let currentIndentifierCount = 0; + +export function prepareToUseHooksForRequest(request: Request) { + if (request.identifierPrefix) { + currentIndentifierPrefix = request.identifierPrefix; + } + currentIndentifierCount = request.identifierCount; +} + +export function resetHooksForRequest(request: Request) { + currentIndentifierPrefix = '$'; + request.identifierCount = currentIndentifierCount; + currentIndentifierCount = 0; +} + function readContext(context: ReactServerContext): T { if (__DEV__) { if (context.$$typeof !== REACT_SERVER_CONTEXT_TYPE) { @@ -61,7 +78,7 @@ export const Dispatcher: DispatcherType = { useLayoutEffect: (unsupportedHook: any), useImperativeHandle: (unsupportedHook: any), useEffect: (unsupportedHook: any), - useId: (unsupportedHook: any), + useId, useMutableSource: (unsupportedHook: any), useSyncExternalStore: (unsupportedHook: any), useCacheRefresh(): (?() => T, ?T) => void { @@ -91,3 +108,12 @@ export function setCurrentCache(cache: Map | null) { export function getCurrentCache() { return currentCache; } + +function useId(): string { + return ( + ':' + + currentIndentifierPrefix + + ':F' + + (currentIndentifierCount++).toString(32) + ); +} diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index e08f308a32fd..700ddaa05bf3 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -39,7 +39,13 @@ import { isModuleReference, } from './ReactFlightServerConfig'; -import {Dispatcher, getCurrentCache, setCurrentCache} from './ReactFlightHooks'; +import { + Dispatcher, + getCurrentCache, + prepareToUseHooksForRequest, + resetHooksForRequest, + setCurrentCache, +} from './ReactFlightHooks'; import { pushProvider, popProvider, @@ -102,14 +108,12 @@ export type Request = { writtenSymbols: Map, writtenModules: Map, writtenProviders: Map, + identifierPrefix?: string, + identifierCount: number, onError: (error: mixed) => void, toJSON: (key: string, value: ReactModel) => ReactJSONValue, }; -export type Options = { - onError?: (error: mixed) => void, -}; - const ReactCurrentDispatcher = ReactSharedInternals.ReactCurrentDispatcher; function defaultErrorHandler(error: mixed) { @@ -126,6 +130,7 @@ export function createRequest( bundlerConfig: BundlerConfig, onError: void | ((error: mixed) => void), context?: Array<[string, ServerContextJSONValue]>, + identifierPrefix?: string, ): Request { const pingedSegments = []; const request = { @@ -143,6 +148,8 @@ export function createRequest( writtenSymbols: new Map(), writtenModules: new Map(), writtenProviders: new Map(), + identifierPrefix, + identifierCount: 1, onError: onError === undefined ? defaultErrorHandler : onError, toJSON: function(key: string, value: ReactModel): ReactJSONValue { return resolveModelToJSON(request, this, key, value); @@ -826,6 +833,7 @@ function performWork(request: Request): void { const prevCache = getCurrentCache(); ReactCurrentDispatcher.current = Dispatcher; setCurrentCache(request.cache); + prepareToUseHooksForRequest(request); try { const pingedSegments = request.pingedSegments; @@ -843,6 +851,7 @@ function performWork(request: Request): void { } finally { ReactCurrentDispatcher.current = prevDispatcher; setCurrentCache(prevCache); + resetHooksForRequest(request); } } From baa298b7efe47a6db05de35288e01228e2226a1a Mon Sep 17 00:00:00 2001 From: Josh Story Date: Sun, 27 Mar 2022 22:18:40 -0700 Subject: [PATCH 2/6] defend against useId call outside of rendering --- .../src/__tests__/ReactFlight-test.js | 8 +++--- packages/react-server/src/ReactFlightHooks.js | 28 ++++++++----------- .../react-server/src/ReactFlightServer.js | 2 +- 3 files changed, 17 insertions(+), 21 deletions(-) diff --git a/packages/react-client/src/__tests__/ReactFlight-test.js b/packages/react-client/src/__tests__/ReactFlight-test.js index a04198a28445..254d5f27eb72 100644 --- a/packages/react-client/src/__tests__/ReactFlight-test.js +++ b/packages/react-client/src/__tests__/ReactFlight-test.js @@ -534,8 +534,8 @@ describe('ReactFlight', () => { }); expect(ReactNoop).toMatchRenderedOutput( <> -
-
+
+
, ); }); @@ -558,8 +558,8 @@ describe('ReactFlight', () => { }); expect(ReactNoop).toMatchRenderedOutput( <> -
-
+
+
, ); }); diff --git a/packages/react-server/src/ReactFlightHooks.js b/packages/react-server/src/ReactFlightHooks.js index ae7666d19ecf..5fbdda8020ee 100644 --- a/packages/react-server/src/ReactFlightHooks.js +++ b/packages/react-server/src/ReactFlightHooks.js @@ -13,20 +13,14 @@ import type {ReactServerContext} from 'shared/ReactTypes'; import {REACT_SERVER_CONTEXT_TYPE} from 'shared/ReactSymbols'; import {readContext as readContextImpl} from './ReactFlightNewContext'; -let currentIndentifierPrefix = '$'; -let currentIndentifierCount = 0; +let currentRequest = null; export function prepareToUseHooksForRequest(request: Request) { - if (request.identifierPrefix) { - currentIndentifierPrefix = request.identifierPrefix; - } - currentIndentifierCount = request.identifierCount; + currentRequest = request; } -export function resetHooksForRequest(request: Request) { - currentIndentifierPrefix = '$'; - request.identifierCount = currentIndentifierCount; - currentIndentifierCount = 0; +export function resetHooksForRequest() { + currentRequest = null; } function readContext(context: ReactServerContext): T { @@ -110,10 +104,12 @@ export function getCurrentCache() { } function useId(): string { - return ( - ':' + - currentIndentifierPrefix + - ':F' + - (currentIndentifierCount++).toString(32) - ); + if (currentRequest === null) { + throw new Error('useId can only be used while React is rendering.'); + } + const prefix = currentRequest.identifierPrefix + ? currentRequest.identifierPrefix + : ''; + const id = currentRequest.identifierCount++; + return ':' + prefix + 'F' + id.toString(32) + ':'; } diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index 700ddaa05bf3..f1253f046da8 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -851,7 +851,7 @@ function performWork(request: Request): void { } finally { ReactCurrentDispatcher.current = prevDispatcher; setCurrentCache(prevCache); - resetHooksForRequest(request); + resetHooksForRequest(); } } From b224f073766aa5deb54d229d21e38e3a8a6258a0 Mon Sep 17 00:00:00 2001 From: Josh Story Date: Sun, 27 Mar 2022 22:56:04 -0700 Subject: [PATCH 3/6] switch to S from F for Server Component ids --- .../react-client/src/__tests__/ReactFlight-test.js | 12 ++++++------ packages/react-server/src/ReactFlightHooks.js | 5 +++-- scripts/error-codes/codes.json | 3 ++- 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/packages/react-client/src/__tests__/ReactFlight-test.js b/packages/react-client/src/__tests__/ReactFlight-test.js index 254d5f27eb72..e5e25d49eef1 100644 --- a/packages/react-client/src/__tests__/ReactFlight-test.js +++ b/packages/react-client/src/__tests__/ReactFlight-test.js @@ -513,9 +513,9 @@ describe('ReactFlight', () => { }); describe('Hooks', () => { - function DivWithId() { + function DivWithId({children}) { const id = React.useId(); - return
; + return
{children}
; } it('should support useId', () => { @@ -534,8 +534,8 @@ describe('ReactFlight', () => { }); expect(ReactNoop).toMatchRenderedOutput( <> -
-
+
+
, ); }); @@ -558,8 +558,8 @@ describe('ReactFlight', () => { }); expect(ReactNoop).toMatchRenderedOutput( <> -
-
+
+
, ); }); diff --git a/packages/react-server/src/ReactFlightHooks.js b/packages/react-server/src/ReactFlightHooks.js index 5fbdda8020ee..ebc71cc82324 100644 --- a/packages/react-server/src/ReactFlightHooks.js +++ b/packages/react-server/src/ReactFlightHooks.js @@ -105,11 +105,12 @@ export function getCurrentCache() { function useId(): string { if (currentRequest === null) { - throw new Error('useId can only be used while React is rendering.'); + throw new Error('useId can only be used while React is rendering'); } const prefix = currentRequest.identifierPrefix ? currentRequest.identifierPrefix : ''; const id = currentRequest.identifierCount++; - return ':' + prefix + 'F' + id.toString(32) + ':'; + // use 'S' for Flight components to distinguish from 'R' and 'r' in Fizz/Client + return ':' + prefix + 'S' + id.toString(32) + ':'; } diff --git a/scripts/error-codes/codes.json b/scripts/error-codes/codes.json index 7a93b9a0ac64..a8c8810bbe49 100644 --- a/scripts/error-codes/codes.json +++ b/scripts/error-codes/codes.json @@ -417,5 +417,6 @@ "429": "ServerContext: %s already defined", "430": "ServerContext can only have a value prop and children. Found: %s", "431": "React elements are not allowed in ServerContext", - "432": "This Suspense boundary was aborted by the server" + "432": "This Suspense boundary was aborted by the server", + "433": "useId can only be used while React is rendering" } From 2c4a074ceda3fcf21a538ae7350af3c065f13ab1 Mon Sep 17 00:00:00 2001 From: Josh Story Date: Tue, 31 May 2022 14:18:28 -0700 Subject: [PATCH 4/6] default to empty string identifier prefix --- packages/react-server/src/ReactFlightHooks.js | 5 +---- packages/react-server/src/ReactFlightServer.js | 4 ++-- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/packages/react-server/src/ReactFlightHooks.js b/packages/react-server/src/ReactFlightHooks.js index ebc71cc82324..5b10d8c7a1f6 100644 --- a/packages/react-server/src/ReactFlightHooks.js +++ b/packages/react-server/src/ReactFlightHooks.js @@ -107,10 +107,7 @@ function useId(): string { if (currentRequest === null) { throw new Error('useId can only be used while React is rendering'); } - const prefix = currentRequest.identifierPrefix - ? currentRequest.identifierPrefix - : ''; const id = currentRequest.identifierCount++; // use 'S' for Flight components to distinguish from 'R' and 'r' in Fizz/Client - return ':' + prefix + 'S' + id.toString(32) + ':'; + return ':' + currentRequest.identifierPrefix + 'S' + id.toString(32) + ':'; } diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index f1253f046da8..d1399ce63ae8 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -108,7 +108,7 @@ export type Request = { writtenSymbols: Map, writtenModules: Map, writtenProviders: Map, - identifierPrefix?: string, + identifierPrefix: string, identifierCount: number, onError: (error: mixed) => void, toJSON: (key: string, value: ReactModel) => ReactJSONValue, @@ -148,7 +148,7 @@ export function createRequest( writtenSymbols: new Map(), writtenModules: new Map(), writtenProviders: new Map(), - identifierPrefix, + identifierPrefix: identifierPrefix || '', identifierCount: 1, onError: onError === undefined ? defaultErrorHandler : onError, toJSON: function(key: string, value: ReactModel): ReactJSONValue { From aa490a3f36ad75e2122911bd94a561560a1d3d6b Mon Sep 17 00:00:00 2001 From: Josh Story Date: Tue, 31 May 2022 14:37:54 -0700 Subject: [PATCH 5/6] Add a test demonstrating that there is no warning when double rendering on the client a server component that used useId --- .../src/__tests__/ReactFlight-test.js | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/packages/react-client/src/__tests__/ReactFlight-test.js b/packages/react-client/src/__tests__/ReactFlight-test.js index e5e25d49eef1..b38db8c58a22 100644 --- a/packages/react-client/src/__tests__/ReactFlight-test.js +++ b/packages/react-client/src/__tests__/ReactFlight-test.js @@ -563,6 +563,47 @@ describe('ReactFlight', () => { , ); }); + + // @gate enableServerContext + it('[TODO] it does not warn if you render a server element passed to a client module reference twice on the client when using useId', async () => { + // @TODO Today if you render a server component with useId and pass it to a client component and that client component renders the element in two or more + // places the id used on the server will be duplicated in the client. This is a deviation from the guarantees useId makes for Fizz/Client and is a consequence + // of the fact that the server component is actually rendered on the server and is reduced to a set of host elements before being passed to the Client component + // so the output passed to the Client has no knowledge of the useId use. In the future we would like to add a DEV warning when this happens. For now + // we just accept that it is a nuance of useId in Flight + function App() { + let id = React.useId(); + let div =
{id}
; + return ; + } + + function ClientDoubler({el}) { + Scheduler.unstable_yieldValue('ClientDoubler'); + return ( + <> + {el} + {el} + + ); + } + + const ClientDoublerModuleRef = moduleReference(ClientDoubler); + + const transport = ReactNoopFlightServer.render(); + expect(Scheduler).toHaveYielded([]); + + act(() => { + ReactNoop.render(ReactNoopFlightClient.read(transport)); + }); + + expect(Scheduler).toHaveYielded(['ClientDoubler']); + expect(ReactNoop).toMatchRenderedOutput( + <> +
:S1:
+
:S1:
+ , + ); + }); }); describe('ServerContext', () => { From d72d44de3d95ac7f2d431ec35992e0af92aaa79e Mon Sep 17 00:00:00 2001 From: Josh Story Date: Tue, 31 May 2022 14:41:08 -0700 Subject: [PATCH 6/6] lints and gates --- packages/react-client/src/__tests__/ReactFlight-test.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/react-client/src/__tests__/ReactFlight-test.js b/packages/react-client/src/__tests__/ReactFlight-test.js index b38db8c58a22..505b9072ff7c 100644 --- a/packages/react-client/src/__tests__/ReactFlight-test.js +++ b/packages/react-client/src/__tests__/ReactFlight-test.js @@ -564,7 +564,6 @@ describe('ReactFlight', () => { ); }); - // @gate enableServerContext it('[TODO] it does not warn if you render a server element passed to a client module reference twice on the client when using useId', async () => { // @TODO Today if you render a server component with useId and pass it to a client component and that client component renders the element in two or more // places the id used on the server will be duplicated in the client. This is a deviation from the guarantees useId makes for Fizz/Client and is a consequence @@ -572,8 +571,8 @@ describe('ReactFlight', () => { // so the output passed to the Client has no knowledge of the useId use. In the future we would like to add a DEV warning when this happens. For now // we just accept that it is a nuance of useId in Flight function App() { - let id = React.useId(); - let div =
{id}
; + const id = React.useId(); + const div =
{id}
; return ; }