Skip to content

Commit b633ab0

Browse files
gnoffgnoff
committed
Support validation for props on duplicate resource and on client resource update`
1 parent cad5c22 commit b633ab0

File tree

10 files changed

+433
-486
lines changed

10 files changed

+433
-486
lines changed

‎packages/react-dom/src/__tests__/ReactDOMResources-test.js‎

Lines changed: 169 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,15 @@ describe('ReactDOMResources', () => {
172172
// }
173173
// }
174174

175+
function normalizeCodeLocInfo(str) {
176+
return (
177+
str &&
178+
str.replace(/\n +(?:at|in) ([\S]+)[^\n]*/g, function(m, name) {
179+
return '\n in ' + name + ' (at **)';
180+
})
181+
);
182+
}
183+
175184
// @gate enableFloat
176185
it('hoists resources to the head if the container is a Document without hydration', async () => {
177186
function App() {
@@ -950,7 +959,7 @@ describe('ReactDOMResources', () => {
950959
rel="stylesheet"
951960
href="foo"
952961
crossOrigin="anonymous"
953-
referrerPolicy="strict-origin-when-cross-origin"
962+
referrerPolicy=""
954963
/>
955964
<link rel="stylesheet" href="foo" crossOrigin="use-credentials" />
956965
</head>
@@ -1034,5 +1043,164 @@ describe('ReactDOMResources', () => {
10341043
/>,
10351044
]);
10361045
});
1046+
1047+
// @gate enableFloat
1048+
it('warns in Dev when two key-matched resources use different values for non-ignored divergent props', async () => {
1049+
const originalConsoleError = console.error;
1050+
const mockError = jest.fn();
1051+
console.error = (...args) => {
1052+
if (args.length > 1) {
1053+
if (typeof args[1] === 'object') {
1054+
mockError(args[0].split('\n')[0]);
1055+
return;
1056+
}
1057+
}
1058+
mockError(...args.map(normalizeCodeLocInfo));
1059+
};
1060+
1061+
try {
1062+
await actIntoEmptyDocument(() => {
1063+
const {pipe} = ReactDOMFizzServer.renderToPipeableStream(
1064+
<html>
1065+
<head />
1066+
<body>
1067+
<div>
1068+
<link rel="stylesheet" href="foo" media="print" />
1069+
<link
1070+
rel="stylesheet"
1071+
href="foo"
1072+
media="screen and (max-width: 600px)"
1073+
/>
1074+
hello world
1075+
</div>
1076+
</body>
1077+
</html>,
1078+
);
1079+
pipe(writable);
1080+
});
1081+
// The second link matches the key of the first link but disagrees on the media prop. this should warn but also only
1082+
// emit the first resource representation for that key
1083+
expect(getVisibleChildren(document)).toEqual(
1084+
<html>
1085+
<head>
1086+
<link rel="stylesheet" href="foo" media="print" />
1087+
</head>
1088+
<body>
1089+
<div>hello world</div>
1090+
</body>
1091+
</html>,
1092+
);
1093+
if (__DEV__) {
1094+
expect(mockError.mock.calls).toEqual([
1095+
[
1096+
'Warning: A "%s" Resource (%s="%s") was %s with a different "%s" prop than the one used originally.' +
1097+
' The original value was "%s" and the new value is "%s". Either the "%s" value should be the same' +
1098+
' or this Resource should point to distinct "%s" location.%s',
1099+
'stylesheet',
1100+
'href',
1101+
'foo',
1102+
'created',
1103+
'media',
1104+
'print',
1105+
'screen and (max-width: 600px)',
1106+
'media',
1107+
'href',
1108+
'\n' +
1109+
' in link (at **)\n' +
1110+
' in div (at **)\n' +
1111+
' in body (at **)\n' +
1112+
' in html (at **)',
1113+
],
1114+
]);
1115+
}
1116+
mockError.mock.calls.length = 0;
1117+
1118+
const root = ReactDOMClient.hydrateRoot(
1119+
document,
1120+
<html>
1121+
<head />
1122+
<body>
1123+
<div>
1124+
<link rel="stylesheet" href="foo" media="print" />
1125+
<link
1126+
rel="stylesheet"
1127+
href="foo"
1128+
media="screen and (max-width: 600px)"
1129+
/>
1130+
hello world
1131+
</div>
1132+
</body>
1133+
</html>,
1134+
);
1135+
expect(Scheduler).toFlushWithoutYielding();
1136+
if (__DEV__) {
1137+
expect(mockError.mock.calls).toEqual([
1138+
[
1139+
'Warning: A "%s" Resource (%s="%s") was %s with a different "%s" prop than the one used originally.' +
1140+
' The original value was "%s" and the new value is "%s". Either the "%s" value should be the same' +
1141+
' or this Resource should point to distinct "%s" location.%s',
1142+
'stylesheet',
1143+
'href',
1144+
'foo',
1145+
'created',
1146+
'media',
1147+
'print',
1148+
'screen and (max-width: 600px)',
1149+
'media',
1150+
'href',
1151+
'\n' +
1152+
' in div (at **)\n' +
1153+
' in body (at **)\n' +
1154+
' in html (at **)',
1155+
],
1156+
]);
1157+
}
1158+
mockError.mock.calls.length = 0;
1159+
1160+
root.render(
1161+
<html>
1162+
<head />
1163+
<body>
1164+
<div>
1165+
<link rel="stylesheet" href="foo" media="print" />
1166+
<link
1167+
rel="stylesheet"
1168+
href="foo"
1169+
media="print"
1170+
integrity="some hash"
1171+
/>
1172+
hello world
1173+
</div>
1174+
</body>
1175+
</html>,
1176+
);
1177+
1178+
expect(Scheduler).toFlushWithoutYielding();
1179+
if (__DEV__) {
1180+
expect(mockError.mock.calls).toEqual([
1181+
[
1182+
'Warning: A "%s" Resource (%s="%s") was %s with a different "%s" prop than the one used originally.' +
1183+
' The original value was "%s" and the new value is "%s". Either the "%s" value should be the same' +
1184+
' or this Resource should point to distinct "%s" location.%s',
1185+
'stylesheet',
1186+
'href',
1187+
'foo',
1188+
'updated',
1189+
'integrity',
1190+
'',
1191+
'some hash',
1192+
'integrity',
1193+
'href',
1194+
'\n' +
1195+
' in div (at **)\n' +
1196+
' in body (at **)\n' +
1197+
' in html (at **)',
1198+
],
1199+
]);
1200+
}
1201+
} finally {
1202+
console.error = originalConsoleError;
1203+
}
1204+
});
10371205
});
10381206
});

‎packages/react-dom/src/client/ReactDOMFloatResources.js‎

Lines changed: 84 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -26,11 +26,15 @@ export type ResourceHost = {
2626
container: ResourceContainer,
2727
};
2828

29+
const STYLE_RESOURCE = 'stylesheet';
30+
31+
type ResourceElementType = 'link';
2932
export type Resource = {
3033
key: string,
31-
type: string,
34+
elementType: ResourceElementType,
3235
count: number,
3336
instance: Element,
37+
props: Object,
3438
};
3539

3640
const CORS_NONE = '';
@@ -46,7 +50,7 @@ let rootIsUsingResources = false;
4650

4751
export function acquireResource(
4852
key: string,
49-
type: string,
53+
elementType: ResourceElementType,
5054
props: Object,
5155
resourceHost: ResourceHost,
5256
): Resource {
@@ -56,28 +60,43 @@ export function acquireResource(
5660
let domElement = embeddedResourceElementMap.get(key);
5761
if (domElement) {
5862
embeddedResourceElementMap.delete(key);
63+
setInitialResourceProperties(
64+
domElement,
65+
elementType,
66+
props,
67+
resourceContainer,
68+
);
5969
} else {
6070
// We cheat somewhat and substitute the resourceHost container instead of the rootContainer.
6171
// Sometimes they are the same but even when they are not, the ownerDocument should be.
6272
domElement = createElement(
63-
type,
73+
elementType,
6474
props,
6575
resourceContainer,
6676
HTML_NAMESPACE,
6777
);
68-
setInitialResourceProperties(domElement, type, props, resourceContainer);
78+
setInitialResourceProperties(
79+
domElement,
80+
elementType,
81+
props,
82+
resourceContainer,
83+
);
6984
insertResource(domElement, resourceHost);
7085
}
7186
resource = {
7287
key,
73-
type,
88+
elementType,
7489
count: 1,
7590
instance: domElement,
91+
props,
7692
};
7793
resourceMap.set(key, resource);
7894
} else if (resource.count++ === 0) {
7995
insertResource(resource.instance, resourceHost);
8096
}
97+
if (__DEV__) {
98+
validateResourceAndProps(resource, props, true);
99+
}
81100
return resource;
82101
}
83102

@@ -242,17 +261,14 @@ export function getResourceKeyFromTypeAndProps(
242261
cors = CORS_NONE;
243262
}
244263

245-
const referrer =
246-
referrerPolicy === 'strict-origin-when-cross-origin'
247-
? ''
248-
: referrerPolicy || '';
264+
const referrer = referrerPolicy || '';
249265

250266
// We use new-lines in the key because they are not valid in urls and thus there should
251267
// never be a collision between a href with no cors/referrer and another href with particular
252268
// cors & referrer.
253269
switch (rel) {
254270
case 'stylesheet': {
255-
return href + '\n' + cors + referrer;
271+
return STYLE_RESOURCE + '\n' + href + '\n' + cors + referrer;
256272
}
257273
default:
258274
return undefined;
@@ -297,3 +313,61 @@ export function resourceFromElement(domElement: HTMLElement): boolean {
297313

298314
return false;
299315
}
316+
317+
// @TODO figure out how to utilize existing prop validation to do this instead of reinventing
318+
let warnOnDivergentPropsStylesheet = null;
319+
if (__DEV__) {
320+
warnOnDivergentPropsStylesheet = ['media', 'integrity', 'title'].map(
321+
propName => {
322+
return {
323+
propName,
324+
type: 'string',
325+
};
326+
},
327+
);
328+
}
329+
330+
// This coercion is to normalize across semantically identical values (such as missing being equivalent to empty string)
331+
// If the user used an improper type for a prop that will be warned on using the normal prop validation mechanism
332+
function getPropertyValue(props, propertyInfo): any {
333+
switch (propertyInfo.type) {
334+
case 'string': {
335+
return props[propertyInfo.propName] || '';
336+
}
337+
}
338+
}
339+
340+
export function validateResourceAndProps(
341+
resource: Resource,
342+
props: Object,
343+
created: boolean,
344+
) {
345+
if (__DEV__) {
346+
switch (resource.elementType) {
347+
case 'link': {
348+
(warnOnDivergentPropsStylesheet: any).forEach(propertyInfo => {
349+
const currentProp = getPropertyValue(resource.props, propertyInfo);
350+
const nextProp = getPropertyValue(props, propertyInfo);
351+
if (currentProp !== nextProp) {
352+
const locationPropName = 'href';
353+
const propName = propertyInfo.propName;
354+
console.error(
355+
'A "%s" Resource (%s="%s") was %s with a different "%s" prop than the one used originally.' +
356+
' The original value was "%s" and the new value is "%s". Either the "%s" value should' +
357+
' be the same or this Resource should point to distinct "%s" location.',
358+
'stylesheet',
359+
locationPropName,
360+
props.href,
361+
created ? 'created' : 'updated',
362+
propName,
363+
currentProp,
364+
nextProp,
365+
propName,
366+
locationPropName,
367+
);
368+
}
369+
});
370+
}
371+
}
372+
}
373+
}

‎packages/react-dom/src/client/ReactDOMHostConfig.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1366,4 +1366,5 @@ export {
13661366
getRootResourceHost,
13671367
insertPendingResources,
13681368
reconcileHydratedResources,
1369+
validateResourceAndProps,
13691370
} from './ReactDOMFloatResources';

0 commit comments

Comments
 (0)