From f83294bb55cffe2de601c52f5eff6048ef8f3ae7 Mon Sep 17 00:00:00 2001 From: Josh Story Date: Tue, 1 Nov 2022 08:55:56 -0700 Subject: [PATCH 1/3] do not make resources inside svg context --- .../src/client/ReactDOMFloatClient.js | 8 +- .../src/client/ReactDOMHostConfig.js | 10 ++ .../src/server/ReactDOMServerFormatConfig.js | 27 +++++- .../src/__tests__/ReactDOMFloat-test.js | 93 ++++++++++++++++++- 4 files changed, 134 insertions(+), 4 deletions(-) diff --git a/packages/react-dom-bindings/src/client/ReactDOMFloatClient.js b/packages/react-dom-bindings/src/client/ReactDOMFloatClient.js index c9e8607eaaf7d..df34bdc3b292b 100644 --- a/packages/react-dom-bindings/src/client/ReactDOMFloatClient.js +++ b/packages/react-dom-bindings/src/client/ReactDOMFloatClient.js @@ -34,6 +34,8 @@ import { getHostContext, } from 'react-reconciler/src/ReactFiberHostContext'; import {getResourceFormOnly} from './validateDOMNesting'; +import {getNamespace} from './ReactDOMHostConfig'; +import {SVG_NAMESPACE} from '../shared/DOMNamespaces'; // The resource types we support. currently they match the form for the as argument. // In the future this may need to change, especially when modules / scripts are supported @@ -1397,10 +1399,14 @@ function insertResourceInstanceBefore( export function isHostResourceType(type: string, props: Props): boolean { let resourceFormOnly: boolean; + const hostContext = getHostContext(); + const namespace = getNamespace(hostContext); if (__DEV__) { - const hostContext = getHostContext(); resourceFormOnly = getResourceFormOnly(hostContext); } + if (namespace === SVG_NAMESPACE) { + return false; + } switch (type) { case 'base': case 'meta': diff --git a/packages/react-dom-bindings/src/client/ReactDOMHostConfig.js b/packages/react-dom-bindings/src/client/ReactDOMHostConfig.js index a4d394a986108..e7e69a59833bc 100644 --- a/packages/react-dom-bindings/src/client/ReactDOMHostConfig.js +++ b/packages/react-dom-bindings/src/client/ReactDOMHostConfig.js @@ -219,6 +219,16 @@ export function getPublicInstance(instance: Instance): Instance { return instance; } +export function getNamespace(hostContext: HostContext): string { + if (__DEV__) { + const hostContextDev: HostContextDev = (hostContext: any); + return hostContextDev.namespace; + } else { + const hostContextProd: HostContextProd = (hostContext: any); + return hostContextProd; + } +} + export function prepareForCommit(containerInfo: Container): Object | null { eventsEnabled = ReactBrowserEventEmitterIsEnabled(); selectionInformation = getSelectionInformation(); diff --git a/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js b/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js index dd6350d2a743b..1a273ce8c72b8 100644 --- a/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js +++ b/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js @@ -1197,10 +1197,12 @@ function pushBase( props: Object, responseState: ResponseState, textEmbedded: boolean, + insertionMode: InsertionMode, noscriptTagInScope: boolean, ): ReactNodeList { if ( enableFloat && + insertionMode !== SVG_MODE && !noscriptTagInScope && resourcesFromElement('base', props) ) { @@ -1222,10 +1224,12 @@ function pushMeta( props: Object, responseState: ResponseState, textEmbedded: boolean, + insertionMode: InsertionMode, noscriptTagInScope: boolean, ): ReactNodeList { if ( enableFloat && + insertionMode !== SVG_MODE && !noscriptTagInScope && resourcesFromElement('meta', props) ) { @@ -1247,9 +1251,15 @@ function pushLink( props: Object, responseState: ResponseState, textEmbedded: boolean, + insertionMode: InsertionMode, noscriptTagInScope: boolean, ): ReactNodeList { - if (enableFloat && !noscriptTagInScope && resourcesFromLink(props)) { + if ( + enableFloat && + insertionMode !== SVG_MODE && + !noscriptTagInScope && + resourcesFromLink(props) + ) { if (textEmbedded) { // This link follows text but we aren't writing a tag. while not as efficient as possible we need // to be safe and assume text will follow by inserting a textSeparator @@ -1371,6 +1381,7 @@ function pushTitle( target: Array, props: Object, responseState: ResponseState, + insertionMode: InsertionMode, noscriptTagInScope: boolean, ): ReactNodeList { if (__DEV__) { @@ -1415,6 +1426,7 @@ function pushTitle( if ( enableFloat && + insertionMode !== SVG_MODE && !noscriptTagInScope && resourcesFromElement('title', props) ) { @@ -1578,9 +1590,15 @@ function pushScript( props: Object, responseState: ResponseState, textEmbedded: boolean, + insertionMode: InsertionMode, noscriptTagInScope: boolean, ): null { - if (enableFloat && !noscriptTagInScope && resourcesFromScript(props)) { + if ( + enableFloat && + insertionMode !== SVG_MODE && + !noscriptTagInScope && + resourcesFromScript(props) + ) { if (textEmbedded) { // This link follows text but we aren't writing a tag. while not as efficient as possible we need // to be safe and assume text will follow by inserting a textSeparator @@ -1926,6 +1944,7 @@ export function pushStartInstance( target, props, responseState, + formatContext.insertionMode, formatContext.noscriptTagInScope, ) : pushStartTitle(target, props, responseState); @@ -1935,6 +1954,7 @@ export function pushStartInstance( props, responseState, textEmbedded, + formatContext.insertionMode, formatContext.noscriptTagInScope, ); case 'script': @@ -1944,6 +1964,7 @@ export function pushStartInstance( props, responseState, textEmbedded, + formatContext.insertionMode, formatContext.noscriptTagInScope, ) : pushStartGenericElement(target, props, type, responseState); @@ -1953,6 +1974,7 @@ export function pushStartInstance( props, responseState, textEmbedded, + formatContext.insertionMode, formatContext.noscriptTagInScope, ); case 'base': @@ -1961,6 +1983,7 @@ export function pushStartInstance( props, responseState, textEmbedded, + formatContext.insertionMode, formatContext.noscriptTagInScope, ); // Newline eating tags diff --git a/packages/react-dom/src/__tests__/ReactDOMFloat-test.js b/packages/react-dom/src/__tests__/ReactDOMFloat-test.js index 276cac2b3e734..a817e824c27f1 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFloat-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFloat-test.js @@ -5385,7 +5385,98 @@ describe('ReactDOMFloat', () => { }); }); - describe('noscript', () => { + describe('resource free contexts', () => { + // @gate enableFloat + it('should not turn descendants of svg into resources', async () => { + await actIntoEmptyDocument(() => { + const {pipe} = ReactDOMFizzServer.renderToPipeableStream( + + + + foo + + bar + + + + , + ); + pipe(writable); + }); + expect(getMeaningfulChildren(document)).toEqual( + + + + + foo + + bar + + + + , + ); + + let root = ReactDOMClient.hydrateRoot( + document, + + + + + foo + + bar + + + + , + ); + expect(Scheduler).toFlushWithoutYielding(); + expect(getMeaningfulChildren(document)).toEqual( + + + + + foo + + bar + + + + , + ); + + root.unmount(); + root = ReactDOMClient.createRoot(document); + root.render( + + + + + foo + + bar + + + + , + ); + expect(Scheduler).toFlushWithoutYielding(); + expect(getMeaningfulChildren(document)).toEqual( + + + + + foo + + bar + + + + , + ); + }); + // @gate enableFloat it('should not turn children of noscript into resources', async () => { function SomeResources() { From 64f470a6e6ae6b9b64afe9d23b6d69af3b02dde8 Mon Sep 17 00:00:00 2001 From: Josh Story Date: Tue, 1 Nov 2022 13:56:55 -0700 Subject: [PATCH 2/3] only restrict title in svg. add warnings for almost-resoures in svg --- .../src/client/ReactDOMFloatClient.js | 104 +++++++++--- .../src/client/ReactDOMHostConfig.js | 23 ++- .../src/server/ReactDOMServerFormatConfig.js | 25 +-- .../src/__tests__/ReactDOMFloat-test.js | 158 +++++++++++++++++- 4 files changed, 262 insertions(+), 48 deletions(-) diff --git a/packages/react-dom-bindings/src/client/ReactDOMFloatClient.js b/packages/react-dom-bindings/src/client/ReactDOMFloatClient.js index df34bdc3b292b..55a37d240351b 100644 --- a/packages/react-dom-bindings/src/client/ReactDOMFloatClient.js +++ b/packages/react-dom-bindings/src/client/ReactDOMFloatClient.js @@ -203,6 +203,28 @@ function getCurrentResourceRoot(): null | FloatRoot { return currentContainer ? currentContainer.getRootNode() : null; } +// This resource type constraint can be loosened. It really is everything except PreloadResource +// because that is the only one that does not have an optional instance type. Expand as needed. +function resetInstance(resource: ScriptResource | HeadResource) { + resource.instance = undefined; +} + +export function clearRootResources(rootContainer: Container): void { + const rootNode: FloatRoot = (rootContainer.getRootNode(): any); + const resources = getResourcesFromRoot(rootNode); + + // We can't actually delete the resource cache because this function is called + // during commit after we have rendered. Instead we detatch any instances from + // the Resource object if they are going to be cleared + + // Styles stay put + // Scripts get reset + resources.scripts.forEach(resetInstance); + // Head Resources get reset + resources.head.forEach(resetInstance); + // lastStructuredMeta stays put +} + // Preloads are somewhat special. Even if we don't have the Document // used by the root that is rendering a component trying to insert a preload // we can still seed the file cache by doing the preload on any document we have @@ -1079,7 +1101,14 @@ function acquireHeadResource(resource: HeadResource): Instance { props, root, ); - insertResourceInstanceBefore(root, instance, titles.item(0)); + const firstTitle = titles[0]; + insertResourceInstanceBefore( + root, + instance, + firstTitle && firstTitle.namespaceURI !== SVG_NAMESPACE + ? firstTitle + : null, + ); break; } case 'meta': { @@ -1399,20 +1428,21 @@ function insertResourceInstanceBefore( export function isHostResourceType(type: string, props: Props): boolean { let resourceFormOnly: boolean; - const hostContext = getHostContext(); - const namespace = getNamespace(hostContext); + let namespace: string; if (__DEV__) { + const hostContext = getHostContext(); resourceFormOnly = getResourceFormOnly(hostContext); - } - if (namespace === SVG_NAMESPACE) { - return false; + namespace = getNamespace(hostContext); } switch (type) { case 'base': - case 'meta': - case 'title': { + case 'meta': { return true; } + case 'title': { + const hostContext = getHostContext(); + return getNamespace(hostContext) !== SVG_NAMESPACE; + } case 'link': { const {onLoad, onError} = props; if (onLoad || onError) { @@ -1423,6 +1453,11 @@ export function isHostResourceType(type: string, props: Props): boolean { ' Try removing onLoad={...} and onError={...} or moving it into the root tag or' + ' somewhere in the .', ); + } else if (namespace === SVG_NAMESPACE) { + console.error( + 'Cannot render a with onLoad or onError listeners as a descendent of .' + + ' Try removing onLoad={...} and onError={...} or moving it above the ancestor.', + ); } } return false; @@ -1432,11 +1467,18 @@ export function isHostResourceType(type: string, props: Props): boolean { const {href, precedence, disabled} = props; if (__DEV__) { validateLinkPropsForStyleResource(props); - if (typeof precedence !== 'string' && resourceFormOnly) { - console.error( - 'Cannot render a outside the main document without knowing its precedence.' + - ' Consider adding precedence="default" or moving it into the root tag.', - ); + if (typeof precedence !== 'string') { + if (resourceFormOnly) { + console.error( + 'Cannot render a outside the main document without knowing its precedence.' + + ' Consider adding precedence="default" or moving it into the root tag.', + ); + } else if (namespace === SVG_NAMESPACE) { + console.error( + 'Cannot render a as a descendent of an element without knowing its precedence.' + + ' Consider adding precedence="default" or moving it above the ancestor.', + ); + } } } return ( @@ -1456,17 +1498,31 @@ export function isHostResourceType(type: string, props: Props): boolean { // precedence with these for style resources const {src, async, onLoad, onError} = props; if (__DEV__) { - if (async !== true && resourceFormOnly) { - console.error( - 'Cannot render a sync or defer tag.', - ); - } else if ((onLoad || onError) && resourceFormOnly) { - console.error( - 'Cannot render a