Skip to content

Commit 9543a7d

Browse files
gnoffgnoff
committed
[Fiber][Float] Error when a host fiber changes "flavor"
Host Components can exist as four semantic types 1. regular Components (Vanilla) 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 incmplete at best and buggy at worst. The most egregious case is the link type. This can be a regular component (stylesheet without precedence) a hositable 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. This is the most urgent because it is the easiest to hit but doesn't add much overhead in hot paths 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 4ec6a6f commit 9543a7d

File tree

4 files changed

+141
-22
lines changed

4 files changed

+141
-22
lines changed

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

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
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="foo" href="bar" />
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+
expect(errors).toEqual([
64+
'Expected HostHoistable to not change between Instance and Resource type. Try adding a key to this component so that when its props change a new instance is mounted.',
65+
]);
66+
});
67+
68+
it('errors when a hoistable Resource becomes an instance', async () => {
69+
const errors = [];
70+
function onError(e) {
71+
errors.push(e.message);
72+
}
73+
const root = ReactDOMClient.createRoot(container, {
74+
onUncaughtError: onError,
75+
});
76+
77+
root.render(
78+
<div>
79+
<link rel="stylesheet" href="bar" precedence="default" />
80+
</div>,
81+
);
82+
await waitForAll([]);
83+
const event = new window.Event('load');
84+
const preloads = document.querySelectorAll('link[rel="preload"]');
85+
for (let i = 0; i < preloads.length; i++) {
86+
const node = preloads[i];
87+
node.dispatchEvent(event);
88+
}
89+
const stylesheets = document.querySelectorAll('link[rel="preload"]');
90+
for (let i = 0; i < stylesheets.length; i++) {
91+
const node = stylesheets[i];
92+
node.dispatchEvent(event);
93+
}
94+
95+
root.render(
96+
<div>
97+
<link rel="foo" href="bar" />
98+
</div>,
99+
);
100+
await waitForAll([]);
101+
expect(errors).toEqual([
102+
'Expected HostHoistable to not change between Instance and Resource type. Try adding a key to this component so that when its props change a new instance is mounted.',
103+
]);
104+
});
105+
});

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1705,6 +1705,21 @@ function updateHostHoistable(
17051705
workInProgress,
17061706
);
17071707
}
1708+
} else {
1709+
// Once a HostHoistable is a certain "type" like an Instance vs a Resource
1710+
// we can't change it's type. For now we error because this is an edge case and
1711+
// should happen rarely. In the future we will implement an inner type so we
1712+
// can change from one implementation to the next during an update
1713+
if (
1714+
// This Fiber is trying to go from Instance to Resource
1715+
(resource && current.memoizedState === null) ||
1716+
// This Fiber is trying to go from Resource to Instance
1717+
(resource === null && current.memoizedState)
1718+
) {
1719+
throw new Error(
1720+
'Expected HostHoistable to not change between Instance and Resource type. Try adding a key to this component so that when its props change a new instance is mounted.',
1721+
);
1722+
}
17081723
}
17091724

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

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

Lines changed: 19 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1052,9 +1052,6 @@ function completeWork(
10521052
return null;
10531053
} else {
10541054
// This is a Hoistable Instance
1055-
1056-
// This must come at the very end of the complete phase.
1057-
bubbleProperties(workInProgress);
10581055
preloadInstanceAndSuspendIfNeeded(
10591056
workInProgress,
10601057
type,
@@ -1064,32 +1061,34 @@ function completeWork(
10641061
return null;
10651062
}
10661063
} 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 {
1064+
// This is an update.
1065+
if (nextResource) {
1066+
// This is a Resource
1067+
if (nextResource !== current.memoizedState) {
1068+
// we have a new Resource. we need to update
1069+
markUpdate(workInProgress);
1070+
// This must come at the very end of the complete phase.
1071+
bubbleProperties(workInProgress);
1072+
// This must come at the very end of the complete phase, because it might
1073+
// throw to suspend, and if the resource immediately loads, the work loop
1074+
// will resume rendering as if the work-in-progress completed. So it must
1075+
// fully complete.
10821076
preloadResourceAndSuspendIfNeeded(
10831077
workInProgress,
10841078
nextResource,
10851079
type,
10861080
newProps,
10871081
renderLanes,
10881082
);
1083+
return null;
1084+
} else {
1085+
// This must come at the very end of the complete phase.
1086+
bubbleProperties(workInProgress);
1087+
workInProgress.flags &= ~MaySuspendCommit;
1088+
return null;
10891089
}
1090-
return null;
10911090
} else {
1092-
// This is a Hoistable Instance
1091+
// This is an Instance
10931092
// We may have props to update on the Hoistable instance.
10941093
if (supportsMutation) {
10951094
const oldProps = current.memoizedProps;
@@ -1107,7 +1106,6 @@ function completeWork(
11071106
renderLanes,
11081107
);
11091108
}
1110-
11111109
// This must come at the very end of the complete phase.
11121110
bubbleProperties(workInProgress);
11131111
preloadInstanceAndSuspendIfNeeded(

‎scripts/error-codes/codes.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -511,5 +511,6 @@
511511
"523": "The render was aborted due to being postponed.",
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.",
514-
"526": "Could not reference an opaque temporary reference. This is likely due to misconfiguring the temporaryReferences options on the server."
514+
"526": "Could not reference an opaque temporary reference. This is likely due to misconfiguring the temporaryReferences options on the server.",
515+
"527": "Expected HostHoistable to not change between Instance and Resource type. Try adding a key to this component so that when its props change a new instance is mounted."
515516
}

0 commit comments

Comments
 (0)