Skip to content

Commit

Permalink
add validation for when props change for a resource
Browse files Browse the repository at this point in the history
  • Loading branch information
@gnoff
gnoff committed Sep 24, 2022
1 parent ad9fb36 commit 9ab40d7
Show file tree
Hide file tree
Showing 5 changed files with 229 additions and 27 deletions.
85 changes: 85 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMFloat-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
<div>
<link
rel="stylesheet"
href="foo"
precedence="foo"
data-something-extra="extra"
/>
<link
rel="stylesheet"
href="bar"
precedence="bar"
data-something-extra="extra"
/>
hello
</div>,
);
expect(Scheduler).toFlushWithoutYielding();

root.render(
<div>
<link
rel="stylesheet"
href="foo"
precedence="fu"
data-something-new="new"
/>
<link
rel="stylesheet"
href="baz"
precedence="baz"
data-something-new="new"
/>
hello
</div>,
);
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(
<html>
<head>
<link
rel="stylesheet"
href="foo"
data-rprec="foo"
data-something-extra="extra"
/>
<link
rel="stylesheet"
href="bar"
data-rprec="bar"
data-something-extra="extra"
/>
<link
rel="stylesheet"
href="baz"
data-rprec="baz"
data-something-new="new"
/>
<link rel="preload" as="style" href="foo" />
<link rel="preload" as="style" href="bar" />
<link rel="preload" as="style" href="baz" />
</head>
<body>
<div id="container">
<div>hello</div>
</div>
</body>
</html>,
);
});

it('warns when style Resource have different values for media for the same href', async () => {
const originalConsoleError = console.error;
const mockError = jest.fn();
Expand Down
80 changes: 55 additions & 25 deletions packages/react-dom/src/client/ReactDOMFloatClient.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import type {Instance} from './ReactDOMHostConfig';
import {pushDispatcher, popDispatcher} from '../shared/ReactDOMDispatcher';
import {
validatePreloadResourceDifference,
validateHrefKeyedUpdatedProps,
validateStyleResourceDifference,
validateLinkPropsForStyleResource,
validateLinkPropsForPreloadResource,
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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.`,
);
Expand Down Expand Up @@ -753,16 +782,17 @@ 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) {
case 'stylesheet': {
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 &&
Expand All @@ -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;
}
}
}
Expand Down
87 changes: 85 additions & 2 deletions packages/react-dom/src/shared/ReactDOMResourceValidation.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@

import hasOwnProperty from 'shared/hasOwnProperty';

type Props = {[string]: mixed};

export function validatePreloadResourceDifference(
originalProps: any,
originalImplicit: boolean,
Expand Down Expand Up @@ -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');
Expand All @@ -303,8 +384,10 @@ export function validateLinkPropsForStyleResource(linkProps: any) {
allPropsUnionPhrase,
includedPropsPhrase,
);
return true;
}
}
return false;
}

function propNamesListJoin(
Expand Down
2 changes: 2 additions & 0 deletions packages/react-reconciler/src/ReactFiberBeginWork.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 2 additions & 0 deletions packages/react-reconciler/src/ReactFiberBeginWork.old.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down

0 comments on commit 9ab40d7

Please sign in to comment.