Skip to content
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

[Flight] Implement useId hook #24172

Merged
merged 6 commits into from May 31, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
100 changes: 96 additions & 4 deletions packages/react-client/src/__tests__/ReactFlight-test.js
Expand Up @@ -512,6 +512,99 @@ describe('ReactFlight', () => {
);
});

describe('Hooks', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a test for how useId would work for sever components that have children that are client components and need the same ID?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if I'm misunderstanding some finer point but my expectation is that if you use useId in Flight you will if necessary pass that id as props to client components like any other prop when needed for some purpose

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, just checking if there's a test for it already!

function DivWithId({children}) {
const id = React.useId();
return <div prop={id}>{children}</div>;
}

it('should support useId', () => {
function App() {
return (
<>
<DivWithId />
<DivWithId />
</>
);
}

const transport = ReactNoopFlightServer.render(<App />);
act(() => {
ReactNoop.render(ReactNoopFlightClient.read(transport));
});
expect(ReactNoop).toMatchRenderedOutput(
<>
<div prop=":S1:" />
<div prop=":S2:" />
</>,
);
});

it('accepts an identifier prefix that prefixes generated ids', () => {
function App() {
return (
<>
<DivWithId />
<DivWithId />
</>
);
}

const transport = ReactNoopFlightServer.render(<App />, {
identifierPrefix: 'foo',
});
act(() => {
ReactNoop.render(ReactNoopFlightClient.read(transport));
});
expect(ReactNoop).toMatchRenderedOutput(
<>
<div prop=":fooS1:" />
<div prop=":fooS2:" />
</>,
);
});

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() {
const id = React.useId();
const div = <div prop={id}>{id}</div>;
return <ClientDoublerModuleRef el={div} />;
}

function ClientDoubler({el}) {
Scheduler.unstable_yieldValue('ClientDoubler');
return (
<>
{el}
{el}
</>
);
}

const ClientDoublerModuleRef = moduleReference(ClientDoubler);

const transport = ReactNoopFlightServer.render(<App />);
expect(Scheduler).toHaveYielded([]);

act(() => {
ReactNoop.render(ReactNoopFlightClient.read(transport));
});

expect(Scheduler).toHaveYielded(['ClientDoubler']);
expect(ReactNoop).toMatchRenderedOutput(
<>
<div prop=":S1:">:S1:</div>
<div prop=":S1:">:S1:</div>
</>,
);
});
});

describe('ServerContext', () => {
// @gate enableServerContext
it('supports basic createServerContext usage', () => {
Expand Down Expand Up @@ -759,15 +852,14 @@ describe('ReactFlight', () => {
function Bar() {
return <span>{React.useContext(ServerContext)}</span>;
}
const transport = ReactNoopFlightServer.render(<Bar />, {}, [
['ServerContext', 'Override'],
]);
const transport = ReactNoopFlightServer.render(<Bar />, {
context: [['ServerContext', 'Override']],
});

act(() => {
const flightModel = ReactNoopFlightClient.read(transport);
ReactNoop.render(flightModel);
});

expect(ReactNoop).toMatchRenderedOutput(<span>Override</span>);
});

Expand Down
11 changes: 5 additions & 6 deletions packages/react-noop-renderer/src/ReactNoopFlightServer.js
Expand Up @@ -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);
Expand Down
Expand Up @@ -21,6 +21,7 @@ import {

type Options = {
onError?: (error: mixed) => void,
identifierPrefix?: string,
};

function render(
Expand All @@ -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);
Expand Down
Expand Up @@ -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(
{
Expand Down
Expand Up @@ -24,6 +24,8 @@ function createDrainHandler(destination, request) {

type Options = {
onError?: (error: mixed) => void,
context?: Array<[string, ServerContextJSONValue]>,
identifierPrefix?: string,
};

type PipeableStream = {|
Expand All @@ -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);
Expand Down
22 changes: 21 additions & 1 deletion packages/react-server/src/ReactFlightHooks.js
Expand Up @@ -8,10 +8,21 @@
*/

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 currentRequest = null;

export function prepareToUseHooksForRequest(request: Request) {
currentRequest = request;
}

export function resetHooksForRequest() {
currentRequest = null;
}

Comment on lines +16 to +25
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sebmarkbage there is already setCurrentCache behavior that I could consolidate since they are set and reset at the same points in performWork. For setting the cache I noticed that the previous cache is restored but my understanding of the code is the previous cache will always be null (meaning you cannot have a second performWork start before a first performWork finishes)

If the prevCache restoration is important it makes consolidation of multiple things to set awkward but we could move to just reading the cache from the request given it is set for the duration of performWork and holds the cache

function readContext<T>(context: ReactServerContext<T>): T {
if (__DEV__) {
if (context.$$typeof !== REACT_SERVER_CONTEXT_TYPE) {
Expand Down Expand Up @@ -61,7 +72,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, ?T) => void {
Expand Down Expand Up @@ -91,3 +102,12 @@ export function setCurrentCache(cache: Map<Function, mixed> | null) {
export function getCurrentCache() {
return currentCache;
}

function useId(): string {
if (currentRequest === null) {
throw new Error('useId can only be used while React is rendering');
}
const id = currentRequest.identifierCount++;
// use 'S' for Flight components to distinguish from 'R' and 'r' in Fizz/Client
return ':' + currentRequest.identifierPrefix + 'S' + id.toString(32) + ':';
}
19 changes: 14 additions & 5 deletions packages/react-server/src/ReactFlightServer.js
Expand Up @@ -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,
Expand Down Expand Up @@ -102,14 +108,12 @@ export type Request = {
writtenSymbols: Map<Symbol, number>,
writtenModules: Map<ModuleKey, number>,
writtenProviders: Map<string, number>,
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) {
Expand All @@ -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 = {
Expand All @@ -143,6 +148,8 @@ export function createRequest(
writtenSymbols: new Map(),
writtenModules: new Map(),
writtenProviders: new Map(),
identifierPrefix: identifierPrefix || '',
identifierCount: 1,
onError: onError === undefined ? defaultErrorHandler : onError,
toJSON: function(key: string, value: ReactModel): ReactJSONValue {
return resolveModelToJSON(request, this, key, value);
Expand Down Expand Up @@ -826,6 +833,7 @@ function performWork(request: Request): void {
const prevCache = getCurrentCache();
ReactCurrentDispatcher.current = Dispatcher;
setCurrentCache(request.cache);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of setting up two different contextual variables (and more to come) we should just use the request for the Cache too. That can be a follow up but should fast follow.

While we're add it we should also put the Cache behind the enableCache since it's not coupled to Flight.

prepareToUseHooksForRequest(request);

try {
const pingedSegments = request.pingedSegments;
Expand All @@ -843,6 +851,7 @@ function performWork(request: Request): void {
} finally {
ReactCurrentDispatcher.current = prevDispatcher;
setCurrentCache(prevCache);
resetHooksForRequest();
}
}

Expand Down
3 changes: 2 additions & 1 deletion scripts/error-codes/codes.json
Expand Up @@ -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"
}