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

[Fizz] Implement renderDocument #25970

Closed
wants to merge 11 commits into from
Closed

Conversation

gnoff
Copy link
Collaborator

@gnoff gnoff commented Jan 7, 2023

stacked on #25703

renderDocument and renderDocumentAsPipeableStream

This PR adds a new rendering mode for rendering an entire Document on the server.

Example

On the server use renderDocument and stream the results to the client

// ReadableStream gets passed to Response or otherwise piped to browser
const stream = renderDocument(
  // children
  <html lang="en">
    <body>
      <div>my primary app</div>
    </body>
  </html>,
  // fallback
  <div>a fallback app</div>
);

Client receives if the Shell does not error for the primary children

<!DOCTYPE html>
<html lang="en">
  <head></head>
  <body>
    <div>my primary app</div>
  </body>
</html>

Client receives the following if the Shell does error for the primary children

<!DOCTYPE html>
<html lang="en">
  <head></head>
  <body>
    <div>a fallback app</div>
  </body>
</html>

Primary Characteristics

There are three primary distinguishing characteristics with the renderDocument implementations that explain why existing functions such as renderToReadableStream were not suitable

  1. It always emits a full HTML document even if you only render some body content. With other render methods you only get what you tell React to render for you so if you render a div with some inner content that is all React will output. Using one of the existing render functions requires you to carefully control what is rendered and if you have intermediate layers (like a framework) in place it can force you into a situation where the App author needs to remember to include the document structure (<html>, <head>, etc...) or not be allowed to provide it at all. The flexibility of embedding whatever renderDocument is given in a full HTML document alleviates this limitation
  2. It will attempt to stream preamble content as early as possible, even before the shell has finished rendering. If we discover a stylesheet or script that can be preloaded or preinitialized while still waiting of the shell to complete rendering we want to send that to the client right away. renderToReadableStream does not provide you with the stream until the shell has finished.
  3. It provides allows you to provide a fallback that will render if the Shell for children errors. While other methods don't flush anything until the Shell finishes and thus you can spin up a separate render call, or handle the error case with a different status code and response content the renderDocument function likely already sent bytes before the Shell has errored so it must coordinate the fallback state more holistically.

Resource Flushing Semantics

Some Resources can flush even before the Shell is finished, including preloads, and scripts. Stylesheets for the first precedence can be flushed as well and we can send preloads for stylesheeets for other precedences. The reason we cannot flush all stylesheets of any precedence early is we need the order to be guaranteed and we don't do any re-ordering on the client so they need to leave the server in the right order unless they are delivered as part of a Suspense Boundary reveal.

Ancillary PR note

A related incidental change accompanies this PR for the other render methods. If we render an <html> tag but do not render a <head> tag, we will emit an <head> tag into the stream just after <html> and before any other rendered content (usually a <body> but you can have non-spec html). It is possible this tag won't be empty because any Resources that get hoisted to the head will emit there. Currently these tags emit after the html tag and before any rendered content which may work based on permissive Browser HTML parsing rules but is not valid HTML. One might try to argue this is a breaking change but relying on there NOT being a <head> is not practically possible given the Browser invents a DOM node for this head anyway

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jan 7, 2023
@gnoff gnoff marked this pull request as draft January 7, 2023 01:04
@gnoff gnoff force-pushed the fizz-into-document branch 2 times, most recently from 6309c30 to 74de63c Compare January 10, 2023 22:49
@gnoff gnoff marked this pull request as ready for review January 11, 2023 02:27
@gnoff gnoff force-pushed the fizz-into-document branch 4 times, most recently from 5abeef7 to 308599c Compare January 12, 2023 21:29
eps1lon added a commit that referenced this pull request Jan 15, 2023
## Summary

Should unblock #25970
If the callback for `toWarnDev` was `async` and threw, we didn't
ultimately reject the await Promise from the matcher. This resulted in
tests failing even though the failure was expected due to a test gate.

## How did you test this change?

- [x] tested in #25970 with `yarn
test --r=stable --env=development
packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js --watch`
- [x] `yarn test`
- [x] CI
github-actions bot pushed a commit that referenced this pull request Jan 15, 2023
## Summary

Should unblock #25970
If the callback for `toWarnDev` was `async` and threw, we didn't
ultimately reject the await Promise from the matcher. This resulted in
tests failing even though the failure was expected due to a test gate.

## How did you test this change?

- [x] tested in #25970 with `yarn
test --r=stable --env=development
packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js --watch`
- [x] `yarn test`
- [x] CI

DiffTrain build for [4f8ffec](4f8ffec)
[View git log for this commit](https://github.com/facebook/react/commits/4f8ffec453c41fcb6e98cb4e003f7319bb1c81b9)
renderIntoContainer allows you to stream react into a specific DOM node identified by a element ID.

It returns a ReadableStream but differs from renderToReadableStream in that you get access to the stream syncronously.

Since the top level Component gets inserted like a boundary I am calling what would normally be the "Shell" of a React app the Root Boundary when using renderIntoContainer. While the top level Components stream in like a boundary there is not actually a Suspense Boundary wrapper. However it will act like one in that stylesheets will be loaded before the content is revealed in the container Node. If stylehseet loading fails hydration will fall back to client rendering

It allows the passing of fallback bootstrap scripts which are used if the Root Boundary errors.
@sebmarkbage
Copy link
Collaborator

Nit: I get that renderIntoDocument is supposed to mean that document is the container argument but it doesn't make sense unless you also consider the hydrateRoot API so it's a bit weird. It also doesn't provide a difference from renderIntoContainer. Like what if the container is the document?

If you just consider the SSR document it's more like you render the "whole" document.

renderDocument or renderWholeDocument might make the distinction clearer.

@@ -109,6 +109,7 @@ export const useModernStrictMode = false;
export const enableFizzExternalRuntime = true;

export const enableFizzIntoContainer = __EXPERIMENTAL__;
export const enableFizzIntoDocument = __EXPERIMENTAL__;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the other one, might as well turn this one on since its use is optional.

@@ -87,6 +87,7 @@ export const useModernStrictMode = false;
export const enableFizzExternalRuntime = false;

export const enableFizzIntoContainer = false;
export const enableFizzIntoDocument = false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turn the flag on in anything that's not expected to use it.

@@ -123,6 +123,9 @@ const DataStreamingFormat: StreamingFormat = 1;
export type ResponseState = {
bootstrapChunks: Array<Chunk | PrecomputedChunk>,
fallbackBootstrapChunks: void | Array<Chunk | PrecomputedChunk>,
requiresEmbedding: boolean,
hasHead: boolean,
hasHtml: boolean,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes V8 will deopt objects with more than 16 properties or something like that. It's probably time to consolidate these to a bitmask.

Same thing with sentXFunction can probably just be a bitmask of which ones have been sent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've force pushed a number of updates since this was in the commits. I think I sent you a commit range but that has been outdated for a while. I worry you probably reviewed a quite outdated version of the implemenation. For this bit of feedback in particular I already updated to a bitmask. I could consolidate further since I'm not using very many of the bits

@@ -2301,18 +2305,15 @@ function flushCompletedQueues(
if (request.pendingRootTasks === 0) {
if (enableFloat) {
const preamble = request.preamble;
for (i = 0; i < preamble.length; i++) {
// we expect the preamble to be tiny and will ignore backpressure
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is still relevant.

@@ -2301,18 +2305,15 @@ function flushCompletedQueues(
if (request.pendingRootTasks === 0) {
if (enableFloat) {
const preamble = request.preamble;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably time to rethink the concept of request.preamble. It was an elegant way to solve this problem before, but now it's way more complicated so it's not elegant and just unnecessary. The information is encoded on the responseState anyway.

let includedAttributeProps = false;

if (!responseState.hasHead) {
responseState.hasHead = true;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<head> should not allow any attributes in this mode. It's too flaky in case we flush the preamble before it's discovered so it would have different semantics in different cases depending on data. So it should never have attributes to avoid you accidentally relying on it.

In other words, this can all be much more simplified because in this mode, you never write head when pushing.

You can always write head when you're writing the HTML tag. Whether it's implicit or not.

That means that the whole thing gets much more simplified because the whole preamble is just a boolean. Whether you have html+head or not.

This commit adds the function renderIntoDocument in react-dom/server and adds the ability to embed the rendered children in the necessary html tags to repereset a full document. this means you can render "<html>...</html>" or "<div>...</div>" and either way the render will emit html, head, and body tags as necessary to describe a valid and complete HTML page.

Like renderIntoContainer, renderIntoDocument provides a stream immediately. While there is a shell of sorts this fucntion will start writing content from the preamble (html and head tags, plus resources that flush in the head) before finishing the shell.

Additionally renderIntoContainer accepts fallback children and fallback bootstrap script options. If the Shell errors the  fallback children will render instead of children. The expectation is that the client will attempt to render fresh on the client.
@gnoff
Copy link
Collaborator Author

gnoff commented Jan 18, 2023

Nit: I get that renderIntoDocument is supposed to mean that document is the container argument but it doesn't make sense unless you also consider the hydrateRoot API so it's a bit weird. It also doesn't provide a difference from renderIntoContainer. Like what if the container is the document?

If you just consider the SSR document it's more like you render the "whole" document.

renderDocument or renderWholeDocument might make the distinction clearer.

I updated to renderDocument. rationale makes sense

@gnoff gnoff changed the title [Fizz] Implement renderIntoDocument [Fizz] Implement renderDocument Jan 18, 2023
@gnoff
Copy link
Collaborator Author

gnoff commented Dec 7, 2023

With the implementation of onHeaders this new API is less necessary since we can do some preloading of resources even if we don't have a shell. There is still work to do to optimize flushing of the head early and some evolution of the promise API for render but going to close this PR and pursue that in other forms

@gnoff gnoff closed this Dec 7, 2023
jerrydev0927 added a commit to jerrydev0927/react that referenced this pull request Jan 5, 2024
## Summary

Should unblock facebook/react#25970
If the callback for `toWarnDev` was `async` and threw, we didn't
ultimately reject the await Promise from the matcher. This resulted in
tests failing even though the failure was expected due to a test gate.

## How did you test this change?

- [x] tested in facebook/react#25970 with `yarn
test --r=stable --env=development
packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js --watch`
- [x] `yarn test`
- [x] CI

DiffTrain build for [4f8ffec453c41fcb6e98cb4e003f7319bb1c81b9](facebook/react@4f8ffec)
[View git log for this commit](https://github.com/facebook/react/commits/4f8ffec453c41fcb6e98cb4e003f7319bb1c81b9)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants