Implement basic stylesheet Resources for react-dom #25060
Conversation
gnoff
force-pushed
the
float-initial-resources
branch
5 times, most recently
from
3bf2574
to
545495d
Compare
gnoff
changed the title
| @@ -242,6 +243,26 @@ export function getChildFormatContext( | |||
| return parentContext; | |||
| } | |||
|
|
|||
| export function isPreambleInsertion(type: string): boolean { | |||
There was a problem hiding this comment.
Why is this exported?
| type: string, | ||
| props: Object, | ||
| responseState: ResponseState, | ||
| formatContext: FormatContext, | ||
| ): ReactNodeList { | ||
| // Preamble type is nullable for feature off cases but is guaranteed when feature is on | ||
| target = enableFloat && isPreambleInsertion(type) ? (preamble: any) : target; |
There was a problem hiding this comment.
There is a pattern that I explicitly tried to avoid when refactoring the DOM bindings as part of Fizz and I was planning to do when refactoring the client next.
The old pattern was basically a bunch of abstractions around specific conditions.
Instead of a bunch of isSomething(...) helpers. I structured it fully based on a switch over each kind of value once and from there the code flow determines what to do next.
In other words, we should not have things like isPreambleInsertion checks and instead always use the control flow that's already there.
Make a special pushStartHTML, pushStartHead etc. or a shared pushStartPreamble (for now) and then call that from within the switch statement. Then you can check the feature flag in there and otherwise call the generic pushStartGenericElement. If the flag is on and you need to fork behavior you can fork pushStartGenericElement and instead you can call the shared inner functions that pushStartGenericElement calls but it's a shared abstraction. However I don't think you need to do that, you can probably just fork the target chunk array. Although that might not work if we do the string thing in the future.
There was a problem hiding this comment.
I see what you mean now
| type: string, | ||
| props: Object, | ||
| ): void { | ||
| // Preamble type is nullable for feature off cases but is guaranteed when feature is on | ||
| target = | ||
| enableFloat && isPostambleInsertion(type) ? (postamble: any) : target; |
There was a problem hiding this comment.
Same thing here. Put this in the existing switch.
| @@ -2149,6 +2168,21 @@ function flushCompletedQueues( | |||
| // We don't need to check any partially completed segments because | |||
| // either they have pending task or they're complete. | |||
| ) { | |||
| allComplete = true; | |||
There was a problem hiding this comment.
Moving this out of the finally is not correct because if something throws above here, then we won't set this to true and so we don't close the connection if that throw caused all tasks to complete (e.g by being aborted).
There was a problem hiding this comment.
Ah, didn't think of that :)
| rootHostContainer, | ||
| ).querySelectorAll(selector); | ||
| if (allLinks.length > 1) { | ||
| throw new Error( |
There was a problem hiding this comment.
We should never throw conditionally on __DEV__. If so, it should be console.error instead.
| @@ -596,6 +616,9 @@ function popHydrationState(fiber: Fiber): boolean { | |||
| if (!supportsHydration) { | |||
| return false; | |||
| } | |||
| if (enableFloat && isHydratableResource(fiber.type, fiber.memoizedProps)) { | |||
There was a problem hiding this comment.
This is always picking this path if it's a hydratable resource, but the push path is sometimes falling through (if there is no matching resource). This means that they can be misbalanced and lead to something weird.
There was a problem hiding this comment.
Ah. That's why you check the state node.
Shouldn't it always just insert a node if it's missing to patch things up rather than leaving it wrong?
There was a problem hiding this comment.
If it is missing, it gets treated like a hydration error and the client side render will patch things up if possible. Sometimes this won't be possible b/c the link will be out of position and I don't yet have insertion semantics for resources (i.e. if you link is a sibling of it will try to insert into the document which will fail)
The least impactful way to do the insertion would be during the hydration itself which feels sorta wrong b/c i'd be mutating the DOM in the render pass. I know we might do this for preloads at some point but that feels a little out of the norm for a hydration insertion.
If I insert via a normal placement I'm going to need to figure out what to do about resources above and it starts to get messy. I'd rather we have a proper Resources implementation that considers hoisting as a first class concept so I was hoping we could leave this alone as a temporary stop gap
| supportsHydration && | ||
| isHydratableResource(node.type, node.memoizedProps) | ||
| ) { | ||
| // This is a crude opt out of Resources from this search algorithm |
There was a problem hiding this comment.
Just leave this to finding the link tag for now. It'll be fixed once you add the new tag. These reflection helpers are semi-deprecated anyway.
There was a problem hiding this comment.
unfortunately, this errors if we don't do this
https://github.com/facebook/react/blob/main/packages/react-dom/src/client/ReactDOMRoot.js#L122-L132
There was a problem hiding this comment.
Should I just not check if the root was manipulated when enableFloat is on?
There was a problem hiding this comment.
Yea, that's not an important warning.
There was a problem hiding this comment.
Ok should be good to review
gnoff
force-pushed
the
float-initial-resources
branch
from
df91473
to
6d94b15
Compare
| @@ -2139,8 +2159,7 @@ function flushCompletedQueues( | |||
| } | |||
| largeBoundaries.splice(0, i); | |||
| } finally { | |||
| completeWriting(destination); | |||
| flushBuffered(destination); | |||
| let allComplete = false; | |||
There was a problem hiding this comment.
You can avoid this flag and just use control flow here instead. Just duplicate the completeWriting/flushBuffered in the else branch.
There was a problem hiding this comment.
What's the tradeoff here? Less branching but trivially more code?
| // we expect the preamble to be tiny and will ignore backpressure | ||
| writeChunk(destination, preamble[i]); | ||
| } | ||
| preamble.length = 0; |
There was a problem hiding this comment.
Why bother resetting this? The memory will still be retained.
There was a problem hiding this comment.
I think a holdover from when this wasn't gated in a flush once block but agree it can be removed
| for (let i = 0; i < postamble.length; i++) { | ||
| writeChunk(destination, postamble[i]); | ||
| } | ||
| postamble.length = 0; |
| responseState: ResponseState, | ||
| ): ReactNodeList { | ||
| // Preamble type is nullable for feature off cases but is guaranteed when feature is on | ||
| target = enableFloat && isPreambleInsertion(tag) ? (preamble: any) : target; |
There was a problem hiding this comment.
You don't need the isPreambleInsertion() checks. That was the point of creating the pushStartHead helper - now you know that it's a preamble insertion.
| responseState: ResponseState, | ||
| ): ReactNodeList { | ||
| // Preamble type is nullable for feature off cases but is guaranteed when feature is on | ||
| target = enableFloat && isPreambleInsertion(tag) ? (preamble: any) : target; |
| case 'html': | ||
| // Preamble type is nullable for feature off cases but is guaranteed when feature is on | ||
| target = | ||
| enableFloat && isPostambleInsertion(type) ? (postamble: any) : target; |
| tag: string, | ||
| responseState: ResponseState, | ||
| ): ReactNodeList { | ||
| // Preamble type is nullable for feature off cases but is guaranteed when feature is on |
There was a problem hiding this comment.
I usually just make the type contain everything you'd expect to exist in the new common state.
There was a problem hiding this comment.
I was trying to avoid two extra array allocations for non dom (even though no one else is really using it yet I suppose). In the future I'd expect that preamble is not nullable for dom fizz server and is explicitly null for environments that do not support preamble but how would I type that? I guess I could export the preamble type but then I'd probably also need a create function for the preamble itself.
Do you recommend doing it that way?
There was a problem hiding this comment.
It'd just make it required. It's at most two per request, and we don't know if other renderers will end up using it yet.
| pushAttribute(target, responseState, 'data-rprec', propValue); | ||
| } else if (__DEV__) { | ||
| throw new Error( | ||
| `the "precedence" prop for links to stylehseets expects to receive a string but received something of type "${typeof propValue}" instead.`, |
There was a problem hiding this comment.
small typo: "stylehseets" -> "stylesheets"
Summary: This sync includes the following changes: - **[c28f313e6](facebook/react@c28f313e6 )**: experimental_use(promise) for SSR ([#25214](facebook/react#25214)) //<Andrew Clark>// - **[d6f9628a8](facebook/react@d6f9628a8 )**: Remove some RSC subset entry points that were removed in the main entry point ([#25209](facebook/react#25209)) //<Sebastian Markbåge>// - **[a473d08fc](facebook/react@a473d08fc )**: Update to Flow from 0.97 to 0.122 ([#25204](facebook/react#25204)) //<Jan Kassens>// - **[7028ce745](facebook/react@7028ce745 )**: experimental_use(promise) for Server Components ([#25207](facebook/react#25207)) //<Andrew Clark>// - **[bfb65681e](facebook/react@bfb65681e )**: experimental_use(context)([#25202](facebook/react#25202)) //<mofeiZ>// - **[f0efa1164](facebook/react@f0efa1164 )**: [flow] remove custom suppress comment config ([#25170](facebook/react#25170)) //<Jan Kassens>// - **[2e7f422fe](facebook/react@2e7f422fe )**: Refactor: its type is Container ([#25153](facebook/react#25153)) //<bubucuo>// - **[2c2d9a1df](facebook/react@2c2d9a1df )**: [eslint-plugin-react-hooks] only allow capitalized component names ([#25162](facebook/react#25162)) //<Jan Kassens>// - **[36c908a6c](facebook/react@36c908a6c )**: Don't use the Flight terminology in public error messages ([#25166](facebook/react#25166)) //<Sebastian Markbåge>// - **[8d1b057ec](facebook/react@8d1b057ec )**: [Flight] Minor error handling fixes ([#25151](facebook/react#25151)) //<Sebastian Markbåge>// - **[9ff738f53](facebook/react@9ff738f53 )**: [devtools][easy] Fix flow type ([#25147](facebook/react#25147)) //<Tianyu Yao>// - **[0de3ddf56](facebook/react@0de3ddf56 )**: Remove Symbol Polyfill (again) ([#25144](facebook/react#25144)) //<Ricky>// - **[b36f72235](facebook/react@b36f72235 )**: Remove ReactFiberFlags MountLayoutDev and MountPassiveDev ([#25091](facebook/react#25091)) //<Samuel Susla>// - **[b6978bc38](facebook/react@b6978bc38 )**: experimental_use(promise) ([#25084](facebook/react#25084)) //<Andrew Clark>// - **[11ed7010c](facebook/react@11ed7010c )**: [Transition Tracing] onMarkerIncomplete - Tracing Marker/Suspense Boundary Deletions ([#24885](facebook/react#24885)) //<Luna Ruan>// - **[b79894259](facebook/react@b79894259 )**: [Flight] Add support for Webpack Async Modules ([#25138](facebook/react#25138)) //<Sebastian Markbåge>// - **[c8b778b7f](facebook/react@c8b778b7f )**: Fix typo: supportsMicrotask -> supportsMicrotasks ([#25142](facebook/react#25142)) //<kwzr>// - **[d0f396651](facebook/react@d0f396651 )**: Allow functions to be used as module references ([#25137](facebook/react#25137)) //<Sebastian Markbåge>// - **[38c5d8a03](facebook/react@38c5d8a03 )**: Test the node-register hooks in unit tests ([#25132](facebook/react#25132)) //<Sebastian Markbåge>// - **[3f70e68ce](facebook/react@3f70e68ce )**: Return closestInstance in `getInspectorDataForViewAtPoint` ([#25118](facebook/react#25118)) //<Tianyu Yao>// - **[3d443cad7](facebook/react@3d443cad7 )**: Update fixtures/flight to webpack 5 ([#25115](facebook/react#25115)) //<Tim Neutkens>// - **[5d1ce6513](facebook/react@5d1ce6513 )**: Align StrictMode behaviour with production ([#25049](facebook/react#25049)) //<Samuel Susla>// - **[9e67e7a31](facebook/react@9e67e7a31 )**: Scaffolding for useMemoCache hook ([#25123](facebook/react#25123)) //<Joseph Savona>// - **[19e9a4c68](facebook/react@19e9a4c68 )**: Add missing createServerContext for experimental shared subset ([#25114](facebook/react#25114)) //<Jiachi Liu>// - **[6ef466c68](facebook/react@6ef466c68 )**: make preamble and postamble types explicit and fix typo ([#25102](facebook/react#25102)) //<Josh Story>// - **[796d31809](facebook/react@796d31809 )**: Implement basic stylesheet Resources for react-dom ([#25060](facebook/react#25060)) //<Josh Story>// - **[32baab38f](facebook/react@32baab38f )**: [Transition Tracing] Add Tag Field to Marker Instance ([#25085](facebook/react#25085)) //<Luna Ruan>// - **[8ef3a7c08](facebook/react@8ef3a7c08 )**: Resume immediately pinged fiber without unwinding ([#25074](facebook/react#25074)) //<Andrew Clark>// - **[7bcc68772](facebook/react@7bcc68772 )**: Remove argument committedLanes from reappearLayoutEffects and recursivelyTraverseReappearLayoutEffects ([#25080](facebook/react#25080)) //<Samuel Susla>// - **[ca990e9a7](facebook/react@ca990e9a7 )**: Add API to force Scheduler to yield for macrotask ([#25044](facebook/react#25044)) //<Andrew Clark>// - **[b4204ede6](facebook/react@b4204ede6 )**: Clean up unused Deletion flag ([#24992](facebook/react#24992)) //<Andrew Clark>// - **[e193be87e](facebook/react@e193be87e )**: [Transition Tracing] Add Offscreen Test ([#25035](facebook/react#25035)) //<Luna Ruan>// - **[9fcaf88d5](facebook/react@9fcaf88d5 )**: Remove rootContainerInstance from unnecessary places ([#25024](facebook/react#25024)) //<Sebastian Markbåge>// - **[80f3d8819](facebook/react@80f3d8819 )**: Mount/unmount passive effects when Offscreen visibility changes ([#24977](facebook/react#24977)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions 4ea064e...c28f313 Reviewed By: rickhanlonii Differential Revision: D39384898 fbshipit-source-id: 20b080a53851d6dd9d522c8468dd02aab9ba76db
Implement basic support for "Resources". In the context of this commit, the only thing that is currently a Resource are <link rel="stylesheet" precedence="some-value" ...> Resources can be rendered anywhere in the react tree, even outside of normal parenting rules, for instance you can render a resource before you have rendered the <html><head> tags for your application. In the stream we reorder this so the browser always receives valid HTML and resources are emitted either in place (normal circumstances) or at the top of the <head> (when you render them above or before the <head> in your react tree) On the client, resources opt into an entirely different hydration path. Instead of matching the location within the Document these resources are queried for in the entire document. It is an error to have more than one resource with the same href attribute. The use of precedence here as an opt-in signal for resourcifying the link is in preparation for a more complete Resource implementation which will dedupe resource references (multiple will be valid), hoist to the appropriate container (body, head, or elsewhere), order (according to precedence) and Suspend boundaries that depend on them. More details will come in the coming weeks on this plan. This feature is gated by an experimental flag and will only be made available in experimental builds until some future time.
Summary: This sync includes the following changes: - **[c28f313e6](facebook/react@c28f313e6 )**: experimental_use(promise) for SSR ([facebook#25214](facebook/react#25214)) //<Andrew Clark>// - **[d6f9628a8](facebook/react@d6f9628a8 )**: Remove some RSC subset entry points that were removed in the main entry point ([facebook#25209](facebook/react#25209)) //<Sebastian Markbåge>// - **[a473d08fc](facebook/react@a473d08fc )**: Update to Flow from 0.97 to 0.122 ([facebook#25204](facebook/react#25204)) //<Jan Kassens>// - **[7028ce745](facebook/react@7028ce745 )**: experimental_use(promise) for Server Components ([facebook#25207](facebook/react#25207)) //<Andrew Clark>// - **[bfb65681e](facebook/react@bfb65681e )**: experimental_use(context)([facebook#25202](facebook/react#25202)) //<mofeiZ>// - **[f0efa1164](facebook/react@f0efa1164 )**: [flow] remove custom suppress comment config ([facebook#25170](facebook/react#25170)) //<Jan Kassens>// - **[2e7f422fe](facebook/react@2e7f422fe )**: Refactor: its type is Container ([facebook#25153](facebook/react#25153)) //<bubucuo>// - **[2c2d9a1df](facebook/react@2c2d9a1df )**: [eslint-plugin-react-hooks] only allow capitalized component names ([facebook#25162](facebook/react#25162)) //<Jan Kassens>// - **[36c908a6c](facebook/react@36c908a6c )**: Don't use the Flight terminology in public error messages ([facebook#25166](facebook/react#25166)) //<Sebastian Markbåge>// - **[8d1b057ec](facebook/react@8d1b057ec )**: [Flight] Minor error handling fixes ([facebook#25151](facebook/react#25151)) //<Sebastian Markbåge>// - **[9ff738f53](facebook/react@9ff738f53 )**: [devtools][easy] Fix flow type ([facebook#25147](facebook/react#25147)) //<Tianyu Yao>// - **[0de3ddf56](facebook/react@0de3ddf56 )**: Remove Symbol Polyfill (again) ([facebook#25144](facebook/react#25144)) //<Ricky>// - **[b36f72235](facebook/react@b36f72235 )**: Remove ReactFiberFlags MountLayoutDev and MountPassiveDev ([facebook#25091](facebook/react#25091)) //<Samuel Susla>// - **[b6978bc38](facebook/react@b6978bc38 )**: experimental_use(promise) ([facebook#25084](facebook/react#25084)) //<Andrew Clark>// - **[11ed7010c](facebook/react@11ed7010c )**: [Transition Tracing] onMarkerIncomplete - Tracing Marker/Suspense Boundary Deletions ([facebook#24885](facebook/react#24885)) //<Luna Ruan>// - **[b79894259](facebook/react@b79894259 )**: [Flight] Add support for Webpack Async Modules ([facebook#25138](facebook/react#25138)) //<Sebastian Markbåge>// - **[c8b778b7f](facebook/react@c8b778b7f )**: Fix typo: supportsMicrotask -> supportsMicrotasks ([facebook#25142](facebook/react#25142)) //<kwzr>// - **[d0f396651](facebook/react@d0f396651 )**: Allow functions to be used as module references ([facebook#25137](facebook/react#25137)) //<Sebastian Markbåge>// - **[38c5d8a03](facebook/react@38c5d8a03 )**: Test the node-register hooks in unit tests ([facebook#25132](facebook/react#25132)) //<Sebastian Markbåge>// - **[3f70e68ce](facebook/react@3f70e68ce )**: Return closestInstance in `getInspectorDataForViewAtPoint` ([facebook#25118](facebook/react#25118)) //<Tianyu Yao>// - **[3d443cad7](facebook/react@3d443cad7 )**: Update fixtures/flight to webpack 5 ([facebook#25115](facebook/react#25115)) //<Tim Neutkens>// - **[5d1ce6513](facebook/react@5d1ce6513 )**: Align StrictMode behaviour with production ([facebook#25049](facebook/react#25049)) //<Samuel Susla>// - **[9e67e7a31](facebook/react@9e67e7a31 )**: Scaffolding for useMemoCache hook ([facebook#25123](facebook/react#25123)) //<Joseph Savona>// - **[19e9a4c68](facebook/react@19e9a4c68 )**: Add missing createServerContext for experimental shared subset ([facebook#25114](facebook/react#25114)) //<Jiachi Liu>// - **[6ef466c68](facebook/react@6ef466c68 )**: make preamble and postamble types explicit and fix typo ([facebook#25102](facebook/react#25102)) //<Josh Story>// - **[796d31809](facebook/react@796d31809 )**: Implement basic stylesheet Resources for react-dom ([facebook#25060](facebook/react#25060)) //<Josh Story>// - **[32baab38f](facebook/react@32baab38f )**: [Transition Tracing] Add Tag Field to Marker Instance ([facebook#25085](facebook/react#25085)) //<Luna Ruan>// - **[8ef3a7c08](facebook/react@8ef3a7c08 )**: Resume immediately pinged fiber without unwinding ([facebook#25074](facebook/react#25074)) //<Andrew Clark>// - **[7bcc68772](facebook/react@7bcc68772 )**: Remove argument committedLanes from reappearLayoutEffects and recursivelyTraverseReappearLayoutEffects ([facebook#25080](facebook/react#25080)) //<Samuel Susla>// - **[ca990e9a7](facebook/react@ca990e9a7 )**: Add API to force Scheduler to yield for macrotask ([facebook#25044](facebook/react#25044)) //<Andrew Clark>// - **[b4204ede6](facebook/react@b4204ede6 )**: Clean up unused Deletion flag ([facebook#24992](facebook/react#24992)) //<Andrew Clark>// - **[e193be87e](facebook/react@e193be87e )**: [Transition Tracing] Add Offscreen Test ([facebook#25035](facebook/react#25035)) //<Luna Ruan>// - **[9fcaf88d5](facebook/react@9fcaf88d5 )**: Remove rootContainerInstance from unnecessary places ([facebook#25024](facebook/react#25024)) //<Sebastian Markbåge>// - **[80f3d8819](facebook/react@80f3d8819 )**: Mount/unmount passive effects when Offscreen visibility changes ([facebook#24977](facebook/react#24977)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions 4ea064e...c28f313 Reviewed By: rickhanlonii Differential Revision: D39384898 fbshipit-source-id: 20b080a53851d6dd9d522c8468dd02aab9ba76db
Implement basic support for "Resources". In the context of this PR, the only thing that is currently a Resource are
<link rel="stylesheet" precedence="some-value" ...>Resources can be rendered anywhere in the react tree, even outside of normal parenting rules, for instance you can render a resource before you have rendered the
<html><head>tags for your application. In the stream we reorder this so the browser always receives valid HTML and resources are emitted either in place (normal circumstances) or at the top of the<head>(when you render them above or before the<head>in your react tree)On the client, resources opt into an entirely different hydration path. Instead of matching the location within the Document these resources are queried for in the entire document. It is an error to have more than one resource with the same
hrefattribute.The use of
precedencehere as an opt-in signal for resourcifying the link is in preparation for a more complete Resource implementation which will dedupe resource references (mutliple will be valid), hoist to the appropriate container (body, head, or elsewhere), order (according to precedence) and Suspend boundaries that depend on them. More details will come in the coming weeks on this plan.This feature is gated by an experimental flag and will only be made available in experimental builds until some future time.