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

Merged
merged 2 commits into from
Feb 12, 2024
Merged
Changes from 1 commit
Commits
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
Next Next commit
Transfer debug info for server-to-server request
A Flight Server can be a consumer of a stream from another Server.

In this case the meta data is attached to debugInfo properties on lazy,
Promises, Arrays or Elements that might in turn get forwarded to the next
stream. In this case we want to forward this debug information to the
client.
  • Loading branch information
sebmarkbage committed Feb 8, 2024
commit ec33294ea6ffeda4cd99e9ad2b32e2ff5a37879b
55 changes: 55 additions & 0 deletions packages/react-client/src/__tests__/ReactFlight-test.js
Original file line number Diff line number Diff line change
@@ -1806,4 +1806,59 @@ 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

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

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

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

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 promise = Promise.resolve(<ThirdPartyComponent />);

const thirdPartyTransport = ReactNoopFlightServer.render([promise, lazy]);

// 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'}] : 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'}] : undefined,
);
expect(thirdPartyChildren[1]._debugInfo).toEqual(
__DEV__ ? [{name: 'ThirdPartyLazyComponent'}] : undefined,
);
ReactNoop.render(result);
});

expect(ReactNoop).toMatchRenderedOutput(
<div>
Hello, <span>stranger</span>
<span>!</span>
</div>,
);
});
});
85 changes: 82 additions & 3 deletions packages/react-server/src/ReactFlightServer.js
Original file line number Diff line number Diff line change
@@ -107,6 +107,9 @@ import {SuspenseException, getSuspendedThenable} from './ReactFlightThenable';

initAsyncDebugInfo();

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

const ObjectPrototype = Object.prototype;

type JSONValue =
@@ -325,6 +328,14 @@ function serializeThenable(
request.abortableTasks,
);

if (__DEV__) {
// If this came from Flight, forward any debug info into this new row.
const debugInfo: ?ReactDebugInfo = (thenable: any)._debugInfo;
if (debugInfo) {
forwardDebugInfo(request, newTask.id, debugInfo);
}
}

switch (thenable.status) {
case 'fulfilled': {
// We have the resolved value, we can go ahead and schedule it for serialization.
@@ -475,6 +486,10 @@ function createLazyWrapperAroundWakeable(wakeable: Wakeable) {
_payload: thenable,
_init: readThenable,
};
if (__DEV__) {
// If this came from React, transfer the debug info.
lazyType._debugInfo = (thenable: any)._debugInfo || [];
}
return lazyType;
}

@@ -552,6 +567,22 @@ function renderFragment(
task: Task,
children: $ReadOnlyArray<ReactClientValue>,
): ReactJSONValue {
if (__DEV__) {
const debugInfo: ?ReactDebugInfo = (children: any)._debugInfo;
if (debugInfo) {
// If this came from Flight, forward any debug info into this new row.
if (debugID === null) {
// We don't have a chunk to assign debug info. We need to outline this
// component to assign it an ID.
return outlineTask(request, task);
} else {
// Forward any debug info we have the first time we see it.
// We do this after init so that we have received all the debug info
// from the server by the time we emit it.
forwardDebugInfo(request, debugID, debugInfo);
}
}
}
if (!enableServerComponentKeys) {
return children;
}
@@ -1206,6 +1237,22 @@ function renderModelDestructive(
}

const element: React$Element<any> = (value: any);

if (__DEV__) {
const debugInfo: ?ReactDebugInfo = (value: any)._debugInfo;
if (debugInfo) {
// If this came from Flight, forward any debug info into this new row.
if (debugID === null) {
// We don't have a chunk to assign debug info. We need to outline this
// component to assign it an ID.
return outlineTask(request, task);
} else {
// Forward any debug info we have the first time we see it.
forwardDebugInfo(request, debugID, debugInfo);
}
}
}

// Attempt to render the Server Component.
return renderElement(
request,
@@ -1218,9 +1265,30 @@ function renderModelDestructive(
);
}
case REACT_LAZY_TYPE: {
const payload = (value: any)._payload;
const init = (value: any)._init;
// Reset the task's thenable state before continuing. If there was one, it was
// from suspending the lazy before.
task.thenableState = null;
Comment on lines +1278 to +1280
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a silly question, but why adding / changing debugInfo has side-effects on task.thenableState?
Or is it just a small drive-by fix?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea, it's mainly a drive-by fix. The bug was introduced in #28068


const lazy: LazyComponent<any, any> = (value: any);
const payload = lazy._payload;
const init = lazy._init;
const resolvedModel = init(payload);
if (__DEV__) {
const debugInfo: ?ReactDebugInfo = lazy._debugInfo;
if (debugInfo) {
// If this came from Flight, forward any debug info into this new row.
if (debugID === null) {
// We don't have a chunk to assign debug info. We need to outline this
// component to assign it an ID.
return outlineTask(request, task);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems unfortunate that we fork the dev/prod behavior here. Means we need to have faith that outlining won't hide or express bugs that won't happen absent the debugInfo.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well it would show up in prod. As long as it's unobservable it doesn't affect dev/prod behavior for users. At some point we just have to trust that we don't mess up the implementation.

} else {
// Forward any debug info we have the first time we see it.
// We do this after init so that we have received all the debug info
// from the server by the time we emit it.
forwardDebugInfo(request, debugID, debugInfo);
}
}
}
return renderModelDestructive(
request,
task,
@@ -1649,7 +1717,7 @@ function emitModelChunk(request: Request, id: number, json: string): void {
function emitDebugChunk(
request: Request,
id: number,
debugInfo: {name: string},
debugInfo: {+name?: string},
): void {
if (!__DEV__) {
// These errors should never make it into a build so we don't need to encode them in codes.json
@@ -1665,6 +1733,17 @@ function emitDebugChunk(
request.completedRegularChunks.push(processedChunk);
}

function forwardDebugInfo(
request: Request,
id: number,
debugInfo: ReactDebugInfo,
) {
for (let i = 0; i < debugInfo.length; i++) {
request.pendingChunks++;
emitDebugChunk(request, id, debugInfo[i]);
}
}

const emptyRoot = {};

function retryTask(request: Request, task: Task): void {