From 204a8f750fe025291c0dfeb60c71d8e6a620a0e8 Mon Sep 17 00:00:00 2001 From: Josh Story Date: Wed, 20 Sep 2023 12:23:04 -0700 Subject: [PATCH 01/10] This change refactors the implementation of Stylesheet resources and Style tag Resources. The Style tag resource was already handled with special rules because it does not Flush is the same way as other resources do. This change reimagines the precedences as a combination of a style tag rules queue and a list of stylesheets. This change eliminates a number of conditionals by embedding these variants in the data represenation itself. This change also moves the binary encoding to work time rather than flush time. Since style resources were the only type that actually used the `type` property I've removed this making resource object size slightly smaller --- .../src/server/ReactFizzConfigDOM.js | 377 ++++++++---------- .../src/server/ReactFizzConfigDOMLegacy.js | 10 +- 2 files changed, 166 insertions(+), 221 deletions(-) diff --git a/packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js b/packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js index 5dd228795ca87..9eb6780fe5de1 100644 --- a/packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js +++ b/packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js @@ -152,8 +152,7 @@ export type RenderState = { fontPreloads: Set, highImagePreloads: Set, // usedImagePreloads: Set, - precedences: Map>, - stylePrecedences: Map, + precedences: Map, bootstrapScripts: Set, scripts: Set, bulkPreloads: Set, @@ -2158,38 +2157,23 @@ function pushLink( } } const resource = { - type: 'stylesheet', chunks: ([]: Array), state, props: resourceProps, }; resumableState.stylesMap[key] = null; if (!stylesInPrecedence) { - stylesInPrecedence = new Map(); - renderState.precedences.set(precedence, stylesInPrecedence); - const emptyStyleResource = { - type: 'style', - chunks: ([]: Array), - state: NoState, - props: { - precedence, - hrefs: ([]: Array), - }, + stylesInPrecedence = { + precedence: stringToChunk(escapeTextForBrowser(precedence)), + rules: ([]: Array), + hrefs: ([]: Array), + sheets: (new Map(): Map), }; - stylesInPrecedence.set('', emptyStyleResource); - if (__DEV__) { - if (renderState.stylePrecedences.has(precedence)) { - console.error( - 'React constructed an empty style resource when a style resource already exists for this precedence: "%s". This is a bug in React.', - precedence, - ); - } - } - renderState.stylePrecedences.set(precedence, emptyStyleResource); + renderState.precedences.set(precedence, stylesInPrecedence); } - stylesInPrecedence.set(key, resource); + stylesInPrecedence.sheets.set(key, resource); if (renderState.boundaryResources) { - renderState.boundaryResources.add(resource); + renderState.boundaryResources.sheets.add(resource); } } else { // We need to track whether this boundary should wait on this resource or not. @@ -2198,10 +2182,10 @@ function pushLink( // and then it wouldn't be recreated in the RenderState and there's no need to track // it again since we should've hoisted it to the shell already. if (stylesInPrecedence) { - const resource = stylesInPrecedence.get(key); + const resource = stylesInPrecedence.sheets.get(key); if (resource) { if (renderState.boundaryResources) { - renderState.boundaryResources.add(resource); + renderState.boundaryResources.sheets.add(resource); } } } @@ -2335,44 +2319,32 @@ function pushStyle( } const key = getResourceKey('style', href); - let resource = renderState.stylePrecedences.get(precedence); + let stylesInPrecedence = renderState.precedences.get(precedence); if (!resumableState.stylesMap.hasOwnProperty(key)) { - if (!resource) { - resource = { - type: 'style', - chunks: [], - state: NoState, - props: { - precedence, - hrefs: [href], - }, + if (!stylesInPrecedence) { + stylesInPrecedence = { + precedence: stringToChunk(escapeTextForBrowser(precedence)), + rules: ([]: Array), + hrefs: [stringToChunk(escapeTextForBrowser(href))], + sheets: (new Map(): Map), }; - renderState.stylePrecedences.set(precedence, resource); - const stylesInPrecedence: Map = new Map(); - stylesInPrecedence.set('', resource); - if (__DEV__) { - if (renderState.precedences.has(precedence)) { - console.error( - 'React constructed a new style precedence set when one already exists for this precedence: "%s". This is a bug in React.', - precedence, - ); - } - } renderState.precedences.set(precedence, stylesInPrecedence); } else { - resource.props.hrefs.push(href); + stylesInPrecedence.hrefs.push( + stringToChunk(escapeTextForBrowser(href)), + ); } resumableState.stylesMap[key] = null; - pushStyleContents(resource.chunks, props); + pushStyleContents(stylesInPrecedence.rules, props); } - if (resource) { + if (stylesInPrecedence) { // We need to track whether this boundary should wait on this resource or not. // Typically this resource should always exist since we either had it or just created // it. However, it's possible when you resume that the style has already been emitted // and then it wouldn't be recreated in the RenderState and there's no need to track // it again since we should've hoisted it to the shell already. if (renderState.boundaryResources) { - renderState.boundaryResources.add(resource); + renderState.boundaryResources.precedences.add(stylesInPrecedence); } } @@ -2546,7 +2518,6 @@ function pushImg( referrerPolicy: props.referrerPolicy, }; resource = { - type: 'preload', chunks: [], state: NoState, props: preloadProps, @@ -2903,7 +2874,6 @@ function pushScript( // We can make this "`, + `"
hello world
"`, ); }); @@ -505,7 +505,7 @@ describe('ReactDOMFizzServerBrowser', () => { ); const result = await readResult(stream); expect(result).toMatchInlineSnapshot( - `"
hello world
"`, + `"
hello world
"`, ); }); diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js index 4a20f1be57f77..f7bab722bb386 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js @@ -98,7 +98,7 @@ describe('ReactDOMFizzServerNode', () => { pipe(writable); jest.runAllTimers(); expect(output.result).toMatchInlineSnapshot( - `"
hello world
"`, + `"
hello world
"`, ); }); diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzStaticBrowser-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzStaticBrowser-test.js index 58e34316b0782..9ec0f2c97c9fb 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzStaticBrowser-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzStaticBrowser-test.js @@ -145,7 +145,7 @@ describe('ReactDOMFizzStaticBrowser', () => { }); const prelude = await readContent(result.prelude); expect(prelude).toMatchInlineSnapshot( - `"
hello world
"`, + `"
hello world
"`, ); }); diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzStaticNode-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzStaticNode-test.js index 01f3bf66bbea0..a049ec4c49222 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzStaticNode-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzStaticNode-test.js @@ -86,7 +86,7 @@ describe('ReactDOMFizzStaticNode', () => { ); const prelude = await readContent(result.prelude); expect(prelude).toMatchInlineSnapshot( - `"
hello world
"`, + `"
hello world
"`, ); }); diff --git a/packages/react-dom/src/shared/ReactDOMTypes.js b/packages/react-dom/src/shared/ReactDOMTypes.js index af54a9e27bc65..c12b5e9969fd5 100644 --- a/packages/react-dom/src/shared/ReactDOMTypes.js +++ b/packages/react-dom/src/shared/ReactDOMTypes.js @@ -24,6 +24,7 @@ export type PreloadModuleOptions = { as?: string, crossOrigin?: string, integrity?: string, + nonce?: string, }; export type PreinitOptions = { as: string, @@ -37,6 +38,7 @@ export type PreinitModuleOptions = { as?: string, crossOrigin?: string, integrity?: string, + nonce?: string, }; export type CrossOriginEnum = '' | 'use-credentials'; @@ -56,6 +58,7 @@ export type PreloadModuleImplOptions = { as?: ?string, crossOrigin?: ?CrossOriginEnum, integrity?: ?string, + nonce?: ?string, }; export type PreinitStyleOptions = { crossOrigin?: ?string, @@ -70,7 +73,8 @@ export type PreinitScriptOptions = { }; export type PreinitModuleScriptOptions = { crossOrigin?: ?CrossOriginEnum, - integrity?: string, + integrity?: ?string, + nonce?: ?string, }; export type HostDispatcher = { diff --git a/packages/react-server-dom-fb/src/__tests__/ReactDOMServerFB-test.internal.js b/packages/react-server-dom-fb/src/__tests__/ReactDOMServerFB-test.internal.js index ddb2efd72e5bb..35b41cbd230d0 100644 --- a/packages/react-server-dom-fb/src/__tests__/ReactDOMServerFB-test.internal.js +++ b/packages/react-server-dom-fb/src/__tests__/ReactDOMServerFB-test.internal.js @@ -59,7 +59,7 @@ describe('ReactDOMServerFB', () => { }); const result = readResult(stream); expect(result).toMatchInlineSnapshot( - `"
hello world
"`, + `"
hello world
"`, ); }); From de72972d88adac9d5ce4b66222bde47211299207 Mon Sep 17 00:00:00 2001 From: Josh Story Date: Thu, 21 Sep 2023 19:34:28 -0700 Subject: [PATCH 04/10] renames type Precedence to PrecedenceQueue to better describe what it is. A precedence is now always the string value and the precedence queue is the thing that holds info about style tag rules and stylesheets that need to be flushed for that precedence --- .../src/server/ReactFizzConfigDOM.js | 89 +++++++++---------- .../src/server/ReactFizzConfigDOMLegacy.js | 4 +- 2 files changed, 45 insertions(+), 48 deletions(-) diff --git a/packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js b/packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js index 94332dedb3b1c..99a8feb2736f5 100644 --- a/packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js +++ b/packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js @@ -152,7 +152,7 @@ export type RenderState = { fontPreloads: Set, highImagePreloads: Set, // usedImagePreloads: Set, - precedences: Map, + precedences: Map, bootstrapScripts: Set, scripts: Set, bulkPreloads: Set, @@ -2196,7 +2196,7 @@ function pushLink( return pushLinkImpl(target, props); } else { // This stylesheet refers to a Resource and we create a new one if necessary - let stylesInPrecedence = renderState.precedences.get(precedence); + let precedenceQueue = renderState.precedences.get(precedence); const hasKey = resumableState.styleResources.hasOwnProperty(key); const resourceState = hasKey ? resumableState.styleResources[key] @@ -2207,14 +2207,14 @@ function pushLink( // If this is the first time we've encountered this precedence we need // to create a PrecedenceQueue - if (!stylesInPrecedence) { - stylesInPrecedence = { + if (!precedenceQueue) { + precedenceQueue = { precedence: stringToChunk(escapeTextForBrowser(precedence)), rules: ([]: Array), hrefs: ([]: Array), sheets: (new Map(): Map), }; - renderState.precedences.set(precedence, stylesInPrecedence); + renderState.precedences.set(precedence, precedenceQueue); } const resource: StylesheetResource = { @@ -2249,7 +2249,7 @@ function pushLink( // We add the newly created resource to our PrecedenceQueue and if necessary // track the resource with the currently rendering boundary - stylesInPrecedence.sheets.set(key, resource); + precedenceQueue.sheets.set(key, resource); if (renderState.boundaryResources) { renderState.boundaryResources.sheets.add(resource); } @@ -2259,8 +2259,8 @@ function pushLink( // it. However, it's possible when you resume that the style has already been emitted // and then it wouldn't be recreated in the RenderState and there's no need to track // it again since we should've hoisted it to the shell already. - if (stylesInPrecedence) { - const resource = stylesInPrecedence.sheets.get(key); + if (precedenceQueue) { + const resource = precedenceQueue.sheets.get(key); if (resource) { if (renderState.boundaryResources) { renderState.boundaryResources.sheets.add(resource); @@ -2397,7 +2397,7 @@ function pushStyle( } const key = getResourceKey(href); - let stylesInPrecedence = renderState.precedences.get(precedence); + let precedenceQueue = renderState.precedences.get(precedence); const hasKey = resumableState.styleResources.hasOwnProperty(key); const resourceState = hasKey ? resumableState.styleResources[key] @@ -2415,32 +2415,30 @@ function pushStyle( } } - if (!stylesInPrecedence) { + if (!precedenceQueue) { // This is the first time we've encountered this precedence we need // to create a PrecedenceQueue. - stylesInPrecedence = { + precedenceQueue = { precedence: stringToChunk(escapeTextForBrowser(precedence)), rules: ([]: Array), hrefs: [stringToChunk(escapeTextForBrowser(href))], sheets: (new Map(): Map), }; - renderState.precedences.set(precedence, stylesInPrecedence); + renderState.precedences.set(precedence, precedenceQueue); } else { // We have seen this precedence before and need to track this href - stylesInPrecedence.hrefs.push( - stringToChunk(escapeTextForBrowser(href)), - ); + precedenceQueue.hrefs.push(stringToChunk(escapeTextForBrowser(href))); } - pushStyleContents(stylesInPrecedence.rules, props); + pushStyleContents(precedenceQueue.rules, props); } - if (stylesInPrecedence) { + if (precedenceQueue) { // We need to track whether this boundary should wait on this resource or not. // Typically this resource should always exist since we either had it or just created // it. However, it's possible when you resume that the style has already been emitted // and then it wouldn't be recreated in the RenderState and there's no need to track // it again since we should've hoisted it to the shell already. if (renderState.boundaryResources) { - renderState.boundaryResources.precedences.add(stylesInPrecedence); + renderState.boundaryResources.precedences.add(precedenceQueue); } } @@ -4196,10 +4194,10 @@ let destinationHasCapacity = true; function flushStyleTagsLateForBoundary( this: Destination, - stylesInPrecedence: Precedence, + precedenceQueue: PrecedenceQueue, ) { - const rules = stylesInPrecedence.rules; - const hrefs = stylesInPrecedence.hrefs; + const rules = precedenceQueue.rules; + const hrefs = precedenceQueue.hrefs; if (__DEV__) { if (rules.length > 0 && hrefs.length === 0) { console.error( @@ -4210,7 +4208,7 @@ function flushStyleTagsLateForBoundary( let i = 0; if (hrefs.length) { writeChunk(this, lateStyleTagResourceOpen1); - writeChunk(this, stylesInPrecedence.precedence); + writeChunk(this, precedenceQueue.precedence); writeChunk(this, lateStyleTagResourceOpen2); for (; i < hrefs.length - 1; i++) { writeChunk(this, hrefs[i]); @@ -4310,21 +4308,21 @@ const styleTagResourceClose = stringToPrecomputedChunk(''); function flushPrecedenceInPreamble( this: Destination, - stylesInPrecedence: Precedence, + precedenceQueue: PrecedenceQueue, precedence: string, ) { - const hasStylesheets = stylesInPrecedence.sheets.size > 0; - stylesInPrecedence.sheets.forEach(flushStyleInPreamble, this); - stylesInPrecedence.sheets.clear(); + const hasStylesheets = precedenceQueue.sheets.size > 0; + precedenceQueue.sheets.forEach(flushStyleInPreamble, this); + precedenceQueue.sheets.clear(); - const rules = stylesInPrecedence.rules; - const hrefs = stylesInPrecedence.hrefs; + const rules = precedenceQueue.rules; + const hrefs = precedenceQueue.hrefs; // If we don't emit any stylesheets at this precedence we still need to maintain the precedence // order so even if there are no rules for style tags at this precedence we emit an empty style // tag with the data-precedence attribute if (!hasStylesheets || hrefs.length) { writeChunk(this, styleTagResourceOpen1); - writeChunk(this, stylesInPrecedence.precedence); + writeChunk(this, precedenceQueue.precedence); let i = 0; if (hrefs.length) { writeChunk(this, styleTagResourceOpen2); @@ -4365,11 +4363,10 @@ function preloadLateStyle(this: Destination, stylesheet: StylesheetResource) { function preloadLateStyles( this: Destination, - stylesInPrecedence: Precedence, - precedence: string, + precedenceQueue: PrecedenceQueue, ) { - stylesInPrecedence.sheets.forEach(preloadLateStyle, this); - stylesInPrecedence.sheets.clear(); + precedenceQueue.sheets.forEach(preloadLateStyle, this); + precedenceQueue.sheets.clear(); } // We don't bother reporting backpressure at the moment because we expect to @@ -5018,11 +5015,11 @@ type StylesheetResource = { }; export type BoundaryResources = { - precedences: Set, + precedences: Set, sheets: Set, }; -export type Precedence = { +export type PrecedenceQueue = { precedence: Chunk | PrecomputedChunk, rules: Array, hrefs: Array, @@ -5321,7 +5318,7 @@ function preinitStyle( precedence = precedence || 'default'; const key = getResourceKey(href); - let stylesInPrecedence = renderState.precedences.get(precedence); + let precedenceQueue = renderState.precedences.get(precedence); const hasKey = resumableState.styleResources.hasOwnProperty(key); const resourceState = hasKey ? resumableState.styleResources[key] @@ -5332,14 +5329,14 @@ function preinitStyle( // If this is the first time we've encountered this precedence we need // to create a PrecedenceQueue - if (!stylesInPrecedence) { - stylesInPrecedence = { + if (!precedenceQueue) { + precedenceQueue = { precedence: stringToChunk(escapeTextForBrowser(precedence)), rules: ([]: Array), hrefs: ([]: Array), sheets: (new Map(): Map), }; - renderState.precedences.set(precedence, stylesInPrecedence); + renderState.precedences.set(precedence, precedenceQueue); } const resource = { @@ -5381,7 +5378,7 @@ function preinitStyle( // We add the newly created resource to our PrecedenceQueue and if necessary // track the resource with the currently rendering boundary - stylesInPrecedence.sheets.set(key, resource); + precedenceQueue.sheets.set(key, resource); // Notify the request that there are resources to flush even if no work is currently happening flushResources(request); @@ -5604,18 +5601,18 @@ function adoptPreloadCredentials( if (target.integrity == null) target.integrity = preloadState[1]; } -function hoistPrecedenceDependency( +function hoistPrecedenceQueueDependency( this: BoundaryResources, - stylesInPrecedence: Precedence, + precedenceQueue: PrecedenceQueue, ) { - this.precedences.add(stylesInPrecedence); + this.precedences.add(precedenceQueue); } function hoistStylesheetDependency( this: BoundaryResources, - resource: StylesheetResource, + stylesheet: StylesheetResource, ) { - this.sheets.add(resource); + this.sheets.add(stylesheet); } export function hoistResources( @@ -5625,7 +5622,7 @@ export function hoistResources( const currentBoundaryResources = renderState.boundaryResources; if (currentBoundaryResources) { source.precedences.forEach( - hoistPrecedenceDependency, + hoistPrecedenceQueueDependency, currentBoundaryResources, ); source.sheets.forEach(hoistStylesheetDependency, currentBoundaryResources); diff --git a/packages/react-dom-bindings/src/server/ReactFizzConfigDOMLegacy.js b/packages/react-dom-bindings/src/server/ReactFizzConfigDOMLegacy.js index 471cf21cf71b0..5c397bd581de3 100644 --- a/packages/react-dom-bindings/src/server/ReactFizzConfigDOMLegacy.js +++ b/packages/react-dom-bindings/src/server/ReactFizzConfigDOMLegacy.js @@ -10,7 +10,7 @@ import type { ResumableState, BoundaryResources, - Precedence, + PrecedenceQueue, } from './ReactFizzConfigDOM'; import { @@ -54,7 +54,7 @@ export type RenderState = { fontPreloads: Set, highImagePreloads: Set, // usedImagePreloads: Set, - precedences: Map, + precedences: Map, bootstrapScripts: Set, scripts: Set, bulkPreloads: Set, From b88f4178b426f2786d8bbd937a1fac25c47800c5 Mon Sep 17 00:00:00 2001 From: Josh Story Date: Fri, 22 Sep 2023 09:37:38 -0700 Subject: [PATCH 05/10] Preloads need to be stashed during a render because they may need to be put in a different flushing queue or may need to be prevented from flushing. This only applies to a few resource types however, stylesheet, images, scripts, and moduleScripts. Another problem is that our resources are keyed using a compound key of href and the resource type but our preloadsMap only used href. This change also updates the preloads tracking object to have a similar compound key structure so there is not chance of collisions based on same href for different resources types (however rare this is in practice) --- .../src/server/ReactFizzConfigDOM.js | 235 ++++++++++-------- .../src/server/ReactFizzConfigDOMLegacy.js | 24 +- 2 files changed, 147 insertions(+), 112 deletions(-) diff --git a/packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js b/packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js index 99a8feb2736f5..409f317dc61ff 100644 --- a/packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js +++ b/packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js @@ -158,7 +158,12 @@ export type RenderState = { bulkPreloads: Set, // Temporarily keeps track of key to preload resources before shell flushes. - preloadsMap: Map, + preloads: { + images: Map, + stylesheets: Map, + scripts: Map, + moduleScripts: Map, + }, // Module-global-like reference for current boundary resources boundaryResources: ?BoundaryResources, @@ -392,7 +397,12 @@ export function createRenderState( scripts: new Set(), bulkPreloads: new Set(), - preloadsMap: new Map(), + preloads: { + images: new Map(), + stylesheets: new Map(), + scripts: new Map(), + moduleScripts: new Map(), + }, nonce, // like a module global for currently rendering boundary @@ -2230,7 +2240,7 @@ function pushLink( adoptPreloadCredentials(resource.props, preloadState); } - const preloadResource = renderState.preloadsMap.get(key); + const preloadResource = renderState.preloads.stylesheets.get(key); if (preloadResource && preloadResource.length > 0) { // The Preload for this resource was created in this render pass and has not flushed yet so // we need to clear it to avoid it flushing. @@ -2578,16 +2588,13 @@ function pushImg( // resumableState. const sizes = typeof props.sizes === 'string' ? props.sizes : undefined; const key = getImageResourceKey(src, srcSet, sizes); - const hasAsType = resumableState.unknownResources.hasOwnProperty('image'); - let hasKey = false; - if (!hasAsType) { - resumableState.unknownResources.image = {}; - } else { - hasKey = resumableState.unknownResources.image.hasOwnProperty(key); - } + const resources: ResumableState['unknownResources']['asType'] = + resumableState.unknownResources.hasOwnProperty('image') + ? resumableState.unknownResources.image + : (resumableState.unknownResources.image = {}); let resource: void | Resource; - if (!hasKey) { + if (!resources.hasOwnProperty(key)) { const preloadProps: PreloadProps = { rel: 'preload', as: 'image', @@ -2605,10 +2612,10 @@ function pushImg( referrerPolicy: props.referrerPolicy, }; resource = []; - resumableState.unknownResources.image[key] = PRELOAD_SIGIL; + resources[key] = PRELOAD_SIGIL; pushLinkImpl(resource, preloadProps); } else { - resource = renderState.preloadsMap.get(key); + resource = renderState.preloads.images.get(key); } if (resource) { if ( @@ -2955,10 +2962,14 @@ function pushScript( const key = getResourceKey(src); // We can make this