[Fizz] implement renderIntoContainer
#25703
Conversation
gnoff
force-pushed
the
fizz-into-container
branch
from
4d43398 to
8ed0dfb
Compare
|
Is this going to enable the ability to stream a React application that is responsible for rendering function App() {
return (
<Suspense>
<html>
<head></head>
<body></body>
</html>
</Suspense>
)
} |
|
@tannerlinsley I believe it's the other way around — this adds support for not rendering the entire document with React. |
|
That's right. Right now The point of this new API is to stream into some DOM element that was sent via other means like a static html file. If you send the HTML first and then send the stream response this api will relocate the streamed in content into a dom node that has the ID you pass in. |
gnoff
force-pushed
the
fizz-into-container
branch
4 times, most recently
from
766245c to
a673ba5
Compare
| if (isContainer) { | ||
| const containerNode = document.getElementById(suspenseBoundaryID); | ||
| if (containerNode) { | ||
| containerNode['_r'] = allInsertions; | ||
| } | ||
| } |
There was a problem hiding this comment.
This is how we "block" hydration until the boundary can switch the content in. There are still an unhandled edge cases though
For fizz runtime the script loads async so it's possible the bootstrap scripts execute first. This means a hydrateRoot call might start to compare HTML before the actual content is swapped in. I don't really have a good idea atm how to defend against this. It may require coordination with the container itself (an attribute that is removed when hydration is unblocked)
cc @sebmarkbage
packages/react-dom-bindings/src/server/fizz-instruction-set/ReactDOMFizzInstructionSet.js
Outdated
Show resolved
Hide resolved
gnoff
requested review from
acdlite and
sebmarkbage
and removed request for
acdlite
gnoff
force-pushed
the
fizz-into-container
branch
from
6fa5d40 to
80ad0df
Compare
| @@ -133,6 +139,86 @@ async function replaceScriptsAndMove( | |||
| } | |||
| } | |||
|
|
|||
| export function installScriptObserver(window: any, document: Document) { | |||
There was a problem hiding this comment.
@mofeiZ I was having trouble getting the latest technique of delaying bootstrap scripts to work with the current test setup for moving scripts. The problem is that in an async tick a script is added to the body but there is no visibility into this event for us to use the trick we use in act.
I implemented an entirely different approach where we use a mutation observer and look for unexecuted scripts on each new node addition. I believe this approach can work for all prior and new use cases but I may not understand well why mutation observer wasn't used in the first place so I would love a review here especially to see if I may need to make changes.
I took this opportunity to also make act better. It now infers the insertion point based on what is being streamed in so we don't need actIntoEntireDocument. I renamed the original act to actIntoContainer. it's not representative of 99% of how streaming will be done so I made it not the default name and we should probably move away from using it
| const scriptObserverFunctionString = ` | ||
| const scriptObserver = new MutationObserver(mutations => { | ||
| let promises = []; | ||
| for (var i = 0; i < mutations.length; i++) { | ||
| const addedNodes = mutations[i].addedNodes; | ||
| if (addedNodes.length) { | ||
| var pendingWork = window.__loadScripts(window); | ||
| window.__pendingWork = (window.__pendingWork || Promise.resolve()).then(() => pendingWork); | ||
| // We can stop here because loadScripts loads scripts globally so there | ||
| // is not additional benefit to calling it for each mutation that added nodes | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| window.__scriptObserver = scriptObserver; | ||
| `; |
There was a problem hiding this comment.
The reason this is a string and is eval'd is because the MutationObserver does not think the Node's passed in are actually Nodes. This is because window.Node is not in the prototype chain in the context of this file since we are using an adhoc jsdom window not the actual global. @mofeiZ if you can think of way to do this so we don't need to have this code as a string I'm open to suggestions :)
There was a problem hiding this comment.
@gnoff I think JSDOM window provides a mocked MutationObserver. Fizz tests still pass when I remove eval(...) and call new window.MutationObserver(...)
function makeScriptObserver(window: JSDOM.DOMWindow) {
return window.__scriptObserver = new window.MutationObserver(...);
}
gnoff
force-pushed
the
fizz-into-container
branch
from
58941bc to
7c39cd5
Compare
|
@sebmarkbage worked out the issues with external fizz runtime. this PR is ready for review now |
gnoff
force-pushed
the
fizz-into-container
branch
from
5ac3ef8 to
688be00
Compare
gnoff
force-pushed
the
fizz-into-container
branch
2 times, most recently
from
986dbbd to
95c9a73
Compare
gnoff
changed the title
renderIntoContainer
gnoff
force-pushed
the
fizz-into-container
branch
from
1435087 to
3af7e54
Compare
gnoff
force-pushed
the
fizz-into-container
branch
from
02ceffe to
4391557
Compare
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.
gnoff
force-pushed
the
fizz-into-container
branch
from
4391557 to
8be28e5
Compare
| node.remove(); | ||
| switch (dataset.ri) { |
There was a problem hiding this comment.
Originally the idea was to encode the instruction in the name of the first argument. That's why it was one argument per instruction since it's fewer bytes to do data-rxi="arg0" than "data-ri="x" data-bid="arg0". We should do that instead.
I'm not sure why the instruction attribute wasn't used for arguments previously though.
There was a problem hiding this comment.
The escaping of the name like dataset['ri'] is good practice because we should ideally be able to use closure compiler advanced mode and in that case these properties would otherwise be minified. So we've been trying to stick to that as much as possible.
There was a problem hiding this comment.
sounds good, made them more compact
| @@ -108,5 +108,7 @@ export const enableCustomElementPropertySupport = __EXPERIMENTAL__; | |||
| export const useModernStrictMode = false; | |||
| export const enableFizzExternalRuntime = true; | |||
|
|
|||
| export const enableFizzIntoContainer = __EXPERIMENTAL__; | |||
There was a problem hiding this comment.
You can just turn it on here and they'll use it if they want. Experimental here is more about classic vs modern which is a different dimension where we mostly turn things off experimentally.
| @@ -2349,7 +2378,6 @@ function flushCompletedQueues( | |||
|
|
|||
| // Next we emit any segments of any boundaries that are partially complete | |||
| // but not deeply complete. | |||
| const partialBoundaries = request.partialBoundaries; | |||
There was a problem hiding this comment.
The idea of these is that we should be able to mutate these as we flush if we discover things, decide to outline them, or in the case of partial boundaries, if we flush them as completed boundaries. Even if we mutate the set, we still have the same one here, but maybe it's better to read it.
We've also discovered that we do this pattern a lot which requires more on the stack and worse reuse of registers. So probably shouldn't do it here.
| @@ -36,7 +37,14 @@ function createAbortHandler(request: Request, reason: string) { | |||
| return () => abort(request, new Error(reason)); | |||
| } | |||
|
|
|||
| type Options = { | |||
There was a problem hiding this comment.
The name of a type is semi-public. Since even if we don't export them, people like @eps1lon have to look at them and give them some name that ends up being part of the export. Therefore changing the name of a type can be a breaking change.
In this case he had to pick some other names:
It's arguably too late to change the convention so seems like we should align on those names and going forward really think through what names we give to these and probably explicitly export the type to make that clear.
There was a problem hiding this comment.
I usually try to stick to ${ucfirst(methodName)}Options for naming. Gets a bit more tricky when we have shared options but overall I like that the verbose name requires less context (see "? Options" or "? toReadableOptions")
There was a problem hiding this comment.
Since we're export flow types and not TS types does a name change here actually ever break anything? I guess for flow users it could.
Should I just export the options now following the existing convention?
export type RenderToReadableStreamOptions = ...
export type RenderIntoContainerOptions = ...
There was a problem hiding this comment.
It seems like the Options convention in JS is just a hack around named arguments anyway and it doesn't actually have its own nominal structure. So it makes sense that each function gets their own. As we've seen, they can also change independently.
That convention shouldn't necessarily carry over to any other type than "Options" though.
There was a problem hiding this comment.
This makes sense to me
| @@ -305,7 +306,17 @@ export function createRequest( | |||
| onShellError: onShellError === undefined ? noop : onShellError, | |||
| onFatalError: onFatalError === undefined ? noop : onFatalError, | |||
| }; | |||
| // This segment represents the root fallback. | |||
| const rootBoundaryID = getRootBoundaryID(responseState); | |||
There was a problem hiding this comment.
responseState isn't really supposed to be where all configuration lives. It represents things that are stateful. We kind of abused it to also store cached things (converted to chunks).
In this case you don't really need to create a response state only to read back the value from it immediately. You can just pass in a SuspenseBoundaryID as the argument to createRequest here to initialize the root boundary.
| let maybeRootBoundary = null; | ||
| if (rootBoundaryID !== UNINITIALIZED_SUSPENSE_BOUNDARY_ID) { | ||
| const fallbackAbortSet: Set<Task> = new Set(); | ||
| const rootBoundary = createSuspenseBoundary(request, fallbackAbortSet); |
There was a problem hiding this comment.
This part makes me a bit uncomfortable because it's not a Suspense boundary on the client although it kind of acts like one. We're going to be splitting Suspense boundaries into the Offscreen boundaries for the server and changing how to annotations work and how they have to line up.
There was a problem hiding this comment.
I don't think that really matters for this implementation. the fact that there is a suspense boundary here only affects the flushing semantics. We never stream a fallback and there is no boundary marker comments emitted so from a client perspective the fact that we use a boundary here is an unobservable implementation detail
Yeah, it's a little tricky. The bootstrap goes first when we are completing the root boundary with styles because we need to ensure the scripts are present before the instruction tries to relocate them. IF we did it after then it's possible we'd miss some bootstrap chunks. But in cases where you aren't using a root boundary or when the root boundary does not depend on styles and can do the swap synchronously we write the bootstrap after the instruction and don't put it in a temporary template. I could fork the entire |
…in instruction writing
If I understand you correctly we want the As for getting it to inline, I tried making a separate When I look at closure documentation it seems to suggest that inlining is used during advanced compilation, but I know that isn't true entirely b/c we have inlining in many places today and we're on simple optimizations |
gnoff
force-pushed
the
fizz-into-container
branch
3 times, most recently
from
fb927b3 to
e4fccad
Compare
gnoff
force-pushed
the
fizz-into-container
branch
from
e4fccad to
8e5cd60
Compare
There was a problem hiding this comment.
It took a while to wrap my head around Fizz Server, but this solution makes a lot of sense. Thanks for the comments and explanations here, super useful!
Is this implementation subject to change (I see quite a few other Float features in development) or mostly stable?
(following feedback is regarding style / docs)
It took a while for me to understand fizz server code and reason about these changes (although it could definitely be just me being slow). Request feels like a big ball of mutable state whose semantics are described mostly by imperative code. In the absence of the Request type encoding its invariants or having nested object types, could we add docblocks (comments + typedefs) to document this?
Some examples:
- mutually exclusive fields
- fields whose values encode rendering state --
completedRootSegmentis only set once, when the first task (the enqueued root segment) completes.nullindicates either that the rootSegment errored, or that we have already flushed - fields that mutate with / depend on another field. E.g.
completedRootSegmentandpendingRootTasks
I understand that maintaining the correctness of comments can be a chore (code is self documenting), but just wanted to bring up for discussion - happy to take a stab at writing this if we do want to add.
| const scriptObserverFunctionString = ` | ||
| const scriptObserver = new MutationObserver(mutations => { | ||
| let promises = []; | ||
| for (var i = 0; i < mutations.length; i++) { | ||
| const addedNodes = mutations[i].addedNodes; | ||
| if (addedNodes.length) { | ||
| var pendingWork = window.__loadScripts(window); | ||
| window.__pendingWork = (window.__pendingWork || Promise.resolve()).then(() => pendingWork); | ||
| // We can stop here because loadScripts loads scripts globally so there | ||
| // is not additional benefit to calling it for each mutation that added nodes | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| window.__scriptObserver = scriptObserver; | ||
| `; |
There was a problem hiding this comment.
@gnoff I think JSDOM window provides a mocked MutationObserver. Fizz tests still pass when I remove eval(...) and call new window.MutationObserver(...)
function makeScriptObserver(window: JSDOM.DOMWindow) {
return window.__scriptObserver = new window.MutationObserver(...);
}| clientRenderBoundary( | ||
| dataset['bid'], | ||
|
|
||
| if (dataset['ix'] != null) { |
There was a problem hiding this comment.
Question - would we expect / handle any of these instruction functions to throw here? I noticed a try-catch in completeContainer, but couldn't find what might throw in that function. Other instruction functions don't look like they would throw either
We should add a try-catch here if other functions can potentially throw (and React is expected to keep rendering). Inline scripts isolate failures, but the external runtime may skip processing further instructions / MutationRecords if one errors.
There was a problem hiding this comment.
I think I ended up removing the throw but not the wrapping try catch. Good call on handling it here though, we probably should not actually throw and just return from completeContainer
| // streaming into a container node when a browser would not do that except in very contrived | ||
| // circumstances. We should migrate to the new act which infers the streaming location as | ||
| // either the document, documentElement, or document.body as appropriate | ||
| async function actIntoContainer(callback) { |
|
Another question (not entirely related to We current use JSON script tags to describe client-side callbacks / events. These are read by a MutationObserver on the client, which resolves + invokes the actual callback. This allows us to fulfill Binary Transparency requirements (i.e. no inline scripts execution). To use I'm still new to this codebase though, so I can definitely be missing something. @gnoff @sebmarkbage |
| writeChunk(destination, completeBoundaryScript3a); | ||
| // boundaryResources encodes an array literal | ||
| if (enableFloat && hasStyleDependencies) { | ||
| // We emit the bootstrap chunks into a template container with an id |
There was a problem hiding this comment.
This is a clever solution. Curious - was the decision to emit bootstrap container as bs:[sid] to support multiple React SSR roots within a page?
There was a problem hiding this comment.
It does potentially enable that however it's unclear how there would be more than one root streaming into the same document at the same time. I'm sure someone could hack it to make that happen but its not something I was trying to explicitly support. I used the sid primarily to add some entropy to the id to avoid colliding with someone else's bs. the sid has the identifier prefix so in theory you can customize it not to collide if you really needed to use bs somewhere else
I'm hesitant to expand the bootstrap scripts description to allow for arbitrary inline scripts, also at the moment we only allow 1 inline script so it would be either executable or not based on the type but you couldn't have both. I get that this isn't really a problem for Meta given the BT constraints but it doesn't feel like the actual best API, just one that happens to be good enough for the use case for today Can you share more about the timing of these scripts. Is it important that their insertion happen in conjunction with the bootstrap phase? If they could go early and then be triggered by the bootstrap then you could just render them as part of the (implied) Root Boundary. They would be in the DOM before the bootstrap and then the bootstrap would kick off the whatever instruction processor needs to run. What sort of timing constraints exist? |
Implements
renderIntoContainerandrenderIntoContainerAsPipeableStreamStreaming rendering similar to
renderToReadableStreamandrenderToPipeableStreambut will hoist root most content into a container element in the DOM. This is useful for people who want to take advantage of React's streaming rendering but have an existing system which produces the "Shell" of the app.This will produce
These new methods don't have a Shell. Instead the topmost children are treated like they are wrapped in a Suspense boundary. the content will be inserted into the container on the client when there are no more pending tasks for this Root Boundary. Stylesheets get emitted as part of the boundary completion just like normal Suspsense boundaries. Other resources will get streamed in as early as possible but not relocated so if you use preloads or prinits of scripts they will likely show up in the body and stay there.
Certain options don't make sense for these methods. There is no
onShellReadyoronShellErrorbecause there is no shell.In the ReadableStream case you get your stream synchronously rather than having to wait for a promsie to resolve when the shell is ready.