Add ReactDOMNodeStream, adding streaming rendering. #10024
Conversation
|
Tests are working fine on my machine, but for some reason CI has I added a file at ETA: I've confirmed that |
| var React = require('react'); | ||
| var ReactDOMPartialRenderer = require('ReactDOMPartialRenderer'); | ||
|
|
||
| var Readable = require('stream').Readable; |
There was a problem hiding this comment.
It's recommended to use readable-stream instead. This might cause wildly different behavior depending on the node version you use
There was a problem hiding this comment.
Thanks for the comment; I wasn't aware of readable-stream!
I did some reading on the subject, though (in particular, this blog post referenced by the readable-stream README), and it seemed like this is only really a problem for versions of Node <= 0.12. Given that those versions of Node are all deprecated, is there still a good reason to use readable-stream that you know of?
(and thanks again for your comment!)
There was a problem hiding this comment.
Hey yeah glad you like it - readable stream contains fixes that aren't necessarily backported to older Node versions (a new version was published today which contains fixes for the ._destroy stuff iirc). Generally it's just a good practice to pin to a specific stream version rather than relying on whatever version is in core. Hope that makes sense (:
There was a problem hiding this comment.
Is there any where we can read someone arguing against depending on the readable-stream package? This would make it easier to evaluate the counter-arguments for someone not too involved in the details. Seems non-obvious to me.
There was a problem hiding this comment.
E.g. how does it integrate with other code that assumes the native Node stream?
There was a problem hiding this comment.
I haven't found much other documentation or argumentation about readable-stream.
E.g. how does it integrate with other code that assumes the native Node stream?
My understanding (which is a bit fuzzy) is that it literally is the native Node stream code pulled out of Node core. The authors are the Node Streams Working Group.
I think the benefit of using readable-stream would be more consistency of behavior across versions of Node, potentially making it easier to repro reported bugs. Of course, Stream hasn't changed that much since 4.x (unlike in the 0.9-0.12 timeframe), so this may not be that big a deal.
On the other hand, linking to a particular version of readable-stream means that someone on the React team should probably track when readable-stream gets released and cut new releases of React if it's useful to upgrade. If we use native stream, though, developers don't have to wait on the React team to get the newest version of streams; they can choose to upgrade Node themselves.
For now, I vote we use stream; it's a simpler setup with less maintenance involved. If we find there are a bunch of streaming bugs due to Node versions, though, we can replace it with a pinned version of readable-stream. Does that sound OK?
…des during tests.
|
This looks good to me. I don't have a problem exposing this as an API but the strategy I had in mind for async component dependencies, coroutines and error boundaries would be a bit different in its performance characteristics. I also think we'll want to expose multiple modes of rendering. E.g. one mode for plain simple HTML like we do today. Another mode for out-of-order "smart" rendering that inserts a bunch of script tags and other junk to do out-of-order stream rendering which can be faster than the simple mode. The naming convention should account for this. |
| ReactDOMNodeStream.renderToStream(element) | ||
| ``` | ||
|
|
||
| Render a React element to its initial HTML. This should only be used in Node.js; it will not work in the browser, since the browser does not support Node.js streams. React will return a [https://nodejs.org/api/stream.html#stream_readable_streams](Readable stream) that outputs an HTML string. The HTML output by this stream should be character-for-character equivalent to the output of [https://facebook.github.io/react/docs/react-dom-server.html#rendertostring](ReactDOMServer.renderToString). You can use this method to generate HTML on the server and send the markup down on the initial request for faster page loads and to allow search engines to crawl your pages for SEO purposes. |
There was a problem hiding this comment.
Yep, you're right! Thanks!
There was a problem hiding this comment.
Should be fixed in 132567d. Thanks!
| ReactDOMNodeStream.renderToStream(element) | ||
| ``` | ||
|
|
||
| Render a React element to its initial HTML. This should only be used in Node.js; it will not work in the browser, since the browser does not support Node.js streams. React will return a [https://nodejs.org/api/stream.html#stream_readable_streams](Readable stream) that outputs an HTML string. The HTML output by this stream should be character-for-character equivalent to the output of [https://facebook.github.io/react/docs/react-dom-server.html#rendertostring](ReactDOMServer.renderToString). You can use this method to generate HTML on the server and send the markup down on the initial request for faster page loads and to allow search engines to crawl your pages for SEO purposes. |
There was a problem hiding this comment.
The HTML output by this stream should be character-for-character equivalent to the output...
Phrasing is a little awkward, could we convey the same idea without "character-for-character" and "should"?
The HTML output by this stream will be exactly equal to what
ReactDOMServer.renderToStringwould return.
There was a problem hiding this comment.
ReactDOMServer.renderToString link should also be inside backticks
There was a problem hiding this comment.
Thanks for the suggestions! Should be fixed in b8f2529.
…for helping me out.
… for the suggestion.
|
|
||
| ## Overview | ||
|
|
||
| The `ReactDOMNodeStream` class allows you to render your components in Node.js and stream the resulting markup. |
There was a problem hiding this comment.
I suppose it's not a class?
There was a problem hiding this comment.
Right you are!
Should be fixed in b6f9e06, and I opened PR #10027 to fix it in the doc for ReactDOMServer, too.
Thanks for the help!
…ment) , ReactDOMServer is an object, not a class.
…ReactDOMServer is an object, not a class. (#10027)
|
So, I've fixed the CI build problem and got an LGTM from @sebmarkbage and reviews from @aweary & @gaearon. My understanding is that I am allowed to merge it now, but since I know this feature has been somewhat controversial in the past, I'll give a little more time (24 hours, say?) in case anyone else wants to chime in or wants more time to review. Thanks to everyone who has looked at this so far! |
| * LICENSE file in the root directory of this source tree. An additional grant | ||
| * of patent rights can be found in the PATENTS file in the same directory. | ||
| * | ||
| * @providesModule ReactDOMNodeStreamRenderer |
There was a problem hiding this comment.
This file should live in the DOM folder. The only reason the other one wasn't is that we expect that most of the logic can be extracted as separate renderer and then move some parts out to DOM.
There was a problem hiding this comment.
Fixed in 3d1421a. I assumed that the same logic applied to ReactDOMStringRenderer.js as well, since the file is parallel in structure to this one.
Thanks!
| @@ -6,7 +6,7 @@ | |||
| * LICENSE file in the root directory of this source tree. An additional grant | |||
| * of patent rights can be found in the PATENTS file in the same directory. | |||
| * | |||
| * @providesModule ReactServerRenderer | |||
| * @providesModule ReactDOMPartialRenderer | |||
There was a problem hiding this comment.
Let's call this just "ReactPartialRenderer" for reasons mentioned above.
There was a problem hiding this comment.
Good call. Done in 3d1421a. Thanks!
ReactDOMNodeStream
|
This PR builds on @tomocchino & @sebmarkbage's work on the server renderer in React 16, exposing the server renderer as a Node.js Readable Stream. This allows folks to pipe a renderer out to the network, and it's a fairly common request of folks using SSR.
The actual code part of it is pretty small, as the existing
read()method of the server renderer in React 16 is already able to do partial/async rendering; all we needed to do was wrap thatread()method in a Readable stream, which is pretty much all in src/renderers/shared/server/ReactDOMNodeStreamRenderer.js. The rest of the PR is code organization, build, test, and documentation changes.I'm not entirely comfortable with the build changes, especially the changes in bundles.js, as I don't really know what things like
fbEntryandhasteNamemean. Names in general were a bit sticky, and I settled onReactDOMNodeStream/react-dom/node-streamfor the new object, mindful that it's possible that a renderer for web streams might be created in the future. I have some other ideas about naming after talking with @tomocchino, but I'll save those for another PR.Thanks so much for all the work y'all do!
ETA: I should also give props to @spicyj, who I believe wrote an earlier version of the server renderer on top of which Tom and Sebastian built their work. Thanks, @spicyj!