From 9236abdb5ad61606c23f81eafd09f4fb51ea9a98 Mon Sep 17 00:00:00 2001 From: Josh Story Date: Sat, 22 Oct 2022 15:11:26 -0700 Subject: [PATCH] when float is enabled only push title and script as a single unit (#25536) replaces: https://github.com/facebook/react/pull/25535 This takes a more huerstic based approach with no new conditionals on the hot path of fizz rendering. If float is enabled * title and script can only have simple children * if non-simple children are found they will be ignored * title and script are pushed in a single unit during pushStartInstance including their children and closing tags If float is not enabled * the original pushing behaviors are in place and you can have complex children but you will get warnings --- .../src/server/ReactDOMFloatServer.js | 3 +- .../src/server/ReactDOMServerFormatConfig.js | 194 +++++++++++++++--- .../src/__tests__/ReactDOMFizzServer-test.js | 106 +++++++++- .../ReactDOMFizzServerBrowser-test.js | 3 +- .../src/__tests__/ReactDOMFloat-test.js | 26 +++ 5 files changed, 293 insertions(+), 39 deletions(-) diff --git a/packages/react-dom-bindings/src/server/ReactDOMFloatServer.js b/packages/react-dom-bindings/src/server/ReactDOMFloatServer.js index 91a37bfc1f0dc..65a843df586b5 100644 --- a/packages/react-dom-bindings/src/server/ReactDOMFloatServer.js +++ b/packages/react-dom-bindings/src/server/ReactDOMFloatServer.js @@ -645,9 +645,8 @@ export function resourcesFromElement(type: string, props: Props): boolean { resources.headsMap.set(key, resource); resources.headResources.add(resource); } - return true; } - return false; + return true; } case 'meta': { let key, propertyPath; diff --git a/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js b/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js index ba6940fdfd7a4..ba53d58310392 100644 --- a/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js +++ b/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js @@ -1294,21 +1294,102 @@ function pushStartMenuItem( return null; } -function pushStartTitle( +function pushTitle( target: Array, props: Object, responseState: ResponseState, ): ReactNodeList { + if (__DEV__) { + const children = props.children; + const childForValidation = + Array.isArray(children) && children.length < 2 + ? children[0] || null + : children; + if (Array.isArray(children) && children.length > 1) { + console.error( + 'A title element received an array with more than 1 element as children. ' + + 'In browsers title Elements can only have Text Nodes as children. If ' + + 'the children being rendered output more than a single text node in aggregate the browser ' + + 'will display markup and comments as text in the title and hydration will likely fail and ' + + 'fall back to client rendering', + ); + } else if ( + childForValidation != null && + childForValidation.$$typeof != null + ) { + console.error( + 'A title element received a React element for children. ' + + 'In the browser title Elements can only have Text Nodes as children. If ' + + 'the children being rendered output more than a single text node in aggregate the browser ' + + 'will display markup and comments as text in the title and hydration will likely fail and ' + + 'fall back to client rendering', + ); + } else if ( + childForValidation != null && + typeof childForValidation !== 'string' && + typeof childForValidation !== 'number' + ) { + console.error( + 'A title element received a value that was not a string or number for children. ' + + 'In the browser title Elements can only have Text Nodes as children. If ' + + 'the children being rendered output more than a single text node in aggregate the browser ' + + 'will display markup and comments as text in the title and hydration will likely fail and ' + + 'fall back to client rendering', + ); + } + } + if (enableFloat && resourcesFromElement('title', props)) { // We have converted this link exclusively to a resource and no longer // need to emit it return null; } + return pushTitleImpl(target, props, responseState); +} - return pushStartTitleImpl(target, props, responseState); +function pushTitleImpl( + target: Array, + props: Object, + responseState: ResponseState, +): null { + target.push(startChunkForTag('title')); + + let children = null; + for (const propKey in props) { + if (hasOwnProperty.call(props, propKey)) { + const propValue = props[propKey]; + if (propValue == null) { + continue; + } + switch (propKey) { + case 'children': + children = propValue; + break; + case 'dangerouslySetInnerHTML': + throw new Error( + '`dangerouslySetInnerHTML` does not make sense on .', + ); + // eslint-disable-next-line-no-fallthrough + default: + pushAttribute(target, responseState, propKey, propValue); + break; + } + } + } + target.push(endOfStartTag); + + const child = + Array.isArray(children) && children.length < 2 + ? children[0] || null + : children; + if (typeof child === 'string' || typeof child === 'number') { + target.push(stringToChunk(escapeTextForBrowser(child))); + } + target.push(endTag1, stringToChunk('title'), endTag2); + return null; } -function pushStartTitleImpl( +function pushStartTitle( target: Array<Chunk | PrecomputedChunk>, props: Object, responseState: ResponseState, @@ -1340,7 +1421,7 @@ function pushStartTitleImpl( target.push(endOfStartTag); if (__DEV__) { - const child = + const childForValidation = Array.isArray(children) && children.length < 2 ? children[0] || null : children; @@ -1352,7 +1433,10 @@ function pushStartTitleImpl( 'will display markup and comments as text in the title and hydration will likely fail and ' + 'fall back to client rendering', ); - } else if (child != null && child.$$typeof != null) { + } else if ( + childForValidation != null && + childForValidation.$$typeof != null + ) { console.error( 'A title element received a React element for children. ' + 'In the browser title Elements can only have Text Nodes as children. If ' + @@ -1361,9 +1445,9 @@ function pushStartTitleImpl( 'fall back to client rendering', ); } else if ( - child != null && - typeof child !== 'string' && - typeof child !== 'number' + childForValidation != null && + typeof childForValidation !== 'string' && + typeof childForValidation !== 'number' ) { console.error( 'A title element received a value that was not a string or number for children. ' + @@ -1374,6 +1458,7 @@ function pushStartTitleImpl( ); } } + return children; } @@ -1410,12 +1495,12 @@ function pushStartHtml( return pushStartGenericElement(target, props, tag, responseState); } -function pushStartScript( +function pushScript( target: Array<Chunk | PrecomputedChunk>, props: Object, responseState: ResponseState, textEmbedded: boolean, -): ReactNodeList { +): null { if (enableFloat && resourcesFromScript(props)) { if (textEmbedded) { // This link follows text but we aren't writing a tag. while not as efficient as possible we need @@ -1427,7 +1512,61 @@ function pushStartScript( return null; } - return pushStartGenericElement(target, props, 'script', responseState); + return pushScriptImpl(target, props, responseState); +} + +function pushScriptImpl( + target: Array<Chunk | PrecomputedChunk>, + props: Object, + responseState: ResponseState, +): null { + target.push(startChunkForTag('script')); + + let children = null; + let innerHTML = null; + for (const propKey in props) { + if (hasOwnProperty.call(props, propKey)) { + const propValue = props[propKey]; + if (propValue == null) { + continue; + } + switch (propKey) { + case 'children': + children = propValue; + break; + case 'dangerouslySetInnerHTML': + innerHTML = propValue; + break; + default: + pushAttribute(target, responseState, propKey, propValue); + break; + } + } + } + target.push(endOfStartTag); + + if (__DEV__) { + if (children != null && typeof children !== 'string') { + const descriptiveStatement = + typeof children === 'number' + ? 'a number for children' + : Array.isArray(children) + ? 'an array for children' + : 'something unexpected for children'; + console.error( + 'A script element was rendered with %s. If script element has children it must be a single string.' + + ' Consider using dangerouslySetInnerHTML or passing a plain string as children.', + descriptiveStatement, + ); + } + } + + pushInnerHTML(target, innerHTML, children); + if (typeof children === 'string') { + target.push(stringToChunk(encodeHTMLTextNode(children))); + } + target.push(endTag1, stringToChunk('script'), endTag2); + return null; } function pushStartGenericElement( @@ -1703,11 +1842,15 @@ export function pushStartInstance( case 'menuitem': return pushStartMenuItem(target, props, responseState); case 'title': - return pushStartTitle(target, props, responseState); + return enableFloat + ? pushTitle(target, props, responseState) + : pushStartTitle(target, props, responseState); case 'link': return pushLink(target, props, responseState, textEmbedded); case 'script': - return pushStartScript(target, props, responseState, textEmbedded); + return enableFloat + ? pushScript(target, props, responseState, textEmbedded) + : pushStartGenericElement(target, props, type, responseState); case 'meta': return pushMeta(target, props, responseState, textEmbedded); // Newline eating tags @@ -1777,9 +1920,19 @@ export function pushEndInstance( props: Object, ): void { switch (type) { + // When float is on we expect title and script tags to always be pushed in + // a unit and never return children. when we end up pushing the end tag we + // want to ensure there is no extra closing tag pushed + case 'title': + case 'script': { + if (!enableFloat) { + break; + } + } // Omitted close tags // TODO: Instead of repeating this switch we could try to pass a flag from above. // That would require returning a tuple. Which might be ok if it gets inlined. + // eslint-disable-next-line-no-fallthrough case 'area': case 'base': case 'br': @@ -2396,8 +2549,7 @@ export function writeInitialResources( scripts.forEach(r => { // should never be flushed already - pushStartGenericElement(target, r.props, 'script', responseState); - pushEndInstance(target, target, 'script', r.props); + pushScriptImpl(target, r.props, responseState); r.flushed = true; r.hint.flushed = true; }); @@ -2415,11 +2567,7 @@ export function writeInitialResources( headResources.forEach(r => { switch (r.type) { case 'title': { - pushStartTitleImpl(target, r.props, responseState); - if (typeof r.props.children === 'string') { - target.push(stringToChunk(escapeTextForBrowser(r.props.children))); - } - pushEndInstance(target, target, 'title', r.props); + pushTitleImpl(target, r.props, responseState); break; } case 'meta': { @@ -2516,11 +2664,7 @@ export function writeImmediateResources( headResources.forEach(r => { switch (r.type) { case 'title': { - pushStartTitleImpl(target, r.props, responseState); - if (typeof r.props.children === 'string') { - target.push(stringToChunk(escapeTextForBrowser(r.props.children))); - } - pushEndInstance(target, target, 'title', r.props); + pushTitleImpl(target, r.props, responseState); break; } case 'meta': { diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js index 1153ac42498dc..e19542b62b9bd 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js @@ -4957,9 +4957,14 @@ describe('ReactDOMFizzServer', () => { expect(mockError).not.toHaveBeenCalled(); } - expect(getVisibleChildren(container)).toEqual( - <title>{'hello1<!-- -->hello2'}, - ); + if (gate(flags => flags.enableFloat)) { + // This title was invalid so it is not emitted + expect(getVisibleChildren(container)).toEqual(undefined); + } else { + expect(getVisibleChildren(container)).toEqual( + {'hello1<!-- -->hello2'}, + ); + } const errors = []; ReactDOMClient.hydrateRoot(container, , { @@ -4970,11 +4975,8 @@ describe('ReactDOMFizzServer', () => { expect(Scheduler).toFlushAndYield([]); if (gate(flags => flags.enableFloat)) { expect(errors).toEqual([]); - // with float, the title doesn't render on the client because it is not a simple child - // we end up seeing the server rendered title - expect(getVisibleChildren(container)).toEqual( - {'hello1<!-- -->hello2'}, - ); + // with float, the title doesn't render on the client or on the server + expect(getVisibleChildren(container)).toEqual(undefined); } else { expect(errors).toEqual( [ @@ -5039,7 +5041,12 @@ describe('ReactDOMFizzServer', () => { expect(mockError).not.toHaveBeenCalled(); } - expect(getVisibleChildren(container)).toEqual(hello); + if (gate(flags => flags.enableFloat)) { + // invalid titles are not emitted on the server when float is on + expect(getVisibleChildren(container)).toEqual(undefined); + } else { + expect(getVisibleChildren(container)).toEqual(hello); + } const errors = []; ReactDOMClient.hydrateRoot(container, , { @@ -5049,7 +5056,12 @@ describe('ReactDOMFizzServer', () => { }); expect(Scheduler).toFlushAndYield([]); expect(errors).toEqual([]); - expect(getVisibleChildren(container)).toEqual(hello); + if (gate(flags => flags.enableFloat)) { + // invalid titles are not emitted on the server when float is on + expect(getVisibleChildren(container)).toEqual(undefined); + } else { + expect(getVisibleChildren(container)).toEqual(hello); + } } finally { console.error = originalConsoleError; } @@ -5414,4 +5426,78 @@ describe('ReactDOMFizzServer', () => { expect(getVisibleChildren(container)).toEqual(); }); }); + + it('can render scripts with simple children', async () => { + await actIntoEmptyDocument(async () => { + const {pipe} = ReactDOMFizzServer.renderToPipeableStream( + + + + + , + ); + pipe(writable); + }); + + expect(document.documentElement.outerHTML).toEqual( + '', + ); + }); + + // @gate enableFloat + it('warns if script has complex children', async () => { + function MyScript() { + return 'bar();'; + } + const originalConsoleError = console.error; + const mockError = jest.fn(); + console.error = (...args) => { + mockError(...args.map(normalizeCodeLocInfo)); + }; + + try { + await actIntoEmptyDocument(async () => { + const {pipe} = ReactDOMFizzServer.renderToPipeableStream( + + + + + + + , + ); + pipe(writable); + }); + + if (__DEV__) { + expect(mockError.mock.calls.length).toBe(3); + expect(mockError.mock.calls[0]).toEqual([ + 'Warning: A script element was rendered with %s. If script element has children it must be a single string. Consider using dangerouslySetInnerHTML or passing a plain string as children.%s', + 'a number for children', + componentStack(['script', 'body', 'html']), + ]); + expect(mockError.mock.calls[1]).toEqual([ + 'Warning: A script element was rendered with %s. If script element has children it must be a single string. Consider using dangerouslySetInnerHTML or passing a plain string as children.%s', + 'an array for children', + componentStack(['script', 'body', 'html']), + ]); + expect(mockError.mock.calls[2]).toEqual([ + 'Warning: A script element was rendered with %s. If script element has children it must be a single string. Consider using dangerouslySetInnerHTML or passing a plain string as children.%s', + 'something unexpected for children', + componentStack(['script', 'body', 'html']), + ]); + } else { + expect(mockError.mock.calls.length).toBe(0); + } + } finally { + console.error = originalConsoleError; + } + }); }); diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js index 1b1357b0e9cea..fc795649ee956 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js @@ -486,7 +486,6 @@ describe('ReactDOMFizzServerBrowser', () => { }); // https://github.com/facebook/react/pull/25534/files - fix transposed escape functions - // @gate enableFloat it('should encode title properly', async () => { const stream = await ReactDOMFizzServer.renderToReadableStream( @@ -499,7 +498,7 @@ describe('ReactDOMFizzServerBrowser', () => { const result = await readResult(stream); expect(result).toEqual( - 'foobar', + 'foobar', ); }); }); diff --git a/packages/react-dom/src/__tests__/ReactDOMFloat-test.js b/packages/react-dom/src/__tests__/ReactDOMFloat-test.js index 7c31700cdafed..4f68167165ac3 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFloat-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFloat-test.js @@ -382,6 +382,32 @@ describe('ReactDOMFloat', () => { ); }); + // @gate enableFloat + it('does not emit closing tags in out of order position when rendering a non-void resource type', async () => { + const chunks = []; + + writable.on('data', chunk => { + chunks.push(chunk); + }); + + await actIntoEmptyDocument(() => { + const {pipe} = ReactDOMFizzServer.renderToPipeableStream( + <> + foo + + bar + + foobar', + '', + ]); + }); + describe('HostResource', () => { // @gate enableFloat it('warns when you update props to an invalid type', async () => {