@samitouri / QOS-React / commits / 9236abdb5a

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

Josh Story committed Oct 22, 2022 at 15:11 UTC 9236abdb5ad61606c23f81eafd09f4fb51ea9a98
5 files changed +293 -39
packages/react-dom-bindings/src/server/ReactDOMFloatServer.js
+1 -2
@@ -645,9 +645,8 @@ export function resourcesFromElement(type: string, props: Props): boolean {
645 resources.headsMap.set(key, resource);
646 resources.headResources.add(resource);
647 }
648 - return true;
648 }
650 - return false;
649 + return true;
650 }
651 case 'meta': {
652 let key, propertyPath;
packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js
+169 -25
@@ -1294,21 +1294,102 @@ function pushStartMenuItem(
1294 return null;
1295 }
1296
1297 -function pushStartTitle(
1297 +function pushTitle(
1298 target: Array<Chunk | PrecomputedChunk>,
1299 props: Object,
1300 responseState: ResponseState,
1301 ): ReactNodeList {
1302 + if (__DEV__) {
1303 + const children = props.children;
1304 + const childForValidation =
1305 + Array.isArray(children) && children.length < 2
1306 + ? children[0] || null
1307 + : children;
1308 + if (Array.isArray(children) && children.length > 1) {
1309 + console.error(
1310 + 'A title element received an array with more than 1 element as children. ' +
1311 + 'In browsers title Elements can only have Text Nodes as children. If ' +
1312 + 'the children being rendered output more than a single text node in aggregate the browser ' +
1313 + 'will display markup and comments as text in the title and hydration will likely fail and ' +
1314 + 'fall back to client rendering',
1315 + );
1316 + } else if (
1317 + childForValidation != null &&
1318 + childForValidation.$$typeof != null
1319 + ) {
1320 + console.error(
1321 + 'A title element received a React element for children. ' +
1322 + 'In the browser title Elements can only have Text Nodes as children. If ' +
1323 + 'the children being rendered output more than a single text node in aggregate the browser ' +
1324 + 'will display markup and comments as text in the title and hydration will likely fail and ' +
1325 + 'fall back to client rendering',
1326 + );
1327 + } else if (
1328 + childForValidation != null &&
1329 + typeof childForValidation !== 'string' &&
1330 + typeof childForValidation !== 'number'
1331 + ) {
1332 + console.error(
1333 + 'A title element received a value that was not a string or number for children. ' +
1334 + 'In the browser title Elements can only have Text Nodes as children. If ' +
1335 + 'the children being rendered output more than a single text node in aggregate the browser ' +
1336 + 'will display markup and comments as text in the title and hydration will likely fail and ' +
1337 + 'fall back to client rendering',
1338 + );
1339 + }
1340 + }
1341 +
1342 if (enableFloat && resourcesFromElement('title', props)) {
1343 // We have converted this link exclusively to a resource and no longer
1344 // need to emit it
1345 return null;
1346 }
1347 + return pushTitleImpl(target, props, responseState);
1348 +}
1349
1308 - return pushStartTitleImpl(target, props, responseState);
1350 +function pushTitleImpl(
1351 + target: Array<Chunk | PrecomputedChunk>,
1352 + props: Object,
1353 + responseState: ResponseState,
1354 +): null {
1355 + target.push(startChunkForTag('title'));
1356 +
1357 + let children = null;
1358 + for (const propKey in props) {
1359 + if (hasOwnProperty.call(props, propKey)) {
1360 + const propValue = props[propKey];
1361 + if (propValue == null) {
1362 + continue;
1363 + }
1364 + switch (propKey) {
1365 + case 'children':
1366 + children = propValue;
1367 + break;
1368 + case 'dangerouslySetInnerHTML':
1369 + throw new Error(
1370 + '`dangerouslySetInnerHTML` does not make sense on <title>.',
1371 + );
1372 + // eslint-disable-next-line-no-fallthrough
1373 + default:
1374 + pushAttribute(target, responseState, propKey, propValue);
1375 + break;
1376 + }
1377 + }
1378 + }
1379 + target.push(endOfStartTag);
1380 +
1381 + const child =
1382 + Array.isArray(children) && children.length < 2
1383 + ? children[0] || null
1384 + : children;
1385 + if (typeof child === 'string' || typeof child === 'number') {
1386 + target.push(stringToChunk(escapeTextForBrowser(child)));
1387 + }
1388 + target.push(endTag1, stringToChunk('title'), endTag2);
1389 + return null;
1390 }
1391
1311 -function pushStartTitleImpl(
1392 +function pushStartTitle(
1393 target: Array<Chunk | PrecomputedChunk>,
1394 props: Object,
1395 responseState: ResponseState,
@@ -1340,7 +1421,7 @@ function pushStartTitleImpl(
1421 target.push(endOfStartTag);
1422
1423 if (__DEV__) {
1343 - const child =
1424 + const childForValidation =
1425 Array.isArray(children) && children.length < 2
1426 ? children[0] || null
1427 : children;
@@ -1352,7 +1433,10 @@ function pushStartTitleImpl(
1433 'will display markup and comments as text in the title and hydration will likely fail and ' +
1434 'fall back to client rendering',
1435 );
1355 - } else if (child != null && child.$$typeof != null) {
1436 + } else if (
1437 + childForValidation != null &&
1438 + childForValidation.$$typeof != null
1439 + ) {
1440 console.error(
1441 'A title element received a React element for children. ' +
1442 'In the browser title Elements can only have Text Nodes as children. If ' +
@@ -1361,9 +1445,9 @@ function pushStartTitleImpl(
1445 'fall back to client rendering',
1446 );
1447 } else if (
1364 - child != null &&
1365 - typeof child !== 'string' &&
1366 - typeof child !== 'number'
1448 + childForValidation != null &&
1449 + typeof childForValidation !== 'string' &&
1450 + typeof childForValidation !== 'number'
1451 ) {
1452 console.error(
1453 'A title element received a value that was not a string or number for children. ' +
@@ -1374,6 +1458,7 @@ function pushStartTitleImpl(
1458 );
1459 }
1460 }
1461 +
1462 return children;
1463 }
1464
@@ -1410,12 +1495,12 @@ function pushStartHtml(
1495 return pushStartGenericElement(target, props, tag, responseState);
1496 }
1497
1413 -function pushStartScript(
1498 +function pushScript(
1499 target: Array<Chunk | PrecomputedChunk>,
1500 props: Object,
1501 responseState: ResponseState,
1502 textEmbedded: boolean,
1418 -): ReactNodeList {
1503 +): null {
1504 if (enableFloat && resourcesFromScript(props)) {
1505 if (textEmbedded) {
1506 // 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(
1512 return null;
1513 }
1514
1430 - return pushStartGenericElement(target, props, 'script', responseState);
1515 + return pushScriptImpl(target, props, responseState);
1516 +}
1517 +
1518 +function pushScriptImpl(
1519 + target: Array<Chunk | PrecomputedChunk>,
1520 + props: Object,
1521 + responseState: ResponseState,
1522 +): null {
1523 + target.push(startChunkForTag('script'));
1524 +
1525 + let children = null;
1526 + let innerHTML = null;
1527 + for (const propKey in props) {
1528 + if (hasOwnProperty.call(props, propKey)) {
1529 + const propValue = props[propKey];
1530 + if (propValue == null) {
1531 + continue;
1532 + }
1533 + switch (propKey) {
1534 + case 'children':
1535 + children = propValue;
1536 + break;
1537 + case 'dangerouslySetInnerHTML':
1538 + innerHTML = propValue;
1539 + break;
1540 + default:
1541 + pushAttribute(target, responseState, propKey, propValue);
1542 + break;
1543 + }
1544 + }
1545 + }
1546 + target.push(endOfStartTag);
1547 +
1548 + if (__DEV__) {
1549 + if (children != null && typeof children !== 'string') {
1550 + const descriptiveStatement =
1551 + typeof children === 'number'
1552 + ? 'a number for children'
1553 + : Array.isArray(children)
1554 + ? 'an array for children'
1555 + : 'something unexpected for children';
1556 + console.error(
1557 + 'A script element was rendered with %s. If script element has children it must be a single string.' +
1558 + ' Consider using dangerouslySetInnerHTML or passing a plain string as children.',
1559 + descriptiveStatement,
1560 + );
1561 + }
1562 + }
1563 +
1564 + pushInnerHTML(target, innerHTML, children);
1565 + if (typeof children === 'string') {
1566 + target.push(stringToChunk(encodeHTMLTextNode(children)));
1567 + }
1568 + target.push(endTag1, stringToChunk('script'), endTag2);
1569 + return null;
1570 }
1571
1572 function pushStartGenericElement(
@@ -1703,11 +1842,15 @@ export function pushStartInstance(
1842 case 'menuitem':
1843 return pushStartMenuItem(target, props, responseState);
1844 case 'title':
1706 - return pushStartTitle(target, props, responseState);
1845 + return enableFloat
1846 + ? pushTitle(target, props, responseState)
1847 + : pushStartTitle(target, props, responseState);
1848 case 'link':
1849 return pushLink(target, props, responseState, textEmbedded);
1850 case 'script':
1710 - return pushStartScript(target, props, responseState, textEmbedded);
1851 + return enableFloat
1852 + ? pushScript(target, props, responseState, textEmbedded)
1853 + : pushStartGenericElement(target, props, type, responseState);
1854 case 'meta':
1855 return pushMeta(target, props, responseState, textEmbedded);
1856 // Newline eating tags
@@ -1777,9 +1920,19 @@ export function pushEndInstance(
1920 props: Object,
1921 ): void {
1922 switch (type) {
1923 + // When float is on we expect title and script tags to always be pushed in
1924 + // a unit and never return children. when we end up pushing the end tag we
1925 + // want to ensure there is no extra closing tag pushed
1926 + case 'title':
1927 + case 'script': {
1928 + if (!enableFloat) {
1929 + break;
1930 + }
1931 + }
1932 // Omitted close tags
1933 // TODO: Instead of repeating this switch we could try to pass a flag from above.
1934 // That would require returning a tuple. Which might be ok if it gets inlined.
1935 + // eslint-disable-next-line-no-fallthrough
1936 case 'area':
1937 case 'base':
1938 case 'br':
@@ -2396,8 +2549,7 @@ export function writeInitialResources(
2549
2550 scripts.forEach(r => {
2551 // should never be flushed already
2399 - pushStartGenericElement(target, r.props, 'script', responseState);
2400 - pushEndInstance(target, target, 'script', r.props);
2552 + pushScriptImpl(target, r.props, responseState);
2553 r.flushed = true;
2554 r.hint.flushed = true;
2555 });
@@ -2415,11 +2567,7 @@ export function writeInitialResources(
2567 headResources.forEach(r => {
2568 switch (r.type) {
2569 case 'title': {
2418 - pushStartTitleImpl(target, r.props, responseState);
2419 - if (typeof r.props.children === 'string') {
2420 - target.push(stringToChunk(escapeTextForBrowser(r.props.children)));
2421 - }
2422 - pushEndInstance(target, target, 'title', r.props);
2570 + pushTitleImpl(target, r.props, responseState);
2571 break;
2572 }
2573 case 'meta': {
@@ -2516,11 +2664,7 @@ export function writeImmediateResources(
2664 headResources.forEach(r => {
2665 switch (r.type) {
2666 case 'title': {
2519 - pushStartTitleImpl(target, r.props, responseState);
2520 - if (typeof r.props.children === 'string') {
2521 - target.push(stringToChunk(escapeTextForBrowser(r.props.children)));
2522 - }
2523 - pushEndInstance(target, target, 'title', r.props);
2667 + pushTitleImpl(target, r.props, responseState);
2668 break;
2669 }
2670 case 'meta': {
packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
+96 -10
@@ -4957,9 +4957,14 @@ describe('ReactDOMFizzServer', () => {
4957 expect(mockError).not.toHaveBeenCalled();
4958 }
4959
4960 - expect(getVisibleChildren(container)).toEqual(
4961 - <title>{'hello1<!-- -->hello2'}</title>,
4962 - );
4960 + if (gate(flags => flags.enableFloat)) {
4961 + // This title was invalid so it is not emitted
4962 + expect(getVisibleChildren(container)).toEqual(undefined);
4963 + } else {
4964 + expect(getVisibleChildren(container)).toEqual(
4965 + <title>{'hello1<!-- -->hello2'}</title>,
4966 + );
4967 + }
4968
4969 const errors = [];
4970 ReactDOMClient.hydrateRoot(container, <App />, {
@@ -4970,11 +4975,8 @@ describe('ReactDOMFizzServer', () => {
4975 expect(Scheduler).toFlushAndYield([]);
4976 if (gate(flags => flags.enableFloat)) {
4977 expect(errors).toEqual([]);
4973 - // with float, the title doesn't render on the client because it is not a simple child
4974 - // we end up seeing the server rendered title
4975 - expect(getVisibleChildren(container)).toEqual(
4976 - <title>{'hello1<!-- -->hello2'}</title>,
4977 - );
4978 + // with float, the title doesn't render on the client or on the server
4979 + expect(getVisibleChildren(container)).toEqual(undefined);
4980 } else {
4981 expect(errors).toEqual(
4982 [
@@ -5039,7 +5041,12 @@ describe('ReactDOMFizzServer', () => {
5041 expect(mockError).not.toHaveBeenCalled();
5042 }
5043
5042 - expect(getVisibleChildren(container)).toEqual(<title>hello</title>);
5044 + if (gate(flags => flags.enableFloat)) {
5045 + // invalid titles are not emitted on the server when float is on
5046 + expect(getVisibleChildren(container)).toEqual(undefined);
5047 + } else {
5048 + expect(getVisibleChildren(container)).toEqual(<title>hello</title>);
5049 + }
5050
5051 const errors = [];
5052 ReactDOMClient.hydrateRoot(container, <App />, {
@@ -5049,7 +5056,12 @@ describe('ReactDOMFizzServer', () => {
5056 });
5057 expect(Scheduler).toFlushAndYield([]);
5058 expect(errors).toEqual([]);
5052 - expect(getVisibleChildren(container)).toEqual(<title>hello</title>);
5059 + if (gate(flags => flags.enableFloat)) {
5060 + // invalid titles are not emitted on the server when float is on
5061 + expect(getVisibleChildren(container)).toEqual(undefined);
5062 + } else {
5063 + expect(getVisibleChildren(container)).toEqual(<title>hello</title>);
5064 + }
5065 } finally {
5066 console.error = originalConsoleError;
5067 }
@@ -5414,4 +5426,78 @@ describe('ReactDOMFizzServer', () => {
5426 expect(getVisibleChildren(container)).toEqual(<span />);
5427 });
5428 });
5429 +
5430 + it('can render scripts with simple children', async () => {
5431 + await actIntoEmptyDocument(async () => {
5432 + const {pipe} = ReactDOMFizzServer.renderToPipeableStream(
5433 + <html>
5434 + <body>
5435 + <script>{'try { foo() } catch (e) {} ;'}</script>
5436 + </body>
5437 + </html>,
5438 + );
5439 + pipe(writable);
5440 + });
5441 +
5442 + expect(document.documentElement.outerHTML).toEqual(
5443 + '<html><head></head><body><script>try { foo() } catch (e) {} ;</script></body></html>',
5444 + );
5445 + });
5446 +
5447 + // @gate enableFloat
5448 + it('warns if script has complex children', async () => {
5449 + function MyScript() {
5450 + return 'bar();';
5451 + }
5452 + const originalConsoleError = console.error;
5453 + const mockError = jest.fn();
5454 + console.error = (...args) => {
5455 + mockError(...args.map(normalizeCodeLocInfo));
5456 + };
5457 +
5458 + try {
5459 + await actIntoEmptyDocument(async () => {
5460 + const {pipe} = ReactDOMFizzServer.renderToPipeableStream(
5461 + <html>
5462 + <body>
5463 + <script>{2}</script>
5464 + <script>
5465 + {[
5466 + 'try { foo() } catch (e) {} ;',
5467 + 'try { bar() } catch (e) {} ;',
5468 + ]}
5469 + </script>
5470 + <script>
5471 + <MyScript />
5472 + </script>
5473 + </body>
5474 + </html>,
5475 + );
5476 + pipe(writable);
5477 + });
5478 +
5479 + if (__DEV__) {
5480 + expect(mockError.mock.calls.length).toBe(3);
5481 + expect(mockError.mock.calls[0]).toEqual([
5482 + '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',
5483 + 'a number for children',
5484 + componentStack(['script', 'body', 'html']),
5485 + ]);
5486 + expect(mockError.mock.calls[1]).toEqual([
5487 + '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',
5488 + 'an array for children',
5489 + componentStack(['script', 'body', 'html']),
5490 + ]);
5491 + expect(mockError.mock.calls[2]).toEqual([
5492 + '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',
5493 + 'something unexpected for children',
5494 + componentStack(['script', 'body', 'html']),
5495 + ]);
5496 + } else {
5497 + expect(mockError.mock.calls.length).toBe(0);
5498 + }
5499 + } finally {
5500 + console.error = originalConsoleError;
5501 + }
5502 + });
5503 });
packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js
+1 -2
@@ -486,7 +486,6 @@ describe('ReactDOMFizzServerBrowser', () => {
486 });
487
488 // https://github.com/facebook/react/pull/25534/files - fix transposed escape functions
489 - // @gate enableFloat
489 it('should encode title properly', async () => {
490 const stream = await ReactDOMFizzServer.renderToReadableStream(
491 <html>
@@ -499,7 +498,7 @@ describe('ReactDOMFizzServerBrowser', () => {
498
499 const result = await readResult(stream);
500 expect(result).toEqual(
502 - '<!DOCTYPE html><html><head><title>foo</title></title></head><body>bar</body></html>',
501 + '<!DOCTYPE html><html><head><title>foo</title></head><body>bar</body></html>',
502 );
503 });
504 });
packages/react-dom/src/__tests__/ReactDOMFloat-test.js
+26
@@ -382,6 +382,32 @@ describe('ReactDOMFloat', () => {
382 );
383 });
384
385 + // @gate enableFloat
386 + it('does not emit closing tags in out of order position when rendering a non-void resource type', async () => {
387 + const chunks = [];
388 +
389 + writable.on('data', chunk => {
390 + chunks.push(chunk);
391 + });
392 +
393 + await actIntoEmptyDocument(() => {
394 + const {pipe} = ReactDOMFizzServer.renderToPipeableStream(
395 + <>
396 + <title>foo</title>
397 + <html>
398 + <body>bar</body>
399 + </html>
400 + <script async={true} src="foo" />
401 + </>,
402 + );
403 + pipe(writable);
404 + });
405 + expect(chunks).toEqual([
406 + '<!DOCTYPE html><html><script async="" src="foo"></script><title>foo</title><body>bar',
407 + '</body></html>',
408 + ]);
409 + });
410 +
411 describe('HostResource', () => {
412 // @gate enableFloat
413 it('warns when you update props to an invalid type', async () => {