[Flight] Transfer Debug Info in Server-to-Server Flight Requests #28275
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
sebmarkbage
force-pushed
the
servertoserver
branch
2 times, most recently
from
b5c0e57
to
053e9c1
Compare
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.
This lets you name the server that is producing the debug info so that you can trace the origin of where that component is executing.
sebmarkbage
force-pushed
the
servertoserver
branch
from
053e9c1
to
c4c5e0f
Compare
|
|
||
| function ServerComponent({transport}) { | ||
| // This is a Server Component that receives other Server Components from a third party. | ||
| const children = ReactNoopFlightClient.read(transport); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
(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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Btw, untrusted is NOT supported. Flight parsing isn't vetted from a security perspective if the protocol is from an untrusted source.
There was a problem hiding this comment.
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!)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| // Reset the task's thenable state before continuing. If there was one, it was | ||
| // from suspending the lazy before. | ||
| task.thenableState = null; |
There was a problem hiding this comment.
Maybe a silly question, but why adding / changing debugInfo has side-effects on task.thenableState?
Or is it just a small drive-by fix?
There was a problem hiding this comment.
Yea, it's mainly a drive-by fix. The bug was introduced in #28068
| // DEV-only | ||
| environmentName: string, |
There was a problem hiding this comment.
Why can't we make this field optional?
This could backfire once someone decides to use this field and not actually inspecting the type definition.
There was a problem hiding this comment.
Yeah personally I'd prefer we any cast the one place we read the name than do this to the Request type
There was a problem hiding this comment.
We have had this pattern elsewhere. It's nice because there's only one place to mess it up.
There will be many more places where environmentName gets used but the Request type is only initialized in one place.
Optional fields are bad for perf when you do mess them up because it breaks the hidden class. You could potentially accidentally add a field by writing to it unconditionally.
I don't like that we use optional fields for Fiber for example because we instead have bugs in the continued working with the code by doing undefined checks, which lets things pass and keep working even if they're undefined or missing. We typically check for example === null exactly so that if we get an undefined flowing through the system it should error. Even if that errors only in production because then we'll quickly find the mistake.
| // DEV-only | ||
| environmentName: string, |
There was a problem hiding this comment.
Yeah personally I'd prefer we any cast the one place we read the name than do this to the Request type
| 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
) 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 in the stream. I also added a DEV only `environmentName` option to the Flight Server. This lets you name the server that is producing the debug info so that you can trace the origin of where that component is executing. This defaults to `"server"`. DevTools could use this for badges or different colors. DiffTrain build for [629541b](629541b)
### React upstream changes - facebook/react#28333 - facebook/react#28334 - facebook/react#28378 - facebook/react#28377 - facebook/react#28376 - facebook/react#28338 - facebook/react#28331 - facebook/react#28336 - facebook/react#28320 - facebook/react#28317 - facebook/react#28375 - facebook/react#28367 - facebook/react#28380 - facebook/react#28368 - facebook/react#28343 - facebook/react#28355 - facebook/react#28374 - facebook/react#28362 - facebook/react#28344 - facebook/react#28339 - facebook/react#28353 - facebook/react#28346 - facebook/react#25790 - facebook/react#28352 - facebook/react#28326 - facebook/react#27688 - facebook/react#28329 - facebook/react#28332 - facebook/react#28340 - facebook/react#28327 - facebook/react#28325 - facebook/react#28324 - facebook/react#28309 - facebook/react#28310 - facebook/react#28307 - facebook/react#28306 - facebook/react#28315 - facebook/react#28318 - facebook/react#28226 - facebook/react#28308 - facebook/react#27563 - facebook/react#28297 - facebook/react#28286 - facebook/react#28284 - facebook/react#28275 - facebook/react#28145 - facebook/react#28301 - facebook/react#28224 - facebook/react#28152 - facebook/react#28296 - facebook/react#28294 - facebook/react#28279 - facebook/react#28273 - facebook/react#28269 - facebook/react#28376 - facebook/react#28338 - facebook/react#28331 - facebook/react#28336 - facebook/react#28320 - facebook/react#28317 - facebook/react#28375 - facebook/react#28367 - facebook/react#28380 - facebook/react#28368 - facebook/react#28343 - facebook/react#28355 - facebook/react#28374 - facebook/react#28362 - facebook/react#28344 - facebook/react#28339 - facebook/react#28353 - facebook/react#28346 - facebook/react#25790 - facebook/react#28352 - facebook/react#28326 - facebook/react#27688 - facebook/react#28329 - facebook/react#28332 - facebook/react#28340 - facebook/react#28327 - facebook/react#28325 - facebook/react#28324 - facebook/react#28309 - facebook/react#28310 - facebook/react#28307 - facebook/react#28306 - facebook/react#28315 - facebook/react#28318 - facebook/react#28226 - facebook/react#28308 - facebook/react#27563 - facebook/react#28297 - facebook/react#28286 - facebook/react#28284 - facebook/react#28275 - facebook/react#28145 - facebook/react#28301 - facebook/react#28224 - facebook/react#28152 - facebook/react#28296 - facebook/react#28294 - facebook/react#28279 - facebook/react#28273 - facebook/react#28269 Closes NEXT-2542 Disable ppr test for strict mode for now, @acdlite will check it and we'll sync again
…ebook#28275) 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 in the stream. I also added a DEV only `environmentName` option to the Flight Server. This lets you name the server that is producing the debug info so that you can trace the origin of where that component is executing. This defaults to `"server"`. DevTools could use this for badges or different colors.
) 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 in the stream. I also added a DEV only `environmentName` option to the Flight Server. This lets you name the server that is producing the debug info so that you can trace the origin of where that component is executing. This defaults to `"server"`. DevTools could use this for badges or different colors. DiffTrain build for commit 629541b.
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 in the stream.
I also added a DEV only
environmentNameoption to the Flight Server. This lets you name the server that is producing the debug info so that you can trace the origin of where that component is executing. This defaults to"server". DevTools could use this for badges or different colors.