-
Notifications
You must be signed in to change notification settings - Fork 47.2k
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] New Server Rendering Infra #14144
Conversation
ReactDOM: size: 0.0%, gzip: -0.0% Details of bundled changes.Comparing: f1bf281...b1182d1 react-dom
react-noop-renderer
react-stream
Generated by 🚫 dangerJS |
@@ -0,0 +1,81 @@ | |||
/** |
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.
@gaearon I named this ReactFizzRenderer but I don't think it makes sense since we use the term "renderer" to refer to the compiled output or the HostConfig. But Reconciler doesn't make sense since that's exactly what it doesn't do. It does everything but reconcile. Ideas?
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.
Umm.. Writer? Streamer? You know like cool kids on YouTube.
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.
Or just Server.
const ReactFizzRenderer = require('./src/ReactFizzRenderer'); | ||
|
||
// TODO: decide on the top-level export form. | ||
// This is hacky but makes it work with both Rollup and Jest. |
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.
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.
Oh, the other entries do that. Maybe I'm confused.
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.
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 think we do that for packages that are ready to be ESM. But here the actual output will be a function anyway so it doesn’t matter. I guess could work either way.
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 understanding based on absolutely nothing was that old packages do this because to switch to ESM would be a breaking change. But new packages don't have that restriction.
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'd love to fix this though. This is probably really bad for inlining/DCE.
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.
Seems legit
packages/react-dom/fizz.js
Outdated
@@ -0,0 +1,12 @@ | |||
/** |
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.
Let’s add unstable to the entry point
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.
To the file name or the method names?
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’d add to the file
packages/react-dom/package.json
Outdated
@@ -29,6 +29,7 @@ | |||
"server.js", | |||
"server.browser.js", | |||
"server.node.js", | |||
"fizz.js", |
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.
Also fizz.node.js?
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.
hm. Why doesn't this break ReactDOMFizzServer-test.js
in build tests? I'll need to look into that.
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.
Because npm entry point (see npm folder) directly points the bundle. The entry point above (which you redirect) only used by Jest. So technically this change isn’t even necessary right now. But would be less confusing.
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.
For consistency you can make npm/fizz
also do a re export of ./fizz.node
. Like npm/server
does.
@@ -0,0 +1,35 @@ | |||
{ | |||
"name": "react-stream", |
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.
Set private to true to not block the sync?
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.
You mean the release? We still want it in the sync so we can try it out at FB.
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.
Ah yes I mean the release
@@ -0,0 +1,81 @@ | |||
/** |
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.
Or just Server.
@@ -0,0 +1,44 @@ | |||
/** |
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.
@gaearon I put this in react-stream/src/
which is a bit weird because it's not part of the core algorithm. It's part of a possible host config. I'm not sure this makes sense to put it here but it's not really shared
and we really shouldn't make shared
a dumping ground.
However, I guess in the future we could have react-stream
export preconfigured environments and all you fill in is the format config. E.g. react-stream/index.node.js
, react-stream/index.browser.js
.
So maybe that makes sense.
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.
Sure. We also never got these things quite right from first try so I wouldn’t expect to do it 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.
Maybe you can add a comment about this? I was just puzzled over this because I forgot.
8bd123b
to
e65b22f
Compare
I also added a browser build. e65b22f |
I'm moderately familiar with the current server renderer and after having gone through this I just have to add that I really like this approach for a new renderer! Makes so much sense to me. |
314a8e0
to
02e7020
Compare
Add a new package for react-stream which allows for custom server renderer outputs. I picked the name because it's a reasonable name but also because the npm name is currently owned by a friend of the project. The react-dom build has its own inlined server renderer under the name `react-dom/fizz`. There is also a noop renderer to be used for testing. At some point we might add a public one to test-renderer but for now I don't want to have to think about public API design for the tests.
We need to separate the format (DOM, React Native, etc) from the host running the server (Node, Browser, etc).
The Node DOM API is pipeToNodeStream which accepts a writable stream.
Simpler API this way but also avoids having to fork the wrapper config. Fixes noop builds.
Used by the server renderer
Also use forwarding to it from fizz.js in builds so that tests covers this.
or even name it yet
This lets Fizz render to WHATWG streams. E.g. for rendering in a Service Worker. I added react-dom/unstable-fizz.browser as the entry point for this. Since we now have two configurations of DOM. I had to add another inlinedHostConfigs configuration called `dom-browser`. The reconciler treats this configuration the same as `dom`. For stream it checks against the ReactFizzHostConfigBrowser instead of the Node one.
This is for testing server rendering - on the client.
Since we landed the Fire infra we can land this too. |
* [Fizz] Add Flow/Jest/Rollup build infra Add a new package for react-stream which allows for custom server renderer outputs. I picked the name because it's a reasonable name but also because the npm name is currently owned by a friend of the project. The react-dom build has its own inlined server renderer under the name `react-dom/fizz`. There is also a noop renderer to be used for testing. At some point we might add a public one to test-renderer but for now I don't want to have to think about public API design for the tests. * Add FormatConfig too We need to separate the format (DOM, React Native, etc) from the host running the server (Node, Browser, etc). * Basic wiring between Node, Noop and DOM configs The Node DOM API is pipeToNodeStream which accepts a writable stream. * Merge host and format config in dynamic react-stream entry point Simpler API this way but also avoids having to fork the wrapper config. Fixes noop builds. * Add setImmediate/Buffer globals to lint config Used by the server renderer * Properly include fizz.node.js Also use forwarding to it from fizz.js in builds so that tests covers this. * Make react-stream private since we're not ready to publish or even name it yet * Rename Renderer -> Streamer * Prefix react-dom/fizz with react-dom/unstable-fizz * Add Fizz Browser host config This lets Fizz render to WHATWG streams. E.g. for rendering in a Service Worker. I added react-dom/unstable-fizz.browser as the entry point for this. Since we now have two configurations of DOM. I had to add another inlinedHostConfigs configuration called `dom-browser`. The reconciler treats this configuration the same as `dom`. For stream it checks against the ReactFizzHostConfigBrowser instead of the Node one. * Add Fizz Browser Fixture This is for testing server rendering - on the client. * Lower version number to detach it from react-reconciler version
* [Fizz] Add Flow/Jest/Rollup build infra Add a new package for react-stream which allows for custom server renderer outputs. I picked the name because it's a reasonable name but also because the npm name is currently owned by a friend of the project. The react-dom build has its own inlined server renderer under the name `react-dom/fizz`. There is also a noop renderer to be used for testing. At some point we might add a public one to test-renderer but for now I don't want to have to think about public API design for the tests. * Add FormatConfig too We need to separate the format (DOM, React Native, etc) from the host running the server (Node, Browser, etc). * Basic wiring between Node, Noop and DOM configs The Node DOM API is pipeToNodeStream which accepts a writable stream. * Merge host and format config in dynamic react-stream entry point Simpler API this way but also avoids having to fork the wrapper config. Fixes noop builds. * Add setImmediate/Buffer globals to lint config Used by the server renderer * Properly include fizz.node.js Also use forwarding to it from fizz.js in builds so that tests covers this. * Make react-stream private since we're not ready to publish or even name it yet * Rename Renderer -> Streamer * Prefix react-dom/fizz with react-dom/unstable-fizz * Add Fizz Browser host config This lets Fizz render to WHATWG streams. E.g. for rendering in a Service Worker. I added react-dom/unstable-fizz.browser as the entry point for this. Since we now have two configurations of DOM. I had to add another inlinedHostConfigs configuration called `dom-browser`. The reconciler treats this configuration the same as `dom`. For stream it checks against the ReactFizzHostConfigBrowser instead of the Node one. * Add Fizz Browser Fixture This is for testing server rendering - on the client. * Lower version number to detach it from react-reconciler version
let chunk = chunks[i]; | ||
writeChunk(destination, chunk); | ||
} | ||
} finally { |
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.
Why doesn't it keep the chunks that failed to be written (due to an exception) in completedChunks
to be retried later?
} finally { | ||
completeWriting(destination); | ||
} | ||
close(destination); |
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.
Why is close(destination)
not called in case of an exception?
This adds a bunch of new stuff related to the build process for the new server renderer (Fizz).
It adds a new package
react-stream
. The name is temporary (but is nicely owned by @aickin).This is equivalent to
react-reconciler
but for server rendering. I considered putting them in the same package but they really shouldn't share anything and splitting them up into folders just make both seem more complicated.This is used to create a two new entry points.
react-dom/unstable-fizz
which is a server renderer for DOM. Can run in Node for Node streams or browsers for WHATWG streams.There is also a
react-noop-rendering/server
for tests.The interesting new twist here is that there is not just a HostConfig (Node.js, Service Worker, other). There's also a FormatConfig (DOM, RN, possibly variations of DOM).
Currently I just copy pasted all the FiberHostConfig overrides to also account for FizzHostConfig and FizzFormatConfig.
The
inlinedHostConfigs.js
model starts breaking down though. Currently thedom
configuration tests the DOM format config + Node host config. However, that means we wouldn't get coverage on DOM format config + Service Worker host config. Maybe that's fine as long as the Service Worker host config is covered by one of the other configurations?