Skip to content

Commit

Permalink
[Fiber][Float] Error when a host fiber changes "flavor"
Browse files Browse the repository at this point in the history
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
  • Loading branch information
gnoff committed May 31, 2024
1 parent 4ec6a6f commit 9543a7d
Show file tree
Hide file tree
Showing 4 changed files with 141 additions and 22 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
/**
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @emails react-core
* @jest-environment ./scripts/jest/ReactDOMServerIntegrationEnvironment
*/

'use strict';

let JSDOM;
let React;
let ReactDOMClient;
let container;
let waitForAll;

describe('ReactDOM HostSingleton', () => {
beforeEach(() => {
jest.resetModules();
JSDOM = require('jsdom').JSDOM;
// Test Environment
const jsdom = new JSDOM(
'<!DOCTYPE html><html><head></head><body><div id="container">',
{
runScripts: 'dangerously',
},
);
global.window = jsdom.window;
global.document = jsdom.window.document;
container = global.document.getElementById('container');

React = require('react');
ReactDOMClient = require('react-dom/client');

const InternalTestUtils = require('internal-test-utils');
waitForAll = InternalTestUtils.waitForAll;
});

it('errors when a hoistable component becomes a Resource', async () => {
const errors = [];
function onError(e) {
errors.push(e.message);
}
const root = ReactDOMClient.createRoot(container, {
onUncaughtError: onError,
});

root.render(
<div>
<link rel="foo" href="bar" />
</div>,
);
await waitForAll([]);

root.render(
<div>
<link rel="stylesheet" href="bar" precedence="default" />
</div>,
);
await waitForAll([]);
expect(errors).toEqual([
'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.',
]);
});

it('errors when a hoistable Resource becomes an instance', async () => {
const errors = [];
function onError(e) {
errors.push(e.message);
}
const root = ReactDOMClient.createRoot(container, {
onUncaughtError: onError,
});

root.render(
<div>
<link rel="stylesheet" href="bar" precedence="default" />
</div>,
);
await waitForAll([]);
const event = new window.Event('load');
const preloads = document.querySelectorAll('link[rel="preload"]');
for (let i = 0; i < preloads.length; i++) {
const node = preloads[i];
node.dispatchEvent(event);
}
const stylesheets = document.querySelectorAll('link[rel="preload"]');
for (let i = 0; i < stylesheets.length; i++) {
const node = stylesheets[i];
node.dispatchEvent(event);
}

root.render(
<div>
<link rel="foo" href="bar" />
</div>,
);
await waitForAll([]);
expect(errors).toEqual([
'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.',
]);
});
});
15 changes: 15 additions & 0 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -1705,6 +1705,21 @@ function updateHostHoistable(
workInProgress,
);
}
} else {
// Once a HostHoistable is a certain "type" like an Instance vs a Resource
// we can't change it's type. For now we error because this is an edge case and
// should happen rarely. In the future we will implement an inner type so we
// can change from one implementation to the next during an update
if (
// This Fiber is trying to go from Instance to Resource
(resource && current.memoizedState === null) ||
// This Fiber is trying to go from Resource to Instance
(resource === null && current.memoizedState)
) {
throw new Error(
'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.',
);
}
}

// Resources never have reconciler managed children. It is possible for
Expand Down
40 changes: 19 additions & 21 deletions packages/react-reconciler/src/ReactFiberCompleteWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -1052,9 +1052,6 @@ function completeWork(
return null;
} else {
// This is a Hoistable Instance

// This must come at the very end of the complete phase.
bubbleProperties(workInProgress);
preloadInstanceAndSuspendIfNeeded(
workInProgress,
type,
Expand All @@ -1064,32 +1061,34 @@ function completeWork(
return null;
}
} else {
// We are updating.
const currentResource = current.memoizedState;
if (nextResource !== currentResource) {
// We are transitioning to, from, or between Hoistable Resources
// and require an update
markUpdate(workInProgress);
}
if (nextResource !== null) {
// This is a Hoistable Resource
// This must come at the very end of the complete phase.

bubbleProperties(workInProgress);
if (nextResource === currentResource) {
workInProgress.flags &= ~MaySuspendCommit;
} else {
// This is an update.
if (nextResource) {
// This is a Resource
if (nextResource !== current.memoizedState) {
// we have a new Resource. we need to update
markUpdate(workInProgress);
// This must come at the very end of the complete phase.
bubbleProperties(workInProgress);
// This must come at the very end of the complete phase, because it might
// throw to suspend, and if the resource immediately loads, the work loop
// will resume rendering as if the work-in-progress completed. So it must
// fully complete.
preloadResourceAndSuspendIfNeeded(
workInProgress,
nextResource,
type,
newProps,
renderLanes,
);
return null;
} else {
// This must come at the very end of the complete phase.
bubbleProperties(workInProgress);
workInProgress.flags &= ~MaySuspendCommit;
return null;
}
return null;
} else {
// This is a Hoistable Instance
// This is an Instance
// We may have props to update on the Hoistable instance.
if (supportsMutation) {
const oldProps = current.memoizedProps;
Expand All @@ -1107,7 +1106,6 @@ function completeWork(
renderLanes,
);
}

// This must come at the very end of the complete phase.
bubbleProperties(workInProgress);
preloadInstanceAndSuspendIfNeeded(
Expand Down
3 changes: 2 additions & 1 deletion scripts/error-codes/codes.json
Original file line number Diff line number Diff line change
Expand Up @@ -511,5 +511,6 @@
"523": "The render was aborted due to being postponed.",
"524": "Values cannot be passed to next() of AsyncIterables passed to Client Components.",
"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.",
"526": "Could not reference an opaque temporary reference. This is likely due to misconfiguring the temporaryReferences options on the server."
"526": "Could not reference an opaque temporary reference. This is likely due to misconfiguring the temporaryReferences options on the server.",
"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."
}

0 comments on commit 9543a7d

Please sign in to comment.