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.

Sign up for GitHub

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] Transfer Debug Info in Server-to-Server Flight Requests #28275

Merged
merged 2 commits into from
Feb 12, 2024
Merged

[Flight] Transfer Debug Info in Server-to-Server Flight Requests #28275

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ec33294
Transfer debug info for server-to-server request
sebmarkbage Feb 7, 2024
c4c5e0f
Add environmentName option
sebmarkbage Feb 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/react-client/src/ReactFlightClient.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ const INITIALIZED = 'fulfilled';
const ERRORED = 'rejected';

// Dev-only
type ReactDebugInfo = Array<{+name?: string}>;
type ReactDebugInfo = Array<{+name?: string, +env?: string}>;

type PendingChunk<T> = {
status: 'pending',
Expand Down
68 changes: 66 additions & 2 deletions packages/react-client/src/__tests__/ReactFlight-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ describe('ReactFlight', () => {
const rootModel = await ReactNoopFlightClient.read(transport);
const greeting = rootModel.greeting;
expect(greeting._debugInfo).toEqual(
__DEV__ ? [{name: 'Greeting'}] : undefined,
__DEV__ ? [{name: 'Greeting', env: 'server'}] : undefined,
);
ReactNoop.render(greeting);
});
Expand All @@ -214,7 +214,7 @@ describe('ReactFlight', () => {
await act(async () => {
const promise = ReactNoopFlightClient.read(transport);
expect(promise._debugInfo).toEqual(
__DEV__ ? [{name: 'Greeting'}] : undefined,
__DEV__ ? [{name: 'Greeting', env: 'server'}] : undefined,
);
ReactNoop.render(await promise);
});
Expand Down Expand Up @@ -1806,4 +1806,68 @@ describe('ReactFlight', () => {

expect(ReactNoop).toMatchRenderedOutput(<div>Ba</div>);
});

it('preserves debug info for server-to-server pass through', async () => {
function ThirdPartyLazyComponent() {
return <span>!</span>;
}

const lazy = React.lazy(async () => ({
default: <ThirdPartyLazyComponent />,
}));

function ThirdPartyComponent() {
return <span>stranger</span>;
}

function ServerComponent({transport}) {
// This is a Server Component that receives other Server Components from a third party.
const children = ReactNoopFlightClient.read(transport);
Copy link
Collaborator

@unstubbable unstubbable Feb 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is an interesting case. Up until this PR I wasn't aware that server-to-server RSC is supposed to be supported. Unfortunately, this is not exposed as public API, or is it? I guess, we would need something like ReactFlightDOMClient.createFromFetch that works without an SSR manifest, and preserves client references, instead of trying to resolve them? Is this something that's planned?

Copy link
Contributor

Choose a reason for hiding this comment

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

(for context, the usecase is federation via RSC, as in https://x.com/dan_abramov2/status/1747983201748861274 for example. currently it requires... some stream serialization acrobatics)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Like anything else it's more of a bundler feature. You just need some way from the third party server to refer to "client" as meaning files in the "server". Like anything else the server needs a manifest from the client.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Btw, untrusted is NOT supported. Flight parsing isn't vetted from a security perspective if the protocol is from an untrusted source.

Copy link
Contributor

@lubieowoce lubieowoce Feb 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

hm. i'm not sure if a server being able to refer to things from another server is a prerequisite for this
(edit: it is, but WMF already has ways of doing this)

i think what @unstubbable was talking about is something that'd allow incorporating foreign (but trusted) trees server-side, without going into client-land. those trees wouldn't necessarily need to refer to things from the consuming server (and if they would, this'd likely be handled using "remotes", i.e. module federation's existing pattern for this).

so it's really more of an implementation headache. because right now this seems to require tunneling one RSC stream over another and then deserializing in client-land, as seen here:
https://twitter.com/lubieowoce/status/1744854538060771614?t=IK4O45iZvCU4isBmTHIf1A
and here:
https://twitter.com/ebey_jacob/status/1744793367085727952?t=fOPDaYf3LLelv7gYhvSuzQ&s=19
which works but seems unnecessarily cumbersome. granted, some of the cumbersomeness is just because Flight doesn't currently handle ReadableStreams as props, but even if it did, we'd still be tunneling

basically, i think the feature request would be for a server-land createFromReadableStream that keeps client references as references, so that it can be returned from a server component like any other element. (it's unfortunate that it'd have to parse just to serialize it right back, but i'm not sure if that can be avoided -- at the very least, rows need renumbering).

also if i'm missing something, @jacob-ebey can probably explain the use-case better, he's had experience implementing this, i just helped out with a couple bits.

ofc the federated use-case ALSO needs bundler support, because eventually you need to resolve those foreign client references to something. but i believe that's been implemented in userspace already, really """just""" a matter of hooking the existing machinery of module federation into it all.

(sorry for making this a huge long thing in a drive-by comment 😅 probably not the best place, hope you don't mind!)

Copy link
Collaborator Author

@sebmarkbage sebmarkbage Feb 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yea, I mean we'll support ReadableStream pass through but that's not really recommended. That's just module federation.

It needs to be deserialized and re-serialized so that the third-party can also render server components inside the first party and everything else can kick in. Sure, there's a slight perf hit but so does SSR. There could be optimizations added on top of that which can keep some parts pass through but that's an optimization and not the core model.

Copy link
Collaborator Author

@sebmarkbage sebmarkbage Feb 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

basically, i think the feature request would be for a server-land createFromReadableStream that keeps client references as references, so that it can be returned from a server component like any other element. (it's unfortunate that it'd have to parse just to serialize it right back, but i'm not sure if that can be avoided

Although now I'm confused what you mean because this is supported. That's what this PR uses.

There's no limitation that /client can't be used in react-server environments.

https://github.com/facebook/react/blob/main/packages/react-server-dom-webpack/package.json#L46-L49

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh I think I understand the misunderstanding. The confusion is that Client References isn't actually 1:1 with "use client". It's not supposed to preserve the Client References. Instead, the Client Reference of a Third Party is a reference to a module in the first party. Requiring that module might be a Server Component but if it's "use client" then importing that same file will be a Client Reference again. So you don't need some way to do the rewriting.

return <div>Hello, {children}</div>;
}

const promiseComponent = Promise.resolve(<ThirdPartyComponent />);

const thirdPartyTransport = ReactNoopFlightServer.render(
[promiseComponent, lazy],
{
environmentName: 'third-party',
},
);

// Wait for the lazy component to initialize
await 0;

const transport = ReactNoopFlightServer.render(
<ServerComponent transport={thirdPartyTransport} />,
);

await act(async () => {
const promise = ReactNoopFlightClient.read(transport);
expect(promise._debugInfo).toEqual(
__DEV__ ? [{name: 'ServerComponent', env: 'server'}] : undefined,
);
const result = await promise;
const thirdPartyChildren = await result.props.children[1];
// We expect the debug info to be transferred from the inner stream to the outer.
expect(thirdPartyChildren[0]._debugInfo).toEqual(
__DEV__
? [{name: 'ThirdPartyComponent', env: 'third-party'}]
: undefined,
);
expect(thirdPartyChildren[1]._debugInfo).toEqual(
__DEV__
? [{name: 'ThirdPartyLazyComponent', env: 'third-party'}]
: undefined,
);
ReactNoop.render(result);
});

expect(ReactNoop).toMatchRenderedOutput(
<div>
Hello, <span>stranger</span>
<span>!</span>
</div>,
);
});
});
6 changes: 5 additions & 1 deletion packages/react-noop-renderer/src/ReactNoopFlightServer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,10 @@ const ReactNoopFlightServer = ReactFlightServer({
});

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

function render(model: ReactClientValue, options?: Options): Destination {
Expand All @@ -80,6 +82,8 @@ function render(model: ReactClientValue, options?: Options): Destination {
bundlerConfig,
options ? options.onError : undefined,
options ? options.identifierPrefix : undefined,
options ? options.onPostpone : undefined,
options ? options.environmentName : undefined,
);
ReactNoopFlightServer.startWork(request);
ReactNoopFlightServer.startFlowing(request, destination);
Expand Down
2 changes: 2 additions & 0 deletions packages/react-server-dom-esm/src/ReactFlightDOMServerNode.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ function createDrainHandler(destination: Destination, request: Request) {
}

type Options = {
environmentName?: string,
onError?: (error: mixed) => void,
onPostpone?: (reason: string) => void,
identifierPrefix?: string,
Expand All @@ -73,6 +74,7 @@ function renderToPipeableStream(
options ? options.onError : undefined,
options ? options.identifierPrefix : undefined,
options ? options.onPostpone : undefined,
options ? options.environmentName : undefined,
);
let hasStartedFlowing = false;
startWork(request);
Expand Down
2 changes: 2 additions & 0 deletions packages/react-server-dom-fb/src/ReactFlightDOMServerFB.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ function renderToDestination(
model,
null,
options ? options.onError : undefined,
undefined,
undefined,
);
startWork(request);
startFlowing(request, destination);
Expand Down
2 changes: 2 additions & 0 deletions packages/react-server-dom-turbopack/src/ReactFlightDOMServerBrowser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ export {
} from './ReactFlightTurbopackReferences';

type Options = {
environmentName?: string,
identifierPrefix?: string,
signal?: AbortSignal,
onError?: (error: mixed) => void,
Expand All @@ -51,6 +52,7 @@ function renderToReadableStream(
options ? options.onError : undefined,
options ? options.identifierPrefix : undefined,
options ? options.onPostpone : undefined,
options ? options.environmentName : undefined,
);
if (options && options.signal) {
const signal = options.signal;
Expand Down
2 changes: 2 additions & 0 deletions packages/react-server-dom-turbopack/src/ReactFlightDOMServerEdge.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ export {
} from './ReactFlightTurbopackReferences';

type Options = {
environmentName?: string,
identifierPrefix?: string,
signal?: AbortSignal,
onError?: (error: mixed) => void,
Expand All @@ -51,6 +52,7 @@ function renderToReadableStream(
options ? options.onError : undefined,
options ? options.identifierPrefix : undefined,
options ? options.onPostpone : undefined,
options ? options.environmentName : undefined,
);
if (options && options.signal) {
const signal = options.signal;
Expand Down
2 changes: 2 additions & 0 deletions packages/react-server-dom-turbopack/src/ReactFlightDOMServerNode.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ function createDrainHandler(destination: Destination, request: Request) {
}

type Options = {
environmentName?: string,
onError?: (error: mixed) => void,
onPostpone?: (reason: string) => void,
identifierPrefix?: string,
Expand All @@ -70,6 +71,7 @@ function renderToPipeableStream(
options ? options.onError : undefined,
options ? options.identifierPrefix : undefined,
options ? options.onPostpone : undefined,
options ? options.environmentName : undefined,
);
let hasStartedFlowing = false;
startWork(request);
Expand Down
2 changes: 2 additions & 0 deletions packages/react-server-dom-webpack/src/ReactFlightDOMServerBrowser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export {
} from './ReactFlightWebpackReferences';

type Options = {
environmentName?: string,
identifierPrefix?: string,
signal?: AbortSignal,
onError?: (error: mixed) => void,
Expand All @@ -55,6 +56,7 @@ function renderToReadableStream(
options ? options.onError : undefined,
options ? options.identifierPrefix : undefined,
options ? options.onPostpone : undefined,
options ? options.environmentName : undefined,
);
if (options && options.signal) {
const signal = options.signal;
Expand Down
2 changes: 2 additions & 0 deletions packages/react-server-dom-webpack/src/ReactFlightDOMServerEdge.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export {
} from './ReactFlightWebpackReferences';

type Options = {
environmentName?: string,
identifierPrefix?: string,
signal?: AbortSignal,
onError?: (error: mixed) => void,
Expand All @@ -55,6 +56,7 @@ function renderToReadableStream(
options ? options.onError : undefined,
options ? options.identifierPrefix : undefined,
options ? options.onPostpone : undefined,
options ? options.environmentName : undefined,
);
if (options && options.signal) {
const signal = options.signal;
Expand Down
2 changes: 2 additions & 0 deletions packages/react-server-dom-webpack/src/ReactFlightDOMServerNode.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ function createCancelHandler(request: Request, reason: string) {
}

type Options = {
environmentName?: string,
onError?: (error: mixed) => void,
onPostpone?: (reason: string) => void,
identifierPrefix?: string,
Expand All @@ -82,6 +83,7 @@ function renderToPipeableStream(
options ? options.onError : undefined,
options ? options.identifierPrefix : undefined,
options ? options.onPostpone : undefined,
options ? options.environmentName : undefined,
);
let hasStartedFlowing = false;
startWork(request);
Expand Down
2 changes: 1 addition & 1 deletion packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ describe('ReactFlightDOMEdge', () => {
<ServerComponent recurse={20} />,
);
const serializedContent = await readResult(stream);
const expectedDebugInfoSize = __DEV__ ? 30 * 20 : 0;
const expectedDebugInfoSize = __DEV__ ? 42 * 20 : 0;
expect(serializedContent.length).toBeLessThan(150 + expectedDebugInfoSize);
});

Expand Down
Loading