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.

Sign up for GitHub

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

Jump to bottom

Update Float tests to check for specific errors #26367

Merged
merged 1 commit into from
Mar 10, 2023
Merged

Update Float tests to check for specific errors #26367

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 9 additions & 3 deletions packages/internal-test-utils/ReactInternalTestUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ ${diff(expectedLog, actualLog)}
throw error;
}

export async function waitForThrow(expectedError: mixed) {
export async function waitForThrow(expectedError: mixed): mixed {
assertYieldsWereCleared(SchedulerMock);

// Create the error object before doing any async work, to get a better
Expand All @@ -123,8 +123,14 @@ export async function waitForThrow(expectedError: mixed) {
try {
SchedulerMock.unstable_flushAllWithoutAsserting();
} catch (x) {
if (expectedError === undefined) {
// If no expected error was provided, then assume the caller is OK with
// any error being thrown. We're returning the error so they can do
// their own checks, if they wish.
return x;
}
if (equals(x, expectedError)) {
return;
return x;
}
if (
typeof expectedError === 'string' &&
Expand All @@ -133,7 +139,7 @@ export async function waitForThrow(expectedError: mixed) {
typeof x.message === 'string' &&
x.message.includes(expectedError)
) {
return;
return x;
}
error.message = `
Expected error was not thrown.
Expand Down
192 changes: 100 additions & 92 deletions packages/react-dom/src/__tests__/ReactDOMFloat-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import {

let JSDOM;
let Stream;
let Scheduler;
let React;
let ReactDOM;
let ReactDOMClient;
Expand All @@ -34,7 +33,7 @@ let hasErrored = false;
let fatalError = undefined;
let renderOptions;
let waitForAll;
let assertLog;
let waitForThrow;

function resetJSDOM(markup) {
// Test Environment
Expand All @@ -57,7 +56,6 @@ describe('ReactDOMFloat', () => {
beforeEach(() => {
jest.resetModules();
JSDOM = require('jsdom').JSDOM;
Scheduler = require('scheduler');
React = require('react');
ReactDOM = require('react-dom');
ReactDOMClient = require('react-dom/client');
Expand All @@ -67,7 +65,7 @@ describe('ReactDOMFloat', () => {

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

textCache = new Map();

Expand Down Expand Up @@ -261,23 +259,6 @@ describe('ReactDOMFloat', () => {
);
}

function renderSafelyAndExpect(root, children) {
root.render(children);
return expect(() => {
try {
// TODO: Migrate this to waitForAll()
Scheduler.unstable_flushAll();
assertLog([]);
} catch (e) {
try {
// TODO: Migrate this to waitForAll()
Scheduler.unstable_flushAll();
assertLog([]);
} catch (f) {}
}
});
}

// @gate enableFloat
it('can render resources before singletons', async () => {
const root = ReactDOMClient.createRoot(document);
Expand Down Expand Up @@ -382,94 +363,127 @@ describe('ReactDOMFloat', () => {
it('warns if you render resource-like elements above <head> or <body>', async () => {
const root = ReactDOMClient.createRoot(document);

renderSafelyAndExpect(
root,
<>
<noscript>foo</noscript>
<html>
<body>foo</body>
</html>
</>,
).toErrorDev(
await expect(async () => {
root.render(
<>
<noscript>foo</noscript>
<html>
<body>foo</body>
</html>
</>,
);
const aggregateError = await waitForThrow();
expect(aggregateError.errors.length).toBe(2);
expect(aggregateError.errors[0].message).toContain(
'Invalid insertion of NOSCRIPT',
);
expect(aggregateError.errors[1].message).toContain(
'The node to be removed is not a child of this node',
);
Comment on lines +376 to +382
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can remove the !DEV gate too now

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Huh I was wondering why the negative flag check wouldn't have caught that and it's because enableFloat is true in all our builds, so the combined gate condition is always true. Can we just clean up the flag now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Given that this already rolled out at Meta I vote for landing the enableFloat flag and removing the associated gates from all our tests. Can do in a follow up.

}).toErrorDev(
[
'Cannot render <noscript> outside the main document. Try moving it into the root <head> tag.',
'Warning: validateDOMNesting(...): <noscript> cannot appear as a child of <#document>.',
],
{withoutStack: 1},
);

renderSafelyAndExpect(
root,
<html>
<template>foo</template>
<body>foo</body>
</html>,
).toErrorDev([
await expect(async () => {
root.render(
<html>
<template>foo</template>
<body>foo</body>
</html>,
);
await waitForAll([]);
}).toErrorDev([
'Cannot render <template> outside the main document. Try moving it into the root <head> tag.',
'Warning: validateDOMNesting(...): <template> cannot appear as a child of <html>.',
]);

renderSafelyAndExpect(
root,
<html>
<body>foo</body>
<style>foo</style>
</html>,
).toErrorDev([
await expect(async () => {
root.render(
<html>
<body>foo</body>
<style>foo</style>
</html>,
);
await waitForAll([]);
}).toErrorDev([
'Cannot render a <style> outside the main document without knowing its precedence and a unique href key. React can hoist and deduplicate <style> tags if you provide a `precedence` prop along with an `href` prop that does not conflic with the `href` values used in any other hoisted <style> or <link rel="stylesheet" ...> tags. Note that hoisting <style> tags is considered an advanced feature that most will not use directly. Consider moving the <style> tag to the <head> or consider adding a `precedence="default"` and `href="some unique resource identifier"`, or move the <style> to the <style> tag.',
'Warning: validateDOMNesting(...): <style> cannot appear as a child of <html>.',
]);

renderSafelyAndExpect(
root,
<>
<html>
<body>foo</body>
</html>
<link rel="stylesheet" href="foo" />
</>,
).toErrorDev(
await expect(async () => {
root.render(
<>
<html>
<body>foo</body>
</html>
<link rel="stylesheet" href="foo" />
</>,
);
const aggregateError = await waitForThrow();
expect(aggregateError.errors.length).toBe(2);
expect(aggregateError.errors[0].message).toContain(
'Invalid insertion of LINK',
);
expect(aggregateError.errors[1].message).toContain(
'The node to be removed is not a child of this node',
);
}).toErrorDev(
[
'Cannot render a <link rel="stylesheet" /> outside the main document without knowing its precedence. Consider adding precedence="default" or moving it into the root <head> tag.',
'Warning: validateDOMNesting(...): <link> cannot appear as a child of <#document>.',
],
{withoutStack: 1},
);

renderSafelyAndExpect(
root,
<>
<html>
<body>foo</body>
<script href="foo" />
</html>
</>,
).toErrorDev([
await expect(async () => {
root.render(
<>
<html>
<body>foo</body>
<script href="foo" />
</html>
</>,
);
await waitForAll([]);
}).toErrorDev([
'Cannot render a sync or defer <script> outside the main document without knowing its order. Try adding async="" or moving it into the root <head> tag.',
'Warning: validateDOMNesting(...): <script> cannot appear as a child of <html>.',
]);

renderSafelyAndExpect(
root,
<>
await expect(async () => {
root.render(
<html>
<script async={true} onLoad={() => {}} href="bar" />
<body>foo</body>
</html>
</>,
).toErrorDev([
</html>,
);
await waitForAll([]);
}).toErrorDev([
'Cannot render a <script> with onLoad or onError listeners outside the main document. Try removing onLoad={...} and onError={...} or moving it into the root <head> tag or somewhere in the <body>.',
]);

renderSafelyAndExpect(
root,
<>
<link rel="foo" onLoad={() => {}} href="bar" />
<html>
<body>foo</body>
</html>
</>,
).toErrorDev(
await expect(async () => {
root.render(
<>
<link rel="foo" onLoad={() => {}} href="bar" />
<html>
<body>foo</body>
</html>
</>,
);
const aggregateError = await waitForThrow();
expect(aggregateError.errors.length).toBe(2);
expect(aggregateError.errors[0].message).toContain(
'Invalid insertion of LINK',
);
expect(aggregateError.errors[1].message).toContain(
'The node to be removed is not a child of this node',
);
}).toErrorDev(
[
'Cannot render a <link> with onLoad or onError listeners outside the main document. Try removing onLoad={...} and onError={...} or moving it into the root <head> tag or somewhere in the <body>.',
],
Expand Down Expand Up @@ -5638,22 +5652,16 @@ background-color: green;
},
},
);
try {
await expect(async () => {
await waitForAll([]);
}).toErrorDev(
[
'Warning: Text content did not match. Server: "server" Client: "client"',
'Warning: An error occurred during hydration. The server HTML was replaced with client content in <#document>.',
],
{withoutStack: 1},
);
} catch (e) {
// When gates are false this test fails on a DOMException if you don't clear the scheduler after catching.
// When gates are true this branch should not be hit

await expect(async () => {
await waitForAll([]);
throw e;
}
}).toErrorDev(
[
'Warning: Text content did not match. Server: "server" Client: "client"',
'Warning: An error occurred during hydration. The server HTML was replaced with client content in <#document>.',
],
{withoutStack: 1},
);
expect(getMeaningfulChildren(document)).toEqual(
<html>
<head>
Expand Down