-
Notifications
You must be signed in to change notification settings - Fork 46.7k
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
[Fizz/Flight] pipeToNodeWritable(..., writable).startWriting() -> renderToPipeableStream(...).pipe(writable) #22450
Conversation
0f3e005
to
2af324a
Compare
Comparing: 6485ef7...a127ca0 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
It is nice that this mirrors the ReadableStream for web streams which can be deferred when they're given to the response. |
2af324a
to
6aac895
Compare
For what it’s worth, Next.js primary reason for wanting to wait is for error handling: if we can generate at least the shell, we can safely rely on the client-side fallback for error handling. Waiting keeps the stream in a known state for us to manually recover outside of React (e.g. to flush a static, known good error page) in case an error occurs before then. If React could provide more guarantees or preferably manage the entire response stream for us, we could likely do away with that need. |
That makes sense. Maybe we need to support error boundaries at the root after all. |
@@ -20,7 +20,8 @@ module.exports = function(req, res) { | |||
const App = m.default.default || m.default; | |||
res.setHeader('Access-Control-Allow-Origin', '*'); | |||
const moduleMap = JSON.parse(data); | |||
pipeToNodeWritable(React.createElement(App), res, moduleMap); | |||
const {pipe} = renderToNodePipe(React.createElement(App), moduleMap); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies in advance for the unsolicited bikeshedding. The name renderToNodePipe
just feels odd to me. Thoughts on something like streamRoot(…).pipe(…)
or renderStreamingRoot(…).pipe(…)
for symmetry with hydrateRoot
and createRoot
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createRoot
and hydrateRoot
deal with a "root" object. The object is important because it's stateful (you can update a root or unmount a root later).
This seems a bit different because (1) you don't get a root object — the root object is a client-only concept -- and (2) it's not possible to update it or such. So it seems misleading. I'd expect something like streamRoot
to accept or produce a "root" object, which it doesn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I also had the impression that “render” was out of place but the equivalent is the root.render(…) function which is when it starts rendering.
Interestingly I have been questioning the root api. It’s odd that you have to have a container before you render it because you really don’t need one to render it. Just to commit it. We used to have a way to explicitly commit and that the first time you need a container.
Similarly you don’t really need the HTML to be there before you start the hydration process. You can start rendering and then later map the tree onto the HTML.
We probably won’t change the client due to implementation details but it’s interesting that the client could end up mirroring this api more in the future.
I think my ideal name would be renderToNodeStream but we already have that name temporarily to help the upgrade path. And what you get isn’t really a Node stream but it kind of is meant to mimic a readable stream that is limited to the pipe() function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking around the “Node” part is that it’s worth clarifying that it’s non-standard. If it was to remain the defacto standard maybe we wouldn’t need the Node prefix but that doesn’t seem to be the case.
Since the modules are auto-swapped by condition we don’t really need a unique name. It can be the same name. However type systems don’t seem to consider the conditional environments yet so it’s nicer to just pretend all the exports exist for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createRoot and hydrateRoot deal with a "root" object. The object is important because it's stateful (you can update a root or unmount a root later).
Well, it is (or was) stateful in a sense. You couldn’t update it, but you can tell it to start writing or to abort. And you give it temporary ownership of a resource to mutate (a stream) along with a desired React tree, which is similar to the DOM node and tree you pass the other functions.
I guess the bigger issue is that it sounds like “root” might have a different meaning for y’all: I guess I assumed it just referred to the root of a React tree that can be controlled over time, but it sounds like it specifically means a root host element?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I think about “hydrateRoot(…)” is that you’re extracting a root that already existed (conceptually).
So in that sense it seems like someone just have created it at some point on the server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some ideas for rendering non-root subtrees for subsequent requests. Those would not be the same as roots on the client. However, they would likely be different APIs. Using the word root might help disambiguate them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks reasonable to me
6aac895
to
404e036
Compare
We don't really need it in this case because there's way less reason to delay the stream in Flight.
This mirrors the ReadableStream API in Node
404e036
to
460b829
Compare
This mimics the renderToReadableStream API for the browser.
One concern I had about this api was that it discourages starting to pipe earlier which means that preload tags can’t be emitted. One solution to that would be to have something like startWriting() but for holding back the head/shell content. Like flushShell(). That would mean that the web ReadableStream api would also need something like this which makes both apis a lot less elegant. However, the reason to hold back streaming is so you can target a bot. However bots also tend to benefit from status codes. Eg to ignore updating the previously scraped result. You can’t stream preload tags before the status code is sent since it’s part of the headers. As a result you have to hold back everything anyway if you want the option to set the status code. So it seems better to go for the simpler api. I renamed it to renderToPipeableStream to pair with renderToReadableStream. |
Summary: This sync includes the following changes: - **[579c008a7](facebook/react@579c008a7 )**: [Fizz/Flight] pipeToNodeWritable(..., writable).startWriting() -> renderToPipeableStream(...).pipe(writable) ([#22450](facebook/react#22450)) //<Sebastian Markbåge>// - **[f2c381131](facebook/react@f2c381131 )**: fix: useSyncExternalStoreExtra ([#22500](facebook/react#22500)) //<Daishi Kato>// - **[0ecbbe142](facebook/react@0ecbbe142 )**: Sync hydrate discrete events in capture phase and dont replay discrete events ([#22448](facebook/react#22448)) //<salazarm>// - **[a724a3b57](facebook/react@a724a3b57 )**: [RFC] Codemod invariant -> throw new Error ([#22435](facebook/react#22435)) //<Andrew Clark>// - **[201af81b0](facebook/react@201af81b0 )**: Release pooled cache reference in complete/unwind ([#22464](facebook/react#22464)) //<Joseph Savona>// - **[033efe731](facebook/react@033efe731 )**: Call get snapshot in useSyncExternalStore server shim ([#22453](facebook/react#22453)) //<salazarm>// - **[7843b142a](facebook/react@7843b142a )**: [Fizz/Flight] Pass in Destination lazily to startFlowing instead of in createRequest ([#22449](facebook/react#22449)) //<Sebastian Markbåge>// - **[d9fb383d6](facebook/react@d9fb383d6 )**: Extract queueing logic into shared functions ([#22452](facebook/react#22452)) //<Andrew Clark>// - **[9175f4d15](facebook/react@9175f4d15 )**: Scheduling Profiler: Show Suspense resource .displayName ([#22451](facebook/react#22451)) //<Brian Vaughn>// - **[eba248c39](facebook/react@eba248c39 )**: [Fizz/Flight] Remove reentrancy hack ([#22446](facebook/react#22446)) //<Sebastian Markbåge>// - **[66388150e](facebook/react@66388150e )**: Remove usereducer eager bailout ([#22445](facebook/react#22445)) //<Joseph Savona>// - **[d3e086932](facebook/react@d3e086932 )**: Make root.unmount() synchronous ([#22444](facebook/react#22444)) //<Andrew Clark>// - **[2cc6d79c9](facebook/react@2cc6d79c9 )**: Rename onReadyToStream to onCompleteShell ([#22443](facebook/react#22443)) //<Sebastian Markbåge>// - **[c88fb49d3](facebook/react@c88fb49d3 )**: Improve DEV errors if string coercion throws (Temporal.*, Symbol, etc.) ([#22064](facebook/react#22064)) //<Justin Grant>// - **[05726d72c](facebook/react@05726d72c )**: [Fix] Errors should not "unsuspend" a transition ([#22423](facebook/react#22423)) //<Andrew Clark>// - **[3746eaf98](facebook/react@3746eaf98 )**: Packages/React/src/ReactLazy ---> changing -1 to unintialized ([#22421](facebook/react#22421)) //<BIKI DAS>// - **[04ccc01d9](facebook/react@04ccc01d9 )**: Hydration errors should force a client render ([#22416](facebook/react#22416)) //<Andrew Clark>// - **[029fdcebb](facebook/react@029fdcebb )**: root.hydrate -> root.isDehydrated ([#22420](facebook/react#22420)) //<Andrew Clark>// - **[af87f5a83](facebook/react@af87f5a83 )**: Scheduling Profiler marks should include thrown Errors ([#22417](facebook/react#22417)) //<Brian Vaughn>// - **[d47339ea3](facebook/react@d47339ea3 )**: [Fizz] Remove assignID mechanism ([#22410](facebook/react#22410)) //<Sebastian Markbåge>// - **[3a50d9557](facebook/react@3a50d9557 )**: Never attach ping listeners in legacy Suspense ([#22407](facebook/react#22407)) //<Andrew Clark>// - **[82c8fa90b](facebook/react@82c8fa90b )**: Add back useMutableSource temporarily ([#22396](facebook/react#22396)) //<Andrew Clark>// - **[5b57bc6e3](facebook/react@5b57bc6e3 )**: [Draft] don't patch console during first render ([#22308](facebook/react#22308)) //<Luna Ruan>// - **[cf07c3df1](facebook/react@cf07c3df1 )**: Delete all but one `build2` reference ([#22391](facebook/react#22391)) //<Andrew Clark>// - **[bb0d06935](facebook/react@bb0d06935 )**: [build2 -> build] Local scripts //<Andrew Clark>// - **[0c81d347b](facebook/react@0c81d347b )**: Write artifacts to `build` instead of `build2` //<Andrew Clark>// - **[4da03c9fb](facebook/react@4da03c9fb )**: useSyncExternalStore React Native version ([#22367](facebook/react#22367)) //<salazarm>// - **[48d475c9e](facebook/react@48d475c9e )**: correct typos ([#22294](facebook/react#22294)) //<Bowen>// - **[cb6c619c0](facebook/react@cb6c619c0 )**: Remove Fiber fields that were used for hydrating useMutableSource ([#22368](facebook/react#22368)) //<Sebastian Silbermann>// - **[64e70f82e](facebook/react@64e70f82e )**: [Fizz] add avoidThisFallback support ([#22318](facebook/react#22318)) //<salazarm>// - **[3ee7a004e](facebook/react@3ee7a004e )**: devtools: Display actual ReactDOM API name in root type ([#22363](facebook/react#22363)) //<Sebastian Silbermann>// - **[79b8fc667](facebook/react@79b8fc667 )**: Implement getServerSnapshot in userspace shim ([#22359](facebook/react#22359)) //<Andrew Clark>// - **[86b3e2461](facebook/react@86b3e2461 )**: Implement useSyncExternalStore on server ([#22347](facebook/react#22347)) //<Andrew Clark>// - **[8209de269](facebook/react@8209de269 )**: Delete useMutableSource implementation ([#22292](facebook/react#22292)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions e8feb11...afcb9cd jest_e2e[run_all_tests] Reviewed By: yungsters Differential Revision: D31541359 fbshipit-source-id: c35941bc303fdf55cb061e9996200dc868a6f2af
…derToPipeableStream(...).pipe(writable) (facebook#22450) * Rename pipeToNodeWritable to renderToNodePipe * Add startWriting API to Flight We don't really need it in this case because there's way less reason to delay the stream in Flight. * Pass the destination to startWriting instead of renderToNode * Rename startWriting to pipe This mirrors the ReadableStream API in Node * Error codes * Rename to renderToPipeableStream This mimics the renderToReadableStream API for the browser.
This changes the API from:
to:
The idea is that this more closely mirrors what you'd typically do with a Node stream. It still allows you to delay writing until a time for your choice (such as after onCompleteAll).
However, now that I think about it there are not really that many best practices remaining.
The idea was that you could wait until you've accumulated enough info to populate the header with meta data - such as the HTTP status code and
<head>
.However, it's not really great to wait before writing because you can't emit the preload tags as early as possible in the stream. You don't have to wait for that to deal with
<head>
but you do for status codes.There's really only two valid options emerging:
It might be better to just let the API reflect that and have two root APIs that do just that and then you don't have to bother with when to call startWriting/pipe. So I might abandon this.
cc @devknoll