Skip to content

Commit 47d0c30

Browse files
gnoffgnoff
authored
[Fiber][Float] Error when a host fiber changes "flavor" (#29693)
Host Components can exist as four semantic types 1. regular Components (Vanilla obv) 2. singleton Components 2. hoistable components 3. resources Each of these component types have their own rules related to mounting and reconciliation however they are not direclty modeled as their own unique fiber type. This is partly for code size but also because reconciling the inner type of these components would be in a very hot path in fiber creation and reconciliation and it's just not practical to do this logic check here. Right now we have three Fiber types used to implement these 4 concepts but we probably need to reconsider the model and think of Host Components as a single fiber type with an inner implementation. Once we do this we can regularize things like transitioning between a resource and a regular component or a singleton and a hoistable instance. The cases where these transitions happen today aren't particularly common but they can be observed and currently the handling of these transitions is incomplete at best and buggy at worst. The most egregious case is the link type. This can be a regular component (stylesheet without precedence) a hoistable component (non stylesheet link tags) or a resource (stylesheet with a precedence) and if you have a single jsx slot that tries to reconcile transitions between these types it just doesn't work well. This commit adds an error for when a Hoistable goes from Instance to Resource. Currently this is only possible for `<link>` elements going to and from stylesheets with precedence. Hopefully we'll be able to remove this error and implement as an inner type before we encounter new categories for the Hoistable types detecting type shifting to and from regular components is harder to do efficiently because we don't want to reevaluate the type on every update for host components which is currently not required and would add overhead to a very hot path singletons can't really type shift in their one practical implementation (DOM) so they are only a problem in theroy not practice
1 parent b421783 commit 47d0c30

File tree

5 files changed

+253
-35
lines changed

5 files changed

+253
-35
lines changed

‎packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js‎

Lines changed: 80 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2358,6 +2358,7 @@ export function getResource(
23582358
type: string,
23592359
currentProps: any,
23602360
pendingProps: any,
2361+
currentResource: null | Resource,
23612362
): null | Resource {
23622363
const resourceRoot = getCurrentResourceRoot();
23632364
if (!resourceRoot) {
@@ -2430,9 +2431,44 @@ export function getResource(
24302431
);
24312432
}
24322433
}
2434+
if (currentProps && currentResource === null) {
2435+
// This node was previously an Instance type and is becoming a Resource type
2436+
// For now we error because we don't support flavor changes
2437+
let diff = '';
2438+
if (__DEV__) {
2439+
diff = `
2440+
2441+
- ${describeLinkForResourceErrorDEV(currentProps)}
2442+
+ ${describeLinkForResourceErrorDEV(pendingProps)}`;
2443+
}
2444+
throw new Error(
2445+
'Expected <link> not to update to be updated to a stylehsheet with precedence.' +
2446+
' Check the `rel`, `href`, and `precedence` props of this component.' +
2447+
' Alternatively, check whether two different <link> components render in the same slot or share the same key.' +
2448+
diff,
2449+
);
2450+
}
24332451
return resource;
2452+
} else {
2453+
if (currentProps && currentResource !== null) {
2454+
// This node was previously a Resource type and is becoming an Instance type
2455+
// For now we error because we don't support flavor changes
2456+
let diff = '';
2457+
if (__DEV__) {
2458+
diff = `
2459+
2460+
- ${describeLinkForResourceErrorDEV(currentProps)}
2461+
+ ${describeLinkForResourceErrorDEV(pendingProps)}`;
2462+
}
2463+
throw new Error(
2464+
'Expected stylesheet with precedence to not be updated to a different kind of <link>.' +
2465+
' Check the `rel`, `href`, and `precedence` props of this component.' +
2466+
' Alternatively, check whether two different <link> components render in the same slot or share the same key.' +
2467+
diff,
2468+
);
2469+
}
2470+
return null;
24342471
}
2435-
return null;
24362472
}
24372473
case 'script': {
24382474
const async = pendingProps.async;
@@ -2473,6 +2509,49 @@ export function getResource(
24732509
}
24742510
}
24752511

2512+
function describeLinkForResourceErrorDEV(props: any) {
2513+
if (__DEV__) {
2514+
let describedProps = 0;
2515+
2516+
let description = '<link';
2517+
if (typeof props.rel === 'string') {
2518+
describedProps++;
2519+
description += ` rel="${props.rel}"`;
2520+
} else if (hasOwnProperty.call(props, 'rel')) {
2521+
describedProps++;
2522+
description += ` rel="${
2523+
props.rel === null ? 'null' : 'invalid type ' + typeof props.rel
2524+
}"`;
2525+
}
2526+
if (typeof props.href === 'string') {
2527+
describedProps++;
2528+
description += ` href="${props.href}"`;
2529+
} else if (hasOwnProperty.call(props, 'href')) {
2530+
describedProps++;
2531+
description += ` href="${
2532+
props.href === null ? 'null' : 'invalid type ' + typeof props.href
2533+
}"`;
2534+
}
2535+
if (typeof props.precedence === 'string') {
2536+
describedProps++;
2537+
description += ` precedence="${props.precedence}"`;
2538+
} else if (hasOwnProperty.call(props, 'precedence')) {
2539+
describedProps++;
2540+
description += ` precedence={${
2541+
props.precedence === null
2542+
? 'null'
2543+
: 'invalid type ' + typeof props.precedence
2544+
}}`;
2545+
}
2546+
if (Object.getOwnPropertyNames(props).length > describedProps) {
2547+
description += ' ...';
2548+
}
2549+
description += ' />';
2550+
return description;
2551+
}
2552+
return '';
2553+
}
2554+
24762555
function styleTagPropsFromRawProps(
24772556
rawProps: StyleTagQualifyingProps,
24782557
): StyleTagProps {

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

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,123 @@
1+
/**
2+
* Copyright (c) Meta Platforms, Inc. and affiliates.
3+
*
4+
* This source code is licensed under the MIT license found in the
5+
* LICENSE file in the root directory of this source tree.
6+
*
7+
* @emails react-core
8+
* @jest-environment ./scripts/jest/ReactDOMServerIntegrationEnvironment
9+
*/
10+
11+
'use strict';
12+
13+
let JSDOM;
14+
let React;
15+
let ReactDOMClient;
16+
let container;
17+
let waitForAll;
18+
19+
describe('ReactDOM HostSingleton', () => {
20+
beforeEach(() => {
21+
jest.resetModules();
22+
JSDOM = require('jsdom').JSDOM;
23+
// Test Environment
24+
const jsdom = new JSDOM(
25+
'<!DOCTYPE html><html><head></head><body><div id="container">',
26+
{
27+
runScripts: 'dangerously',
28+
},
29+
);
30+
global.window = jsdom.window;
31+
global.document = jsdom.window.document;
32+
container = global.document.getElementById('container');
33+
34+
React = require('react');
35+
ReactDOMClient = require('react-dom/client');
36+
37+
const InternalTestUtils = require('internal-test-utils');
38+
waitForAll = InternalTestUtils.waitForAll;
39+
});
40+
41+
it('errors when a hoistable component becomes a Resource', async () => {
42+
const errors = [];
43+
function onError(e) {
44+
errors.push(e.message);
45+
}
46+
const root = ReactDOMClient.createRoot(container, {
47+
onUncaughtError: onError,
48+
});
49+
50+
root.render(
51+
<div>
52+
<link rel="preload" href="bar" as="style" />
53+
</div>,
54+
);
55+
await waitForAll([]);
56+
57+
root.render(
58+
<div>
59+
<link rel="stylesheet" href="bar" precedence="default" />
60+
</div>,
61+
);
62+
await waitForAll([]);
63+
if (__DEV__) {
64+
expect(errors).toEqual([
65+
`Expected <link> not to update to be updated to a stylehsheet with precedence. Check the \`rel\`, \`href\`, and \`precedence\` props of this component. Alternatively, check whether two different <link> components render in the same slot or share the same key.
66+
67+
- <link rel=\"preload\" href=\"bar\" ... />
68+
+ <link rel=\"stylesheet\" href=\"bar\" precedence=\"default\" />`,
69+
]);
70+
} else {
71+
expect(errors).toEqual([
72+
'Expected <link> not to update to be updated to a stylehsheet with precedence. Check the `rel`, `href`, and `precedence` props of this component. Alternatively, check whether two different <link> components render in the same slot or share the same key.',
73+
]);
74+
}
75+
});
76+
77+
it('errors when a hoistable Resource becomes an instance', async () => {
78+
const errors = [];
79+
function onError(e) {
80+
errors.push(e.message);
81+
}
82+
const root = ReactDOMClient.createRoot(container, {
83+
onUncaughtError: onError,
84+
});
85+
86+
root.render(
87+
<div>
88+
<link rel="stylesheet" href="bar" precedence="default" />
89+
</div>,
90+
);
91+
await waitForAll([]);
92+
const event = new window.Event('load');
93+
const preloads = document.querySelectorAll('link[rel="preload"]');
94+
for (let i = 0; i < preloads.length; i++) {
95+
const node = preloads[i];
96+
node.dispatchEvent(event);
97+
}
98+
const stylesheets = document.querySelectorAll('link[rel="preload"]');
99+
for (let i = 0; i < stylesheets.length; i++) {
100+
const node = stylesheets[i];
101+
node.dispatchEvent(event);
102+
}
103+
104+
root.render(
105+
<div>
106+
<link rel="foo" href="bar" />
107+
</div>,
108+
);
109+
await waitForAll([]);
110+
if (__DEV__) {
111+
expect(errors).toEqual([
112+
`Expected stylesheet with precedence to not be updated to a different kind of <link>. Check the \`rel\`, \`href\`, and \`precedence\` props of this component. Alternatively, check whether two different <link> components render in the same slot or share the same key.
113+
114+
- <link rel=\"stylesheet\" href=\"bar\" precedence=\"default\" />
115+
+ <link rel=\"foo\" href=\"bar\" />`,
116+
]);
117+
} else {
118+
expect(errors).toEqual([
119+
'Expected stylesheet with precedence to not be updated to a different kind of <link>. Check the `rel`, `href`, and `precedence` props of this component. Alternatively, check whether two different <link> components render in the same slot or share the same key.',
120+
]);
121+
}
122+
});
123+
});

‎packages/react-reconciler/src/ReactFiberBeginWork.js‎

Lines changed: 28 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1689,22 +1689,36 @@ function updateHostHoistable(
16891689
renderLanes: Lanes,
16901690
) {
16911691
markRef(current, workInProgress);
1692-
const currentProps = current === null ? null : current.memoizedProps;
1693-
const resource = (workInProgress.memoizedState = getResource(
1694-
workInProgress.type,
1695-
currentProps,
1696-
workInProgress.pendingProps,
1697-
));
1692+
16981693
if (current === null) {
1699-
if (!getIsHydrating() && resource === null) {
1700-
// This is not a Resource Hoistable and we aren't hydrating so we construct the instance.
1701-
workInProgress.stateNode = createHoistableInstance(
1702-
workInProgress.type,
1703-
workInProgress.pendingProps,
1704-
getRootHostContainer(),
1705-
workInProgress,
1706-
);
1694+
const resource = getResource(
1695+
workInProgress.type,
1696+
null,
1697+
workInProgress.pendingProps,
1698+
null,
1699+
);
1700+
if (resource) {
1701+
workInProgress.memoizedState = resource;
1702+
} else {
1703+
if (!getIsHydrating()) {
1704+
// This is not a Resource Hoistable and we aren't hydrating so we construct the instance.
1705+
workInProgress.stateNode = createHoistableInstance(
1706+
workInProgress.type,
1707+
workInProgress.pendingProps,
1708+
getRootHostContainer(),
1709+
workInProgress,
1710+
);
1711+
}
17071712
}
1713+
} else {
1714+
// Get Resource may or may not return a resource. either way we stash the result
1715+
// on memoized state.
1716+
workInProgress.memoizedState = getResource(
1717+
workInProgress.type,
1718+
current.memoizedProps,
1719+
workInProgress.pendingProps,
1720+
current.memoizedState,
1721+
);
17081722
}
17091723

17101724
// Resources never have reconciler managed children. It is possible for

‎packages/react-reconciler/src/ReactFiberCompleteWork.js‎

Lines changed: 19 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1052,7 +1052,6 @@ function completeWork(
10521052
return null;
10531053
} else {
10541054
// This is a Hoistable Instance
1055-
10561055
// This must come at the very end of the complete phase.
10571056
bubbleProperties(workInProgress);
10581057
preloadInstanceAndSuspendIfNeeded(
@@ -1064,32 +1063,34 @@ function completeWork(
10641063
return null;
10651064
}
10661065
} else {
1067-
// We are updating.
1068-
const currentResource = current.memoizedState;
1069-
if (nextResource !== currentResource) {
1070-
// We are transitioning to, from, or between Hoistable Resources
1071-
// and require an update
1072-
markUpdate(workInProgress);
1073-
}
1074-
if (nextResource !== null) {
1075-
// This is a Hoistable Resource
1076-
// This must come at the very end of the complete phase.
1077-
1078-
bubbleProperties(workInProgress);
1079-
if (nextResource === currentResource) {
1080-
workInProgress.flags &= ~MaySuspendCommit;
1081-
} else {
1066+
// This is an update.
1067+
if (nextResource) {
1068+
// This is a Resource
1069+
if (nextResource !== current.memoizedState) {
1070+
// we have a new Resource. we need to update
1071+
markUpdate(workInProgress);
1072+
// This must come at the very end of the complete phase.
1073+
bubbleProperties(workInProgress);
1074+
// This must come at the very end of the complete phase, because it might
1075+
// throw to suspend, and if the resource immediately loads, the work loop
1076+
// will resume rendering as if the work-in-progress completed. So it must
1077+
// fully complete.
10821078
preloadResourceAndSuspendIfNeeded(
10831079
workInProgress,
10841080
nextResource,
10851081
type,
10861082
newProps,
10871083
renderLanes,
10881084
);
1085+
return null;
1086+
} else {
1087+
// This must come at the very end of the complete phase.
1088+
bubbleProperties(workInProgress);
1089+
workInProgress.flags &= ~MaySuspendCommit;
1090+
return null;
10891091
}
1090-
return null;
10911092
} else {
1092-
// This is a Hoistable Instance
1093+
// This is an Instance
10931094
// We may have props to update on the Hoistable instance.
10941095
if (supportsMutation) {
10951096
const oldProps = current.memoizedProps;
@@ -1107,7 +1108,6 @@ function completeWork(
11071108
renderLanes,
11081109
);
11091110
}
1110-
11111111
// This must come at the very end of the complete phase.
11121112
bubbleProperties(workInProgress);
11131113
preloadInstanceAndSuspendIfNeeded(

‎scripts/error-codes/codes.json‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -512,5 +512,7 @@
512512
"524": "Values cannot be passed to next() of AsyncIterables passed to Client Components.",
513513
"525": "A React Element from an older version of React was rendered. This is not supported. It can happen if:\n- Multiple copies of the \"react\" package is used.\n- A library pre-bundled an old copy of \"react\" or \"react/jsx-runtime\".\n- A compiler tries to \"inline\" JSX instead of using the runtime.",
514514
"526": "Could not reference an opaque temporary reference. This is likely due to misconfiguring the temporaryReferences options on the server.",
515-
"527": "Incompatible React versions: The \"react\" and \"react-dom\" packages must have the exact same version. Instead got:\n - react: %s\n - react-dom: %s\nLearn more: https://react.dev/warnings/version-mismatch"
515+
"527": "Incompatible React versions: The \"react\" and \"react-dom\" packages must have the exact same version. Instead got:\n - react: %s\n - react-dom: %s\nLearn more: https://react.dev/warnings/version-mismatch",
516+
"528": "Expected <link> not to update to be updated to a stylehsheet with precedence. Check the `rel`, `href`, and `precedence` props of this component. Alternatively, check whether two different <link> components render in the same slot or share the same key.%s",
517+
"529": "Expected stylesheet with precedence to not be updated to a different kind of <link>. Check the `rel`, `href`, and `precedence` props of this component. Alternatively, check whether two different <link> components render in the same slot or share the same key.%s"
516518
}

0 commit comments

Comments
 (0)