From fc929cf4ead35f99c4e9612a95e8a0bb8f5df25d Mon Sep 17 00:00:00 2001 From: Josh Story Date: Thu, 15 Jun 2023 14:50:22 -0700 Subject: [PATCH] [Float][Fizz][Fiber] support imagesrcset and imagesizes for `ReactDOM.preload()` (#26940) For float methods the href argument is usually all we need to uniquely key the request. However when preloading responsive images it is possible that you may need more than one preload for differing imagesizes attributes. When using imagesrcset for preloads the href attribute acts more like a fallback href. For keying purposes the imagesrcset becomes the primary key conceptually. This change updates the keying logic for `ReactDOM.preload()` when you pass `{as: "image"}` 1. If `options.imageSrcSet` is a non-emtpy string the key is defined as `options.imageSrcSet + options.imageSizes`. The `href` argument is still required but does not participate in keying. 2. If `options.imageSrcSet` is empty, missing, or an invalid format the key is defined as the `href`. Changing the `options.imageSizes` does not affect the key as this option is inert when not using `options.imageSrcSet` Additionally, currently there is a bug in webkit (Safari) that causes preload links to fail to use imageSrcSet and fallback to href even when the browser will correctly resolve srcset on an `` tag. Because the drawbacks of preloading the wrong image (href over imagesrcset) in a modern browser outweight the drawbacks of not preloading anything for responsive images in browsers that do not support srcset at all we will omit the `href` attribute whenever `options.imageSrcSet` is provided. We still require you provide an href since we want to be able to revert this behavior once all major browsers support it bug link: https://bugs.webkit.org/show_bug.cgi?id=231150 --- .../src/client/ReactFiberConfigDOM.js | 65 +++++++- .../src/server/ReactFizzConfigDOM.js | 64 ++++++-- .../src/shared/ReactDOMResourceValidation.js | 54 ------- .../src/__tests__/ReactDOMFloat-test.js | 149 +++++++++++++++++- .../react-dom/src/shared/ReactDOMTypes.js | 2 + .../src/__tests__/ReactFlightDOM-test.js | 4 +- .../__tests__/ReactFlightDOMBrowser-test.js | 2 +- 7 files changed, 254 insertions(+), 86 deletions(-) diff --git a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js index 6e884c0b1f..94698a8d52 100644 --- a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js +++ b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js @@ -105,7 +105,6 @@ import { } from 'react-reconciler/src/ReactWorkTags'; import {listenToAllSupportedEvents} from '../events/DOMPluginEventSystem'; import { - validatePreloadArguments, validatePreinitArguments, validateLinkPropsForStyleResource, getValueDescriptorExpectingObjectForWarning, @@ -2016,7 +2015,7 @@ type ScriptProps = { type PreloadProps = { rel: 'preload', - href: string, + href: ?string, [string]: mixed, }; @@ -2167,7 +2166,29 @@ function preload(href: string, options: PreloadOptions) { return; } if (__DEV__) { - validatePreloadArguments(href, options); + // TODO move this to ReactDOMFloat and expose a stricter function interface or possibly + // typed functions (preloadImage, preloadStyle, ...) + let encountered = ''; + if (typeof href !== 'string' || !href) { + encountered += `The \`href\` argument encountered was ${getValueDescriptorExpectingObjectForWarning( + href, + )}.`; + } + if (options == null || typeof options !== 'object') { + encountered += `The \`options\` argument encountered was ${getValueDescriptorExpectingObjectForWarning( + options, + )}.`; + } else if (typeof options.as !== 'string' || !options.as) { + encountered += `The \`as\` option encountered was ${getValueDescriptorExpectingObjectForWarning( + options.as, + )}.`; + } + if (encountered) { + console.error( + 'ReactDOM.preload(): Expected two arguments, a non-empty `href` string and an `options` object with an `as` property valid for a `` tag. %s', + encountered, + ); + } } const ownerDocument = getDocumentForImperativeFloatMethods(); if ( @@ -2175,13 +2196,35 @@ function preload(href: string, options: PreloadOptions) { href && typeof options === 'object' && options !== null && + typeof options.as === 'string' && + options.as && ownerDocument ) { const as = options.as; - const limitedEscapedHref = - escapeSelectorAttributeValueInsideDoubleQuotes(href); - const preloadSelector = `link[rel="preload"][as="${as}"][href="${limitedEscapedHref}"]`; - + let preloadSelector = `link[rel="preload"][as="${escapeSelectorAttributeValueInsideDoubleQuotes( + as, + )}"]`; + if (as === 'image') { + const {imageSrcSet, imageSizes} = options; + if (typeof imageSrcSet === 'string' && imageSrcSet !== '') { + preloadSelector += `[imagesrcset="${escapeSelectorAttributeValueInsideDoubleQuotes( + imageSrcSet, + )}"]`; + if (typeof imageSizes === 'string') { + preloadSelector += `[imagesizes="${escapeSelectorAttributeValueInsideDoubleQuotes( + imageSizes, + )}"]`; + } + } else { + preloadSelector += `[href="${escapeSelectorAttributeValueInsideDoubleQuotes( + href, + )}"]`; + } + } else { + preloadSelector += `[href="${escapeSelectorAttributeValueInsideDoubleQuotes( + href, + )}"]`; + } // Some preloads are keyed under their selector. This happens when the preload is for // an arbitrary type. Other preloads are keyed under the resource key they represent a preload for. // Here we figure out which key to use to determine if we have a preload already. @@ -2227,14 +2270,20 @@ function preloadPropsFromPreloadOptions( options: PreloadOptions, ): PreloadProps { return { - href, rel: 'preload', as, + // There is a bug in Safari where imageSrcSet is not respected on preload links + // so we omit the href here if we have imageSrcSet b/c safari will load the wrong image. + // This harms older browers that do not support imageSrcSet by making their preloads not work + // but this population is shrinking fast and is already small so we accept this tradeoff. + href: as === 'image' && options.imageSrcSet ? undefined : href, crossOrigin: as === 'font' ? '' : options.crossOrigin, integrity: options.integrity, type: options.type, nonce: options.nonce, fetchPriority: options.fetchPriority, + imageSrcSet: options.imageSrcSet, + imageSizes: options.imageSizes, }; } diff --git a/packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js b/packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js index 4d23f0ff1a..b6e1e90507 100644 --- a/packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js +++ b/packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js @@ -4819,12 +4819,12 @@ type PreconnectResource = TResource<'preconnect', null>; type PreloadAsProps = { rel: 'preload', as: string, - href: string, + href: ?string, [string]: mixed, }; type PreloadModuleProps = { rel: 'modulepreload', - href: string, + href: ?string, [string]: mixed, }; type PreloadProps = PreloadAsProps | PreloadModuleProps; @@ -5063,20 +5063,25 @@ export function preload(href: string, options: PreloadOptions) { } const resources = getResources(request); if (__DEV__) { + let encountered = ''; if (typeof href !== 'string' || !href) { + encountered += ` The \`href\` argument encountered was ${getValueDescriptorExpectingObjectForWarning( + href, + )}.`; + } + if (options == null || typeof options !== 'object') { + encountered += ` The \`options\` argument encountered was ${getValueDescriptorExpectingObjectForWarning( + options, + )}.`; + } else if (typeof options.as !== 'string' || !options.as) { + encountered += ` The \`as\` option encountered was ${getValueDescriptorExpectingObjectForWarning( + options.as, + )}.`; + } + if (encountered) { console.error( - 'ReactDOM.preload(): Expected the `href` argument (first) to be a non-empty string but encountered %s instead.', - getValueDescriptorExpectingObjectForWarning(href), - ); - } else if (options == null || typeof options !== 'object') { - console.error( - 'ReactDOM.preload(): Expected the `options` argument (second) to be an object with an `as` property describing the type of resource to be preloaded but encountered %s instead.', - getValueDescriptorExpectingEnumForWarning(options), - ); - } else if (typeof options.as !== 'string') { - console.error( - 'ReactDOM.preload(): Expected the `as` property in the `options` argument (second) to contain a string value describing the type of resource to be preloaded but encountered %s instead. Values that are valid in for the `as` attribute of a `` tag are valid here.', - getValueDescriptorExpectingEnumForWarning(options.as), + 'ReactDOM.preload(): Expected two arguments, a non-empty `href` string and an `options` object with an `as` property valid for a `` tag.%s', + encountered, ); } } @@ -5085,10 +5090,29 @@ export function preload(href: string, options: PreloadOptions) { href && typeof options === 'object' && options !== null && - typeof options.as === 'string' + typeof options.as === 'string' && + options.as ) { const as = options.as; - const key = getResourceKey(as, href); + let key: string; + if (as === 'image') { + // For image preloads the key contains either the imageSrcSet + imageSizes or the href but not + // both. This is to prevent identical calls with the same srcSet and sizes to be duplicated + // by varying the href. this is an edge case but it is the most correct behavior. + const {imageSrcSet, imageSizes} = options; + let uniquePart = ''; + if (typeof imageSrcSet === 'string' && imageSrcSet !== '') { + uniquePart += '[' + imageSrcSet + ']'; + if (typeof imageSizes === 'string') { + uniquePart += '[' + imageSizes + ']'; + } + } else { + uniquePart += '[][]' + href; + } + key = getResourceKey(as, uniquePart); + } else { + key = getResourceKey(as, href); + } let resource = resources.preloadsMap.get(key); if (__DEV__) { const devResource = getAsResourceDEV(resource); @@ -5528,12 +5552,18 @@ function preloadPropsFromPreloadOptions( return { rel: 'preload', as, - href, + // There is a bug in Safari where imageSrcSet is not respected on preload links + // so we omit the href here if we have imageSrcSet b/c safari will load the wrong image. + // This harms older browers that do not support imageSrcSet by making their preloads not work + // but this population is shrinking fast and is already small so we accept this tradeoff. + href: as === 'image' && options.imageSrcSet ? undefined : href, crossOrigin: as === 'font' ? '' : options.crossOrigin, integrity: options.integrity, type: options.type, nonce: options.nonce, fetchPriority: options.fetchPriority, + imageSrcSet: options.imageSrcSet, + imageSizes: options.imageSizes, }; } diff --git a/packages/react-dom-bindings/src/shared/ReactDOMResourceValidation.js b/packages/react-dom-bindings/src/shared/ReactDOMResourceValidation.js index 71f48665bb..20367f4283 100644 --- a/packages/react-dom-bindings/src/shared/ReactDOMResourceValidation.js +++ b/packages/react-dom-bindings/src/shared/ReactDOMResourceValidation.js @@ -62,60 +62,6 @@ function propNamesListJoin( } } -export function validatePreloadArguments(href: mixed, options: mixed) { - if (__DEV__) { - if (!href || typeof href !== 'string') { - const typeOfArg = getValueDescriptorExpectingObjectForWarning(href); - console.error( - 'ReactDOM.preload() expected the first argument to be a string representing an href but found %s instead.', - typeOfArg, - ); - } else if (typeof options !== 'object' || options === null) { - const typeOfArg = getValueDescriptorExpectingObjectForWarning(options); - console.error( - 'ReactDOM.preload() expected the second argument to be an options argument containing at least an "as" property' + - ' specifying the Resource type. It found %s instead. The href for the preload call where this warning originated is "%s".', - typeOfArg, - href, - ); - } else { - const as = options.as; - switch (as) { - // Font specific validation of options - case 'font': { - if (options.crossOrigin === 'use-credentials') { - console.error( - 'ReactDOM.preload() was called with an "as" type of "font" and with a "crossOrigin" option of "use-credentials".' + - ' Fonts preloading must use crossOrigin "anonymous" to be functional. Please update your font preload to omit' + - ' the crossOrigin option or change it to any other value than "use-credentials" (Browsers default all other values' + - ' to anonymous mode). The href for the preload call where this warning originated is "%s"', - href, - ); - } - break; - } - case 'script': - case 'style': { - break; - } - - // We have an invalid as type and need to warn - default: { - const typeOfAs = getValueDescriptorExpectingEnumForWarning(as); - console.error( - 'ReactDOM.preload() expected a valid "as" type in the options (second) argument but found %s instead.' + - ' Please use one of the following valid values instead: %s. The href for the preload call where this' + - ' warning originated is "%s".', - typeOfAs, - '"style", "font", or "script"', - href, - ); - } - } - } - } -} - export function validatePreinitArguments(href: mixed, options: mixed) { if (__DEV__) { if (!href || typeof href !== 'string') { diff --git a/packages/react-dom/src/__tests__/ReactDOMFloat-test.js b/packages/react-dom/src/__tests__/ReactDOMFloat-test.js index b73bc3f41e..40b3a61e8e 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFloat-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFloat-test.js @@ -3545,6 +3545,147 @@ body { ); }); + it('uses imageSrcSet and imageSizes when keying image preloads', async () => { + function App({isClient}) { + // Will key off href in absense of imageSrcSet + ReactDOM.preload('foo', {as: 'image'}); + ReactDOM.preload('foo', {as: 'image'}); + + // Will key off imageSrcSet + imageSizes + ReactDOM.preload('foo', {as: 'image', imageSrcSet: 'fooset'}); + ReactDOM.preload('foo2', {as: 'image', imageSrcSet: 'fooset'}); + + // Will key off imageSrcSet + imageSizes + ReactDOM.preload('foo', { + as: 'image', + imageSrcSet: 'fooset', + imageSizes: 'foosizes', + }); + ReactDOM.preload('foo2', { + as: 'image', + imageSrcSet: 'fooset', + imageSizes: 'foosizes', + }); + + // Will key off href in absense of imageSrcSet, imageSizes is ignored. these should match the + // first preloads not not emit a new preload tag + ReactDOM.preload('foo', {as: 'image', imageSizes: 'foosizes'}); + ReactDOM.preload('foo', {as: 'image', imageSizes: 'foosizes'}); + + // These preloads are for something that isn't an image + // They should all key off the href + ReactDOM.preload('bar', {as: 'somethingelse'}); + ReactDOM.preload('bar', { + as: 'somethingelse', + imageSrcSet: 'makes no sense', + }); + ReactDOM.preload('bar', { + as: 'somethingelse', + imageSrcSet: 'makes no sense', + imageSizes: 'makes no sense', + }); + + if (isClient) { + // Will key off href in absense of imageSrcSet + ReactDOM.preload('client', {as: 'image'}); + ReactDOM.preload('client', {as: 'image'}); + + // Will key off imageSrcSet + imageSizes + ReactDOM.preload('client', {as: 'image', imageSrcSet: 'clientset'}); + ReactDOM.preload('client2', {as: 'image', imageSrcSet: 'clientset'}); + + // Will key off imageSrcSet + imageSizes + ReactDOM.preload('client', { + as: 'image', + imageSrcSet: 'clientset', + imageSizes: 'clientsizes', + }); + ReactDOM.preload('client2', { + as: 'image', + imageSrcSet: 'clientset', + imageSizes: 'clientsizes', + }); + + // Will key off href in absense of imageSrcSet, imageSizes is ignored. these should match the + // first preloads not not emit a new preload tag + ReactDOM.preload('client', {as: 'image', imageSizes: 'clientsizes'}); + ReactDOM.preload('client', {as: 'image', imageSizes: 'clientsizes'}); + } + + return ( + + hello + + ); + } + + await act(() => { + renderToPipeableStream().pipe(writable); + }); + expect(getMeaningfulChildren(document)).toEqual( + + + + + + + + hello + , + ); + + const root = ReactDOMClient.hydrateRoot(document, ); + await waitForAll([]); + expect(getMeaningfulChildren(document)).toEqual( + + + + + + + + hello + , + ); + + root.render(); + await waitForAll([]); + expect(getMeaningfulChildren(document)).toEqual( + + + + + + + + + + + hello + , + ); + }); + describe('ReactDOM.prefetchDNS(href)', () => { it('creates a dns-prefetch resource when called', async () => { function App({url}) { @@ -3834,10 +3975,10 @@ body { renderToPipeableStream().pipe(writable); }); }).toErrorDev([ - 'ReactDOM.preload(): Expected the `href` argument (first) to be a non-empty string but encountered `undefined` instead.', - 'ReactDOM.preload(): Expected the `href` argument (first) to be a non-empty string but encountered an empty string instead.', - 'ReactDOM.preload(): Expected the `options` argument (second) to be an object with an `as` property describing the type of resource to be preloaded but encountered `null` instead.', - 'ReactDOM.preload(): Expected the `as` property in the `options` argument (second) to contain a string value describing the type of resource to be preloaded but encountered `undefined` instead. Values that are valid in for the `as` attribute of a `` tag are valid here.', + 'ReactDOM.preload(): Expected two arguments, a non-empty `href` string and an `options` object with an `as` property valid for a `` tag. The `href` argument encountered was `undefined`. The `options` argument encountered was `undefined`.', + 'ReactDOM.preload(): Expected two arguments, a non-empty `href` string and an `options` object with an `as` property valid for a `` tag. The `href` argument encountered was an empty string. The `options` argument encountered was `undefined`.', + 'ReactDOM.preload(): Expected two arguments, a non-empty `href` string and an `options` object with an `as` property valid for a `` tag. The `options` argument encountered was `null`.', + 'ReactDOM.preload(): Expected two arguments, a non-empty `href` string and an `options` object with an `as` property valid for a `` tag. The `as` option encountered was `undefined`.', ]); }); diff --git a/packages/react-dom/src/shared/ReactDOMTypes.js b/packages/react-dom/src/shared/ReactDOMTypes.js index 61d220bf11..7c6e9a49ff 100644 --- a/packages/react-dom/src/shared/ReactDOMTypes.js +++ b/packages/react-dom/src/shared/ReactDOMTypes.js @@ -16,6 +16,8 @@ export type PreloadOptions = { type?: string, nonce?: string, fetchPriority?: 'high' | 'low' | 'auto', + imageSrcSet?: string, + imageSizes?: string, }; export type PreinitOptions = { as: string, diff --git a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOM-test.js b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOM-test.js index 32224491e4..8254a25c1a 100644 --- a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOM-test.js +++ b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOM-test.js @@ -1190,8 +1190,8 @@ describe('ReactFlightDOM', () => { root.render(); }); expect(document.head.innerHTML).toBe( - '' + - '', + '' + + '', ); expect(container.innerHTML).toBe('

hello world

'); }); diff --git a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js index b1e41f2a23..b2cb66f5bb 100644 --- a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js +++ b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js @@ -1101,7 +1101,7 @@ describe('ReactFlightDOMBrowser', () => { root.render(); }); expect(document.head.innerHTML).toBe( - '', + '', ); expect(container.innerHTML).toBe('

hello world

'); });