[ServerRenderer] Add option to send instructions as data attributes #25437
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
| const node = fakeBody.firstChild; | ||
| if ( | ||
| node.nodeName === 'SCRIPT' && | ||
| (node.nodeName === 'SCRIPT' || node.nodeName === 'script') && |
There was a problem hiding this comment.
Why is the lower case needed? These always get uppercased by the DOM, right?
| fakeBody.removeChild(node); | ||
| parent.appendChild(script); | ||
| } else { | ||
| replaceScripts(node); |
There was a problem hiding this comment.
What's this doing? It looks like it's just doing the same thing as above but recursively?
We can kind of assume that the script tags are always inserted in the body since that what we do.
| (CSPnonce === null || node.getAttribute('nonce') === CSPnonce) | ||
| ) { | ||
| const script = document.createElement('script'); | ||
| const script = document.createElement('SCRIPT'); |
There was a problem hiding this comment.
The branch here is really just assuming that these are inline scripts and to get them to execute, we need to reinsert them because (for security reasons innerHTML doesn't).
What you can do is check if .src is specified and if so, fake reading the source file as source, and then injecting it as a script tag with textContent instead. That would simulate more what the browser would do here.
| window.__init_instruction_observer__ = () => { | ||
| global.testDocument = document; | ||
| global.MutationObserver = jsdom.window.MutationObserver; | ||
| reactInstructionObserver = ReactDOMFizzServer.getReactDOMClientMutationObserver(); |
There was a problem hiding this comment.
Like I mentioned below, we should simulate what would happen when we inject this as a <script src="..."> tag. Not getting an already initialized version of this file.
Once you add the new bundle you can read it with fs.readFileSync(require.resolve(script.src)).
| writeChunk(destination, completeSegmentScript1Partial); | ||
| // <div data-attr='["$RS", " | ||
| (writerState: ScriptDataWriterState); | ||
| writeChunk(destination, dataElementStart); |
There was a problem hiding this comment.
Just noting for myself and others...
Originally I was hoping that we could attach these attributes to the node added by writeStartSegment. However, I don't think that would work.
<div hidden data-attr="...">
<div>first</div>
<div>second</div>
</div>
In this case the mutation observer would fire when the first div was added to the tree. Whether or not the second two divs have been streamed in yet. There's also no signal when it is done.
Therefore, unfortunately I think the best we can do is emit an extra node after - like this.
There was a problem hiding this comment.
For segments completing, I think we could maybe listen to any element being added to the segment and just move them as they come in. Since they're just getting added to something hidden anyway.
For boundaries completing, we'd still need a separate element regardless to know when it's safe to do the reveal.
Probably not worth the complexity to special case segments completing.
We're adding an option to Fizz to support an alternate output format that doesn't rely on inline script tags (see facebook#25437). The two outputs will share the same "instruction set" of functions. These functions are currently inlined into the source file; to maintain a single source of truth, in preparation for the new option, this commit moves the instruction set to a separate file that is injected by the build system, similar to a macro. In the future, we could improve this further by running Closure on the instruction set instead of hardcoding the output. This isn't an urgent improvement, though, because we rarely modify the instruction set.
We're adding an option to Fizz to support an alternate output format that doesn't rely on inline script tags (see facebook#25437). The two outputs will share the same "instruction set" of functions. These functions are currently inlined into the source file; to maintain a single source of truth, in preparation for the new option, this commit moves the instruction set to a separate file that is injected by the build system, similar to a macro. In the future, we could improve this further by running Closure on the instruction set instead of hardcoding the output. This isn't an urgent improvement, though, because we rarely modify the instruction set. Co-authored-by: Mofei Zhang <[email protected]>
We're adding an option to Fizz to support an alternate output format that doesn't rely on inline script tags (see facebook#25437). The two outputs will share the same "instruction set" of functions. These functions are currently inlined into the source file; to make this a bit more maintainable, and eventually have a single source of truth, in preparation for the new option, this commit moves the instruction set to a separate files that are imported. In the future, we could improve this further by running Closure on the instruction set and generating it at build time. This isn't an urgent improvement, though, because we rarely modify the instruction set. Co-authored-by: Mofei Zhang <[email protected]>
We're adding an option to Fizz to support an alternate output format that doesn't rely on inline script tags (see facebook#25437). The two outputs will share the same "instruction set" of functions. These functions are currently inlined into the source file; to make this a bit more maintainable, and eventually have a single source of truth, in preparation for the new option, this commit moves the instruction set to a separate files that are imported. In the future, we could improve this further by running Closure on the instruction set and generating it at build time. This isn't an urgent improvement, though, because we rarely modify the instruction set. Co-authored-by: Mofei Zhang <[email protected]>
We're adding an option to Fizz to support an alternate output format that doesn't rely on inline script tags (see facebook#25437). The two outputs will share the same "instruction set" of functions. These functions are currently inlined into the source file; to make this a bit more maintainable, and eventually have a single source of truth, in preparation for the new option, this commit moves the instruction set to a separate files that are imported. In the future, we could improve this further by running Closure on the instruction set and generating it at build time. This isn't an urgent improvement, though, because we rarely modify the instruction set. Co-authored-by: Mofei Zhang <[email protected]>
We're adding an option to Fizz to support an alternate output format that doesn't rely on inline script tags (see #25437). The two outputs will share the same "instruction set" of functions. These functions are currently inlined into the source file; to make this a bit more maintainable, and eventually have a single source of truth, in preparation for the new option, this commit moves the instruction set to a separate files that are imported. In the future, we could improve this further by running Closure on the instruction set and generating it at build time. This isn't an urgent improvement, though, because we rarely modify the instruction set. Co-authored-by: Mofei Zhang <[email protected]> Co-authored-by: Mofei Zhang <[email protected]>
mofeiZ
force-pushed
the
fizz-add-non-execution-format
branch
from
8002669 to
0881b4b
Compare
There was a problem hiding this comment.
Putting this up for review again (thanks @acdlite for helping me with the infra / bundling changes!)
Sorry this is such a large PR. I split out the renaming / trivial changes to the first commit, so ReactDOMFizzServer-test should be easier to review if you click past that.
Changes since last review:
- rebased on @acdlite 's changes (mostly renaming)
- data attribute format changed from JSON list to be unrolled strings
data-instr="$INSTR" data-arg0="param0" data-arg1="param1" ...- We ultimately still are calling JSON.parse for (1) unescaping added characters and (2) style resources, whose types are lists of strings
- Fix in ExternalRuntime: since this script may execute after some instruction tags have already arrived, we now process existing document elements
| let window; | ||
| let document; | ||
| let writable; | ||
| let CSPnonce = null; |
There was a problem hiding this comment.
Removed since it looks like we weren't really using this variable, but happy to add it back
|
|
||
| renderOptions = {}; | ||
| if (gate(flags => flags.enableFizzExternalRuntime)) { | ||
| renderOptions.bootstrapScriptContent = |
There was a problem hiding this comment.
This is pretty hacky
Since the external runtime is a standalone script (not a module), jest needs to do one of these.
- fork the source code (ReactDOMServerExternalRuntime.test.js)
- manually strip
import ... from ...and inject those imported declarations as globals
I chose (2) for now, but would love to change it if there is a cleaner alternative
| } else { | ||
| for (let i = 0; i < node.childNodes.length; i++) { | ||
| const inner = node.childNodes[i]; | ||
| replaceScriptsAndMove(inner, null); |
There was a problem hiding this comment.
From my understanding, we need this to be recursive since some tests stream into containers (divs inside of body). As a result, there are non top level scripts.
mofeiZ
force-pushed
the
fizz-add-non-execution-format
branch
2 times, most recently
from
2d1b1ce to
6facdf9
Compare
scripts/rollup/bundles.js
Outdated
| /******* React DOM Fizz Server External Runtime *******/ | ||
| { | ||
| bundleTypes: [BROWSER_SCRIPT], | ||
| bundleTypes: [BROWSER_SCRIPT, FB_WWW_DEV, FB_WWW_PROD], |
There was a problem hiding this comment.
What's this for? We shouldn't need it
There was a problem hiding this comment.
I believe we need this because Comet infra will statically require this file in their bootstrap module + pass an empty string as externalRuntimeSrc (see internal diff + React sync). The external runtime then should be added into Meta's React build and synced into our repo.
There was a problem hiding this comment.
I see, I was thinking we would update the internal sync script to copy over the one used by npm. That's how we sync the ESLint plugin, for example. We're trying to trend in the direction of less FB-specific infra in the open source repo.
| } | ||
|
|
||
| /// Typechecking code, follows from above functions | ||
| function isSuspenseBoundaryID(s) { |
There was a problem hiding this comment.
I'm not convinced all these dev-only runtime type checking code is worth it. We don't even have a dev build of this module. The only time it runs is when we run our Jest tests.
There was a problem hiding this comment.
I was thinking we could enable this for dev mode internally, but happy to remove (I'm sure debugging $R instructions, should anything go wrong, probably falls on us too).
| switch (instr) { | ||
| case '$RX': | ||
| clientRenderBoundaryImpl( | ||
| node.getAttribute('data-rarg0'), |
There was a problem hiding this comment.
The way we access these in the other runtime is slightly different:
const dataset = node.data
dataset["rarg0"]See:
That would compress slightly better. But also we might as well be consistent.
Also, maybe these should use the $ prefix convention? And instead of rarg1 make it as short as possible to avoid clashes.
3-4 characters seems like enough, like $ra1, $ra2, etc.
There was a problem hiding this comment.
Thanks for pointing out dataset, will change my code to use that!
I had originally used $r. However, reading through the html spec for data attribute names (link), I think the name may need to be XML compatible. If this is the case, that would rule out $.
Happy to defer to you here though - I'm not at all familiar with specs + browser implementations
There was a problem hiding this comment.
Oh right. I remember now that useId generates ids with : for this reason. The convention for useId is :r1: so we can do something like that:
:a0, :a1 and so on.
In a follow up, what we should do is support the identifierPrefix option. Then prepend that to all of these attributes. We can send down prefix as an attribute early in the stream. So that the external runtime knows which attributes to look for. We wanted to do that anyway so that users don't have to manually pass the prefix to hydrateRoot.
| // This runtime may be sent to the client multiple times (if FizzServer.render | ||
| // is called more than once). Here, we check whether the mutation observer | ||
| // was already created / installed | ||
| if (!window.reactInstructionObserver && window.MutationObserver) { |
There was a problem hiding this comment.
We shouldn't need to check for the existence of MutationObserver. There's no fallback behavior so it should just hard error if it doesn't exist.
There was a problem hiding this comment.
Ah you're totally right, this can be moved to Meta's internal infra. Thanks for calling out the Meta-specific code in this PR
| // ReactDOMFizzInstructionSet | ||
| window.$RC = completeBoundary; | ||
| window.$RM = new Map(); | ||
| window.reactInstructionObserver = new MutationObserver(mutations => { |
There was a problem hiding this comment.
reactInstructionObserver needs to be obscure, too. It's essentially a private field; should only be accessed by this module. Use the $ naming convention.
|
@gnoff @sebmarkbage |
| const domBodyObserver = new MutationObserver(() => { | ||
| // We expect the body node to be stable once parsed / created | ||
| if (document.body) { | ||
| if (document.readyState === 'loading') { |
There was a problem hiding this comment.
Not sure the order of DOM parsing / changing readyState - this could totally be a redundant check
Added just in case readyState gets set loading -> interactive before mutation observer listeners are triggered.
(experimented with inline scripts on my laptop's version of Chrome, but wary about other browser implementations)
There was a problem hiding this comment.
I don't know off the top of my head. seems safe enough to leave in
| } else { | ||
| // body may not exist yet if the fizz runtime is sent in <head> | ||
| // (e.g. as a preinit resource) | ||
| const domBodyObserver = new MutationObserver(() => { |
There was a problem hiding this comment.
I feel like I am missing something but does this observer ever observe anything?
There was a problem hiding this comment.
Ahh I can't believe I didn't call observe, thanks so much for catching this - definitely a problem.
| const domBodyObserver = new MutationObserver(() => { | ||
| // We expect the body node to be stable once parsed / created | ||
| if (document.body) { | ||
| if (document.readyState === 'loading') { |
There was a problem hiding this comment.
I don't know off the top of my head. seems safe enough to leave in
|
(Sorry for the back and forth, and thanks again for the review! Once again putting this back on your queue @gnoff ) |
1 similar comment
|
(Sorry for the back and forth, and thanks again for the review! Once again putting this back on your queue @gnoff ) |
| ...defaultOptions, | ||
| ...options, | ||
| }; | ||
| } |
There was a problem hiding this comment.
This function doesn't really need to exist. The function name is longer than the syntax to write this out. This is strictly worse since it made me look up this definition, thinking it was something more involved, but it also drops the types because it's effectively "any" typed.
| } | ||
| return nodes.filter( | ||
| n => | ||
| (n.tagName !== 'SCRIPT' && n.tagName !== 'script') || |
There was a problem hiding this comment.
Why the lower case? SVG?
There was a problem hiding this comment.
@sebmarkbage
I'm not sure why this is needed, but I see that tagName is sometimes lowercase. I saw some existing code like this, and didn't think too much of it.
Can look into it if it seems suggestive of a larger issue!
6 participants
Changes made:
ReactDOMServerExternalRuntime, which processes and removes these nodesunstable_externalRuntimeSrcpreinit(...)inside a component.unstable_externalRuntimeSrc, soenableFizzExternalRuntimeis only valid whenenableFloatis also set.What's left (done)
AddenableFizzExternalRuntimeas an entrypoint parameterAdd a MutationObserver implementationAdd ^ to test setup (when using Meta's feature flags)Summary
How did you test this change?