From 9ab40d7ec410fe94e6b6c15b2da767ed69a6c9b4 Mon Sep 17 00:00:00 2001 From: Josh Story Date: Sat, 24 Sep 2022 00:18:14 -0700 Subject: [PATCH] add validation for when props change for a resource --- .../src/__tests__/ReactDOMFloat-test.js | 85 ++++++++++++++++++ .../src/client/ReactDOMFloatClient.js | 80 +++++++++++------ .../src/shared/ReactDOMResourceValidation.js | 87 ++++++++++++++++++- .../src/ReactFiberBeginWork.new.js | 2 + .../src/ReactFiberBeginWork.old.js | 2 + 5 files changed, 229 insertions(+), 27 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFloat-test.js b/packages/react-dom/src/__tests__/ReactDOMFloat-test.js index 7a13aaab4f855..18175cc1c0776 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFloat-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFloat-test.js @@ -2858,6 +2858,91 @@ describe('ReactDOMFloat', () => { }); describe('prop validation', () => { + it('warns when you change props on a resource unless you also change the href', async () => { + const root = ReactDOMClient.createRoot(container); + root.render( +
+ + + hello +
, + ); + expect(Scheduler).toFlushWithoutYielding(); + + root.render( +
+ + + hello +
, + ); + expect(() => { + expect(Scheduler).toFlushWithoutYielding(); + }).toErrorDev( + 'Warning: A style Resource with href "foo" recieved new props with different values from the props used' + + ' when this Resource was first rendered. React will only use the props provided when' + + ' this resource was first rendered until a new href is provided. Unlike conventional' + + ' DOM elements, Resources instances do not have a one to one correspondence with Elements' + + ' in the DOM and as such, every instance of a Resource for a single Resource identifier' + + ' (href) must have props that agree with each other. The differences are described below.' + + '\n data-something-extra: missing or null in latest props, "extra" in original props' + + '\n data-something-new: "new" in latest props, missing or null in original props' + + '\n precedence: "fu" in latest props, "foo" in original props', + ); + expect(getVisibleChildren(document)).toEqual( + + + + + + + + + + +
+
hello
+
+ + , + ); + }); + it('warns when style Resource have different values for media for the same href', async () => { const originalConsoleError = console.error; const mockError = jest.fn(); diff --git a/packages/react-dom/src/client/ReactDOMFloatClient.js b/packages/react-dom/src/client/ReactDOMFloatClient.js index 3b0ee89254a6e..c3ae6f47d17ed 100644 --- a/packages/react-dom/src/client/ReactDOMFloatClient.js +++ b/packages/react-dom/src/client/ReactDOMFloatClient.js @@ -10,6 +10,7 @@ import type {Instance} from './ReactDOMHostConfig'; import {pushDispatcher, popDispatcher} from '../shared/ReactDOMDispatcher'; import { validatePreloadResourceDifference, + validateHrefKeyedUpdatedProps, validateStyleResourceDifference, validateLinkPropsForStyleResource, validateLinkPropsForPreloadResource, @@ -287,44 +288,64 @@ function stylePropsFromPreinitOptions( // Resources from render // -------------------------------------- +type StyleQualifyingProps = {href: string, precedence: string, [string]: mixed}; +type PreloadQualifyingProps = {href: string, as: ResourceType, [string]: mixed}; + // This function is called in complete work and we should always have a currentDocument set -export function getResource(type: string, rawProps: Object): Resource { +export function getResource( + type: string, + pendingProps: Props, + currentProps: null | Props, +): Resource { if (!currentDocument) { throw new Error( '"currentDocument" was expected to exist. This is a bug in React.', ); } - const {href} = rawProps; switch (type) { case 'link': { - switch (rawProps.rel) { + switch (pendingProps.rel) { case 'stylesheet': { + let didWarn; if (__DEV__) { - validateLinkPropsForStyleResource(rawProps); + if (currentProps) { + didWarn = validateHrefKeyedUpdatedProps( + pendingProps, + currentProps, + ); + } + if (!didWarn) { + didWarn = validateLinkPropsForStyleResource(pendingProps); + } } - const {precedence} = rawProps; + const { + precedence, + href, + } = ((pendingProps: any): StyleQualifyingProps); // We construct or get an existing resource for the style itself and return it let resource = styleResources.get(href); if (resource) { if (__DEV__) { - const latestProps = stylePropsFromRawProps( - href, - precedence, - rawProps, - ); - if ((resource: any)._dev_preload_props) { - adoptPreloadProps( - latestProps, - (resource: any)._dev_preload_props, + if (!didWarn) { + const latestProps = stylePropsFromRawProps( + href, + precedence, + pendingProps, ); + if ((resource: any)._dev_preload_props) { + adoptPreloadProps( + latestProps, + (resource: any)._dev_preload_props, + ); + } + validateStyleResourceDifference(resource.props, latestProps); } - validateStyleResourceDifference(resource.props, latestProps); } } else { const resourceProps = stylePropsFromRawProps( href, precedence, - rawProps, + pendingProps, ); resource = createStyleResource( currentDocument, @@ -337,16 +358,20 @@ export function getResource(type: string, rawProps: Object): Resource { return resource; } case 'preload': { - const {as} = rawProps; + const {href, as} = ((pendingProps: any): PreloadQualifyingProps); if (__DEV__) { - validateLinkPropsForPreloadResource(rawProps); + validateLinkPropsForPreloadResource(pendingProps); } let resource = preloadResources.get(href); if (resource) { if (__DEV__) { const originallyImplicit = (resource: any)._dev_implicit_construction === true; - const latestProps = preloadPropsFromRawProps(href, as, rawProps); + const latestProps = preloadPropsFromRawProps( + href, + as, + pendingProps, + ); validatePreloadResourceDifference( resource.props, originallyImplicit, @@ -355,7 +380,11 @@ export function getResource(type: string, rawProps: Object): Resource { ); } } else { - const resourceProps = preloadPropsFromRawProps(href, as, rawProps); + const resourceProps = preloadPropsFromRawProps( + href, + as, + pendingProps, + ); resource = createPreloadResource( currentDocument, href, @@ -367,7 +396,7 @@ export function getResource(type: string, rawProps: Object): Resource { } default: { // eslint-disable-next-line react-internal/safe-string-coercion - const relString = String(rawProps.rel); + const relString = String(pendingProps.rel); throw new Error( `getResource encountered a link type (rel) it did not expect: "${relString}". this is a bug in React.`, ); @@ -753,7 +782,7 @@ function insertPreloadInstance( } } -export function isHostResourceType(type: string, props: Object): boolean { +export function isHostResourceType(type: string, props: Props): boolean { switch (type) { case 'link': { switch (props.rel) { @@ -761,8 +790,9 @@ export function isHostResourceType(type: string, props: Object): boolean { if (__DEV__) { validateLinkPropsForStyleResource(props); } - const {precedence, onLoad, onError, disabled} = props; + const {href, precedence, onLoad, onError, disabled} = props; return ( + typeof href === 'string' && typeof precedence === 'string' && !onLoad && !onError && @@ -773,8 +803,8 @@ export function isHostResourceType(type: string, props: Object): boolean { if (__DEV__) { validateLinkPropsForStyleResource(props); } - const {onLoad, onError} = props; - return !onLoad && !onError; + const {href, onLoad, onError} = props; + return typeof href === 'string' && !onLoad && !onError; } } } diff --git a/packages/react-dom/src/shared/ReactDOMResourceValidation.js b/packages/react-dom/src/shared/ReactDOMResourceValidation.js index 87a858c4a193d..f2ed0f9870926 100644 --- a/packages/react-dom/src/shared/ReactDOMResourceValidation.js +++ b/packages/react-dom/src/shared/ReactDOMResourceValidation.js @@ -9,6 +9,8 @@ import hasOwnProperty from 'shared/hasOwnProperty'; +type Props = {[string]: mixed}; + export function validatePreloadResourceDifference( originalProps: any, originalImplicit: boolean, @@ -279,10 +281,89 @@ function getResourceNameForWarning( return 'Resource'; } -export function validateLinkPropsForStyleResource(linkProps: any) { +export function validateHrefKeyedUpdatedProps( + pendingProps: Props, + currentProps: Props, +): boolean { + if (__DEV__) { + // This function should never be called if we don't have hrefs so we don't bother considering + // Whether they are null or undefined + if (pendingProps.href === currentProps.href) { + // If we have the same href we need all other props to be the same + let missingProps; + let extraProps; + let differentProps; + const allProps = Array.from( + new Set(Object.keys(currentProps).concat(Object.keys(pendingProps))), + ); + for (let i = 0; i < allProps.length; i++) { + const propName = allProps[i]; + const pendingValue = pendingProps[propName]; + const currentValue = currentProps[propName]; + if ( + pendingValue !== currentValue && + !(pendingValue == null && currentValue == null) + ) { + if (pendingValue == null) { + missingProps = missingProps || {}; + missingProps[propName] = currentValue; + } else if (currentValue == null) { + extraProps = extraProps || {}; + extraProps[propName] = pendingValue; + } else { + differentProps = differentProps || {}; + differentProps[propName] = { + original: currentValue, + latest: pendingValue, + }; + } + } + } + if (missingProps || extraProps || differentProps) { + const latestWarningName = getResourceNameForWarning( + 'style', + currentProps, + false, + ); + + let comparisonStatement = ''; + if (missingProps !== null && typeof missingProps === 'object') { + for (const propName in missingProps) { + comparisonStatement += `\n ${propName}: missing or null in latest props, "${missingProps[propName]}" in original props`; + } + } + if (extraProps !== null && typeof extraProps === 'object') { + for (const propName in extraProps) { + comparisonStatement += `\n ${propName}: "${extraProps[propName]}" in latest props, missing or null in original props`; + } + } + if (differentProps !== null && typeof differentProps === 'object') { + for (const propName in differentProps) { + comparisonStatement += `\n ${propName}: "${differentProps[propName].latest}" in latest props, "${differentProps[propName].original}" in original props`; + } + } + console.error( + 'A %s with href "%s" recieved new props with different values from the props used' + + ' when this Resource was first rendered. React will only use the props provided when' + + ' this resource was first rendered until a new href is provided. Unlike conventional' + + ' DOM elements, Resources instances do not have a one to one correspondence with Elements' + + ' in the DOM and as such, every instance of a Resource for a single Resource identifier' + + ' (href) must have props that agree with each other. The differences are described below.%s', + latestWarningName, + currentProps.href, + comparisonStatement, + ); + return true; + } + } + } + return false; +} + +export function validateLinkPropsForStyleResource(props: Props): boolean { if (__DEV__) { // This should only be called when we know we are opting into Resource semantics (i.e. precedence is not null) - const {href, onLoad, onError, disabled} = linkProps; + const {href, onLoad, onError, disabled} = props; const allProps = ['onLoad', 'onError', 'disabled']; const includedProps = []; if (onLoad) includedProps.push('onLoad'); @@ -303,8 +384,10 @@ export function validateLinkPropsForStyleResource(linkProps: any) { allPropsUnionPhrase, includedPropsPhrase, ); + return true; } } + return false; } function propNamesListJoin( diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.new.js b/packages/react-reconciler/src/ReactFiberBeginWork.new.js index a2242a4b6ed82..52a29b9e35a1a 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.new.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.new.js @@ -1585,9 +1585,11 @@ function updateHostComponent( function updateHostResource(current, workInProgress, renderLanes) { pushHostContext(workInProgress); markRef(current, workInProgress); + const currentProps = current === null ? null : current.memoizedProps; workInProgress.memoizedState = getResource( workInProgress.type, workInProgress.pendingProps, + currentProps, ); reconcileChildren( current, diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.old.js b/packages/react-reconciler/src/ReactFiberBeginWork.old.js index be73af58bdc49..eafc79eba36da 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.old.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.old.js @@ -1585,9 +1585,11 @@ function updateHostComponent( function updateHostResource(current, workInProgress, renderLanes) { pushHostContext(workInProgress); markRef(current, workInProgress); + const currentProps = current === null ? null : current.memoizedProps; workInProgress.memoizedState = getResource( workInProgress.type, workInProgress.pendingProps, + currentProps, ); reconcileChildren( current,