Add support for renderToNodeStream

[bohdanbirdie]

Please, consider checking my solution for renderToNodeStream. I was not sure what tests should be written.

@davnicwil

[davnicwil]

Hi @bohdanbirdie thanks for this.

I would prefer not to introduce a separate function for this, rather just have a (default false) option passed to the existing frontloadServerRender to indicate that render outputs a NodeStream.

Also seems like the existing fronloadServerRender has just been copied and pasted and the part that deals with reading the stream on first render is the only thing that's different? I don't think we need to duplicate this code - just consume the stream after first render if the option is set true I guess.

In terms of tests, check out the existing tests for server render - line 394 of index.test.js - tests for this should be almost the same.

[bohdanbirdie]

@davnicwil alright how about checking if this is a NodeStream under the hood? Like checking for instanceof ReactMarkupReadableStream or checking if .on property if available?

[bohdanbirdie]

But actually, checking for instanceof will force us to import it from the react-dom/server so maybe passing extra argument is the best approach here.

[davnicwil]

Actually yeah, good idea, auto detection would be better. Maybe could do it the other way around, test if the output of render is a string with typeof output === 'string'?

If true then we can assume renderToString was called, else renderToNodeStream was.

[bohdanbirdie]

Deal, I'll update it soon :)

[bohdanbirdie]

Seems like checking for typeof output === "string"won't work for those render() method from enzyme, there is no way to render to node stream with it as far as I understood. I stuck to

let firstRenderResult = render(true)
  if (!firstRenderResult.on) {

@davnicwil

[davnicwil]

Thanks again for the update.

So couple of things here

The server render tests will need to actually test the real server render methods from react as a result of this change. Right now they use enzyme for convenience because it didn't matter what was in the server render method but after your change it does matter, so it needs to test the real implementation using renderToNodeStream and renderToString from react-dom, and not use enzyme at all.

The second thing is that the changes here still contain all the duplication I was talking about above - I think all you have to do is change a few lines after line 222 (output = render(true)) to check what the output type is and then just consume the stream if it's a stream. There is no need to duplicate all the other code for both paths (that I can see)

[bohdanbirdie]

Good to know. Ok, I'll update it :)

[davnicwil]

@bohdanbirdie some significant changes have happened since this PR was raised, most notably the new feature for arbitrary nesting on server render. This will have to added on top of that, so I'm going to close for now as I think it'll be easier to do from scratch in a new PR. This would be a great feature, thanks for the idea and if you do have the time I welcome a future PR for this 😄

[bohdanbirdie]

@davnicwil cool Sorry, I got really busy since that time and didn't have a chance to finish the PR

Good luck!