Skip to content

Commit 2b06221

Browse files
gnoffgnoff
committed
opt-out of resource semantics when using data attributes
1 parent 9d10fb5 commit 2b06221

File tree

4 files changed

+159
-8
lines changed

4 files changed

+159
-8
lines changed

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

+110
Original file line numberDiff line numberDiff line change
@@ -783,6 +783,116 @@ describe('ReactDOMResources', () => {
783783
);
784784
});
785785

786+
it('treats resource eligible elements with data-* attributes as components instead of resources', async () => {
787+
await actIntoEmptyDocument(async () => {
788+
const {pipe} = ReactDOMFizzServer.renderToPipeableStream(
789+
<html>
790+
<head>
791+
<link rel="stylesheet" href="foo" />
792+
<link rel="stylesheet" href="foo" crossOrigin="" />
793+
<link rel="stylesheet" href="foo" crossOrigin="use-credentials" />
794+
</head>
795+
<body>
796+
<link rel="stylesheet" href="foo" crossOrigin="" data-foo="" />
797+
<link rel="stylesheet" href="foo" crossOrigin="" data-foo="" />
798+
<div>hello world</div>
799+
</body>
800+
</html>,
801+
);
802+
pipe(writable);
803+
});
804+
// data attribute links get their own individual representation in the stream because they are treated
805+
// like regular HostComponents
806+
expect(getVisibleChildren(document)).toEqual(
807+
<html>
808+
<head>
809+
<link rel="stylesheet" href="foo" />
810+
<link rel="stylesheet" href="foo" crossorigin="" />
811+
<link rel="stylesheet" href="foo" crossorigin="use-credentials" />
812+
</head>
813+
<body>
814+
<link rel="stylesheet" href="foo" crossorigin="" data-foo="" />
815+
<link rel="stylesheet" href="foo" crossorigin="" data-foo="" />
816+
<div>hello world</div>
817+
</body>
818+
</html>,
819+
);
820+
821+
const root = ReactDOMClient.hydrateRoot(
822+
container,
823+
<html>
824+
<head>
825+
<link rel="stylesheet" href="foo" />
826+
<link rel="stylesheet" href="foo" crossOrigin="" />
827+
<link rel="stylesheet" href="foo" crossOrigin="use-credentials" />
828+
</head>
829+
<body>
830+
<link rel="stylesheet" href="foo" crossOrigin="" data-foo="" />
831+
<link rel="stylesheet" href="foo" crossOrigin="" data-foo="" />
832+
<div>hello world</div>
833+
</body>
834+
</html>,
835+
);
836+
expect(Scheduler).toFlushWithoutYielding();
837+
expect(getVisibleChildren(document)).toEqual(
838+
<html>
839+
<head>
840+
<link rel="stylesheet" href="foo" />
841+
<link rel="stylesheet" href="foo" crossorigin="" />
842+
<link rel="stylesheet" href="foo" crossorigin="use-credentials" />
843+
</head>
844+
<body>
845+
<link rel="stylesheet" href="foo" crossorigin="" data-foo="" />
846+
<link rel="stylesheet" href="foo" crossorigin="" data-foo="" />
847+
<div>hello world</div>
848+
</body>
849+
</html>,
850+
);
851+
852+
// Drop the foo crossorigin anonymous HostResource that might match if we weren't useing data attributes
853+
// It is actually removed from the head because the body representation is a HostComponent and completely
854+
// disconnected from the Resource runtime.
855+
root.render(
856+
<html>
857+
<head>
858+
<link rel="stylesheet" href="foo" />
859+
<link rel="stylesheet" href="foo" crossOrigin="use-credentials" />
860+
</head>
861+
<body>
862+
<link
863+
rel="stylesheet"
864+
href="foo"
865+
crossOrigin=""
866+
data-bar="baz"
867+
data-foo=""
868+
/>
869+
<link rel="stylesheet" href="foo" crossOrigin="" data-foo="" />
870+
<div>hello world</div>
871+
</body>
872+
</html>,
873+
);
874+
expect(Scheduler).toFlushWithoutYielding();
875+
expect(getVisibleChildren(document)).toEqual(
876+
<html>
877+
<head>
878+
<link rel="stylesheet" href="foo" />
879+
<link rel="stylesheet" href="foo" crossorigin="use-credentials" />
880+
</head>
881+
<body>
882+
<link
883+
rel="stylesheet"
884+
href="foo"
885+
crossorigin=""
886+
data-bar="baz"
887+
data-foo=""
888+
/>
889+
<link rel="stylesheet" href="foo" crossorigin="" data-foo="" />
890+
<div>hello world</div>
891+
</body>
892+
</html>,
893+
);
894+
});
895+
786896
describe('link resources', () => {
787897
// @gate enableFloat
788898
it('keys resources on href, crossOrigin, and referrerPolicy', async () => {

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

+33-3
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,6 @@ export function acquireResource(
6767
);
6868
setInitialResourceProperties(domElement, type, props, resourceContainer);
6969
}
70-
insertResource(domElement, resourceHost);
7170
resource = {
7271
key,
7372
type,
@@ -76,7 +75,9 @@ export function acquireResource(
7675
};
7776
resourceMap.set(key, resource);
7877
}
79-
resource.count++;
78+
if (resource.count++ === 0) {
79+
insertResource(resource.instance, resourceHost);
80+
}
8081
return resource;
8182
}
8283

@@ -206,13 +207,28 @@ export function getRootResourceHost(
206207
};
207208
}
208209

210+
export function isResource(type: string, props: Object): boolean {
211+
const key = getResourceKeyFromTypeAndProps(type, props);
212+
if (key) {
213+
// This is potentially a Resource. We need to check if props contain
214+
// data attributes. Resources do not support data attributes.
215+
for (const prop in props) {
216+
if (prop.startsWith('data-')) {
217+
return false;
218+
}
219+
}
220+
}
221+
return !!key;
222+
}
223+
209224
export function getResourceKeyFromTypeAndProps(
210225
type: string,
211226
props: Object,
212227
): ?string {
213228
switch (type) {
214229
case 'link': {
215230
const {rel, href, crossOrigin, referrerPolicy} = props;
231+
216232
if (!href) {
217233
return undefined;
218234
}
@@ -247,9 +263,23 @@ export function getResourceKeyFromTypeAndProps(
247263
}
248264
}
249265

250-
export function resourceFromElement(domElement: Element): boolean {
266+
export function resourceFromElement(domElement: HTMLElement): boolean {
251267
if (rootIsUsingResources) {
268+
if (Object.keys(domElement.dataset).length) {
269+
// This element has data attributes. Managing data attributes for resources is impractical
270+
// because they suggest an intention to query / manipulate DOM Elmenets directly and
271+
// turning the React representation into a deduped reference is incongruent with such
272+
// intention.
273+
return false;
274+
}
252275
const type = domElement.tagName.toLowerCase();
276+
// This is really unfortunate that we need to create this intermediate props object.
277+
// Originally I tried to just pass the domElement as the props object since jsx prop names
278+
// match HTMLElement property names. However interface for the values passed to props more
279+
// closely matches attributes. crossOrigin in particular is a pain where the attribute being
280+
// missing is supposed to encode no cors but the property returns an empty string. However
281+
// when the attribute is the empty string we are supped to be in cors anonymous mode but the property
282+
// can also return empty string in this case (at least in JSDOM);
253283
const props = {
254284
rel: domElement.getAttribute('rel'),
255285
href: domElement.getAttribute('href'),

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

+1-4
Original file line numberDiff line numberDiff line change
@@ -1357,13 +1357,10 @@ export function prepareToRender(rootTag: RootTag) {
13571357

13581358
export function cleanupAfterRender() {}
13591359

1360-
export function isResource(type: string, props: Props) {
1361-
return !!getResourceKeyFromTypeAndProps(type, props);
1362-
}
1363-
13641360
export {getResourceKeyFromTypeAndProps};
13651361

13661362
export {
1363+
isResource,
13671364
acquireResource,
13681365
releaseResource,
13691366
getRootResourceHost,

‎packages/react-dom/src/server/ReactDOMFloatServer.js

+15-1
Original file line numberDiff line numberDiff line change
@@ -400,13 +400,27 @@ export function resourcesFromLink(props: Object): boolean {
400400
// }
401401
case 'stylesheet': {
402402
preinit(href, {as: 'style', crossOrigin});
403-
return true;
403+
// If this component is a valid resource, meaning it does not have anything that would
404+
// cause it to need to be treated like a component we can omit it and return true here.
405+
// If it is in fact a component it will need to be inserted and we return false here.
406+
return validateResourceProps(props);
404407
}
405408
default:
406409
return false;
407410
}
408411
}
409412

413+
// returns false if props object contains any props which invalidate
414+
// treating the element entirely as a Resource
415+
function validateResourceProps(props: Object): boolean {
416+
for (const prop in props) {
417+
if (prop.toLowerCase().startsWith('data-')) {
418+
return false;
419+
}
420+
}
421+
return true;
422+
}
423+
410424
// Construct a resource from script props.
411425
export function resourcesFromScript(props: Object) {
412426
// const src = props.src;

0 commit comments

Comments
 (0)