Skip to content
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

[Beta]: renderToPipeableStream, but just for a part of the page #5663

Closed
azangru opened this issue Mar 9, 2023 · 8 comments
Closed

[Beta]: renderToPipeableStream, but just for a part of the page #5663

azangru opened this issue Mar 9, 2023 · 8 comments

Comments

@azangru
Copy link

azangru commented Mar 9, 2023

Background

The new React docs say, in the description of the renderToPipeableStream api:

Your root component should return the entire document including the root tag.

I.e. the intention seems to be that React control the full DOM tree, starting from the html element. However, doing so opens the React app to various hydration problems caused by third-party scripts and browser extensions (see e.g. this report from a Sentry developer, or this report by me).

As a response to this problem, some developers seem to be combining the new renderToPipeableStream api with a more traditional approach of rendering the React app into a dedicated DOM element inside the html body. Here is an example I found in the Remix community. The relevant part in that example appears as follows:

const callbackName = isbot(request.headers.get("user-agent"))
    ? "onAllReady"
    : "onShellReady";

/* ... */

const { pipe, abort } = renderToPipeableStream(
  <RemixServer context={remixContext} url={request.url} />,
  {
    [callbackName]: () => {
      const body = new PassThrough();

      responseHeaders.set("Content-Type", "text/html");

      resolve(
        new Response(body, {
          headers: responseHeaders,
          status: didError ? 500 : responseStatusCode,
        })
      );
      const html = `<!DOCTYPE html><html><head><!--start head-->${head}<!--end head--></head><body><div id="root">`;
      body.write(html);
      pipe(body);
      body.write(`</div></body></html>`);
    },
    onShellError(err: unknown) {
      reject(err);
    },
    onError(error: unknown) {
      didError = true;

      console.error(error);
    },
  }
);

Questions

  • Is there anything about the renderToPipeableStream api that requires the rendered React node to be the html element with all its children? If not, do you think it would be worth updating the docs to reflect that?
  • In the code example above, I am worried about this part:
body.write(html);
pipe(body);
body.write(`</div></body></html>`);

Looking at the source code of renderToPipeableStream, I cannot tell whether there is anything that guarantees in the snippet above that the second body.write will happen strictly after the pipe(body) has finished; but then, I am not very well versed in node streams.

Could you please let me know if you have any concerns about the snippet above? If there indeed is a timing problem with the streams, then shouldn't the pipe on the renderToPipeableStream have some way of notifying the consumer that it has finished piping? If, on the other hand, everything is all right with the snippet above, then perhaps this approach merits discussion in the docs?

@gaearon
Copy link
Member

gaearon commented Mar 9, 2023

I do not think that this is a supported way to use renderToPipeableStream. I'm not 100% sure what exactly would break here though. (I'll ask around)

@azangru
Copy link
Author

azangru commented Mar 9, 2023

If not renderToPipeableStream, is there a server-side rendering api that would be more suitable for this use case? The docs say renderToString won't support suspense with lazy loading, which only seems to leave renderToPipeableStream?

@gaearon
Copy link
Member

gaearon commented Mar 9, 2023

What exactly is the use case? Is it rendering into a part of a document (because it’s owned by some other part of your stack), or is it fixing the hydration issues? One isn’t supposed to be solution for the other — I think these are different things. For the hydration issues, we’ve given some detailed responses in the thread so I wonder if there’s something missing there.

@gnoff
Copy link
Collaborator

gnoff commented Mar 9, 2023

@azangru The technique you described will work somewhat by accident. I would not reccomend it since React doesn't really guarantee that it will continue to work and you may find upgrading to newer versions difficult if you end up being unable to unwind this approach.

body.write(html);
pipe(body);
body.write(`</div></body></html>`);

in this case the initial content (everything non suspended) will render inside the <div> as expected but later content that is streamed in will come after the closing html tag. Browsers can actually handle this just fine, they will insert the late DOM nodes at the end of the body and react's streaming runtime will patch them into the right place in the document.

However in the future React will start to hoist certain tags into the head or body but if you render the stream into some deep tag the hoisted content will go there instead. Things again might just work but it will be hard to guarantee.

We are considering a new render function that can do this streaming into a specific dom node. It might look like renderIntoContainer(<App />, "id-of-the-container") and then on the client ReactDOM.hydrateRoot(document.getElementById("id-of-the-container", <App />). In this mode the stream should be piped in after whatever static html content you want to send but React will know how to place the primary content in the right spot which in your example above is happening but just by accident of our implementation and how browsers parse late HTML chunks

@gnoff
Copy link
Collaborator

gnoff commented Mar 9, 2023

I will add that our hydration tolerance should be improving dramatically in the next non-patch release of React. I would try it out once it is realeased and see if you can go back to rendering the entire document again

@azangru
Copy link
Author

azangru commented Mar 9, 2023

For the hydration issues, we’ve given some detailed responses in the thread so I wonder if there’s something missing there.

Apologies; I've just discovered and have been reading through facebook/react#24430, where the React team makes its position on hydration clear.

For me, it was the hydration issue that was driving me up the wall; so I will be looking forward to the improvements expected in the next non-patch release of React; but regardless of that, one might reasonably consider the need for a general strategy for rendering React apps into a part of the document.

@gaearon
Copy link
Member

gaearon commented Mar 9, 2023

Sounds like we can close this one? Or is there you'd like to see clarified in the doc?

@azangru
Copy link
Author

azangru commented Mar 9, 2023

Yes, please do. Thank you both for the clarification and for the heads-up about the new upcoming rendering api 🙇

@azangru azangru closed this as completed Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants