Introduce renderToNodeStream #93
Conversation
|
Hmm - it seems like in order to support backpressure, we'd need to render directly into the stream rather than rendering to a String and converting after. Thoughts? |
|
I'm not 100% familiar with backpressure in node. From what I understand we would need to create a readable stream that pauses rendering until I'll definitely look into this when I find some time and if nobody does so before I do :) |
| [ | ||
| "@babel/env", | ||
| { | ||
| "exclude": [ |
There was a problem hiding this comment.
should probably set the target to the minimum node version here
There was a problem hiding this comment.
Well renderToNodeStream requires node 10. The other parts of this package work fine with 9 and more importantly in the browser
There was a problem hiding this comment.
just a bit concerned about the async generator
There was a problem hiding this comment.
I get that point. The renderToNodeStream should only be used in node anyways, thus an async generator feels good to me :)
I'd propose to disallow async generators in eslint as bundling regenerator would result in a massive bundle bloat. That will make sure we don't use it in modules targeted to browsers
|
This is great and the backpressure support is on point. One question: does the async support here work with the Suspense API? It seems like it's handling the throwing of a Promise, but it would be doing so quite differently than if there was a |
Co-Authored-By: Jason Miller <[email protected]>
|
Can you elaborate a bit on the concerns of the " One thing that is currently not too nice is that the rendering is blocked until one promise resolves but I can't come up with a simple solution to that one. |
| const VOID_ELEMENTS = /^(area|base|br|col|embed|hr|img|input|link|meta|param|source|track|wbr)$/; | ||
|
|
||
|
|
||
| function PreactReadableStream(vnode, context, opts) { |
There was a problem hiding this comment.
I think it's safe to use a class here. The current node LTS version supports them natively.
There was a problem hiding this comment.
Just wanted to say that the PR is really cool! You made the right call to wait for generator support in microbundle imo.
One thing I'm pondering about is if we should extract the shared code from the sync version and this one. It would be a tradeoff between perf and ease of maintenance for sure. Personally I'd favour the latter because I'm worried that any future bugs will only be fixed in one place and not the other. We need some more opinions on this. What do you all think?
| // the context value as `this.context` just for this component. | ||
| let cxType = nodeName.contextType; | ||
| let provider = cxType && context[cxType.__c]; | ||
| let cctx = cxType != null ? (provider ? provider.props.value : cxType._defaultValue) : context; |
There was a problem hiding this comment.
Hello! Looks like the use of _defaultValue might be a bug (#130); changing it to __ should fix it.
|
Hey, guys |
|
Any progress here? |
| import stream from 'stream'; | ||
|
|
||
| // components without names, kept as a hash for later comparison to return consistent UnnamedComponentXX names. | ||
| const UNNAMED = []; |
There was a problem hiding this comment.
Note to self: Seems like a memory leak when keeping references to components around in a global variable
|
everybody already wants to use it in production! how long? |
|
Superseded by #296 |
This PR aims to introduce
renderToNodeStream.renderToNodeStreamis especially useful when used together withSuspense. This will allow SSR to send HTML to the client as soon as possible.TODO:
ReactDOMServer.renderToNodeStream