Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fiber][Float] Error when a host fiber changes "flavor" #29693

Merged
merged 1 commit into from
Jun 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 80 additions & 1 deletion packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js
Original file line number Diff line number Diff line change
Expand Up @@ -2358,6 +2358,7 @@ export function getResource(
type: string,
currentProps: any,
pendingProps: any,
currentResource: null | Resource,
): null | Resource {
const resourceRoot = getCurrentResourceRoot();
if (!resourceRoot) {
Expand Down Expand Up @@ -2430,9 +2431,44 @@ export function getResource(
);
}
}
if (currentProps && currentResource === null) {
// This node was previously an Instance type and is becoming a Resource type
// For now we error because we don't support flavor changes
let diff = '';
if (__DEV__) {
diff = `

- ${describeLinkForResourceErrorDEV(currentProps)}
+ ${describeLinkForResourceErrorDEV(pendingProps)}`;
}
throw new Error(
'Expected <link> not to update to be updated to a stylehsheet with precedence.' +
Copy link
Contributor

@unstubbable unstubbable Jun 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'Expected <link> not to update to be updated to a stylehsheet with precedence.' +
'Expected <link> to not be updated to a stylesheet 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.' +
diff,
);
}
return resource;
} else {
if (currentProps && currentResource !== null) {
// This node was previously a Resource type and is becoming an Instance type
// For now we error because we don't support flavor changes
let diff = '';
if (__DEV__) {
diff = `

- ${describeLinkForResourceErrorDEV(currentProps)}
+ ${describeLinkForResourceErrorDEV(pendingProps)}`;
}
throw new Error(
'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.' +
diff,
);
}
return null;
}
return null;
}
case 'script': {
const async = pendingProps.async;
Expand Down Expand Up @@ -2473,6 +2509,49 @@ export function getResource(
}
}

function describeLinkForResourceErrorDEV(props: any) {
if (__DEV__) {
let describedProps = 0;

let description = '<link';
if (typeof props.rel === 'string') {
describedProps++;
description += ` rel="${props.rel}"`;
} else if (hasOwnProperty.call(props, 'rel')) {
describedProps++;
description += ` rel="${
props.rel === null ? 'null' : 'invalid type ' + typeof props.rel
}"`;
}
if (typeof props.href === 'string') {
describedProps++;
description += ` href="${props.href}"`;
} else if (hasOwnProperty.call(props, 'href')) {
describedProps++;
description += ` href="${
props.href === null ? 'null' : 'invalid type ' + typeof props.href
}"`;
}
if (typeof props.precedence === 'string') {
describedProps++;
description += ` precedence="${props.precedence}"`;
} else if (hasOwnProperty.call(props, 'precedence')) {
describedProps++;
description += ` precedence={${
props.precedence === null
? 'null'
: 'invalid type ' + typeof props.precedence
}}`;
}
if (Object.getOwnPropertyNames(props).length > describedProps) {
description += ' ...';
}
description += ' />';
return description;
}
return '';
}

function styleTagPropsFromRawProps(
rawProps: StyleTagQualifyingProps,
): StyleTagProps {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
/**
* 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="preload" href="bar" as="style" />
</div>,
);
await waitForAll([]);

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

@unstubbable unstubbable Jun 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`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.
`Expected <link> to not be updated to a stylesheet 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.


- <link rel=\"preload\" href=\"bar\" ... />
+ <link rel=\"stylesheet\" href=\"bar\" precedence=\"default\" />`,
]);
} else {
expect(errors).toEqual([
'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.',
Copy link
Contributor

@unstubbable unstubbable Jun 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'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.',
'Expected <link> to not be updated to a stylesheet 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.',

]);
}
});

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([]);
if (__DEV__) {
expect(errors).toEqual([
`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.

- <link rel=\"stylesheet\" href=\"bar\" precedence=\"default\" />
+ <link rel=\"foo\" href=\"bar\" />`,
]);
} else {
expect(errors).toEqual([
'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.',
]);
}
});
});
42 changes: 28 additions & 14 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -1689,22 +1689,36 @@ function updateHostHoistable(
renderLanes: Lanes,
) {
markRef(current, workInProgress);
const currentProps = current === null ? null : current.memoizedProps;
const resource = (workInProgress.memoizedState = getResource(
workInProgress.type,
currentProps,
workInProgress.pendingProps,
));

if (current === null) {
if (!getIsHydrating() && resource === null) {
// This is not a Resource Hoistable and we aren't hydrating so we construct the instance.
workInProgress.stateNode = createHoistableInstance(
workInProgress.type,
workInProgress.pendingProps,
getRootHostContainer(),
workInProgress,
);
const resource = getResource(
workInProgress.type,
null,
workInProgress.pendingProps,
null,
);
if (resource) {
workInProgress.memoizedState = resource;
} else {
if (!getIsHydrating()) {
// This is not a Resource Hoistable and we aren't hydrating so we construct the instance.
workInProgress.stateNode = createHoistableInstance(
workInProgress.type,
workInProgress.pendingProps,
getRootHostContainer(),
workInProgress,
);
}
}
} else {
// Get Resource may or may not return a resource. either way we stash the result
// on memoized state.
workInProgress.memoizedState = getResource(
workInProgress.type,
current.memoizedProps,
workInProgress.pendingProps,
current.memoizedState,
);
}

// Resources never have reconciler managed children. It is possible for
Expand Down
38 changes: 19 additions & 19 deletions packages/react-reconciler/src/ReactFiberCompleteWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -1052,7 +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);
gnoff marked this conversation as resolved.
Show resolved Hide resolved
preloadInstanceAndSuspendIfNeeded(
Expand All @@ -1064,32 +1063,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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What all this refactoring about? Does it do anything?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We no longer support transitioning between types (we error in begin work) so we can simplify the update logic. If we have a resource now then we must have had one before. If we don't have one now we must have not had one before.

this removes a duplicate check of nextResource !== currentResource.

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 +1108,6 @@ function completeWork(
renderLanes,
);
}

// This must come at the very end of the complete phase.
bubbleProperties(workInProgress);
preloadInstanceAndSuspendIfNeeded(
Expand Down
4 changes: 3 additions & 1 deletion scripts/error-codes/codes.json
Original file line number Diff line number Diff line change
Expand Up @@ -512,5 +512,7 @@
"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.",
"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"
"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",
"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",
Copy link
Contributor

@unstubbable unstubbable Jun 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"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",
"528": "Expected <link> to not be updated to a stylesheet 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",

"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"
}