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

add warnings for non-resources rendered outside body or head #25532

Merged
merged 4 commits into from
Oct 22, 2022
Merged

add warnings for non-resources rendered outside body or head #25532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
65127d0
add warnings for non-resources rendered outside body or head
gnoff Oct 22, 2022
bd78683
forks
gnoff Oct 22, 2022
0fb69cf
gates
gnoff Oct 22, 2022
a066705
hide hostcontexttype by exposing a getter for ancestorInfo.resourceFo…
gnoff Oct 22, 2022
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
56 changes: 54 additions & 2 deletions packages/react-dom-bindings/src/client/ReactDOMFloatClient.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,11 @@ import {
markNodeAsResource,
} from './ReactDOMComponentTree';
import {HTML_NAMESPACE} from '../shared/DOMNamespaces';
import {getCurrentRootHostContainer} from 'react-reconciler/src/ReactFiberHostContext';
import {
getCurrentRootHostContainer,
getHostContext,
} from 'react-reconciler/src/ReactFiberHostContext';
import {getResourceFormOnly} from './validateDOMNesting';

// The resource types we support. currently they match the form for the as argument.
// In the future this may need to change, especially when modules / scripts are supported
Expand Down Expand Up @@ -1331,6 +1335,11 @@ function insertResourceInstanceBefore(
}

export function isHostResourceType(type: string, props: Props): boolean {
let resourceFormOnly: boolean;
if (__DEV__) {
const hostContext = getHostContext();
resourceFormOnly = getResourceFormOnly(hostContext);
}
switch (type) {
case 'meta':
case 'title': {
Expand All @@ -1339,14 +1348,29 @@ export function isHostResourceType(type: string, props: Props): boolean {
case 'link': {
const {onLoad, onError} = props;
if (onLoad || onError) {
if (__DEV__) {
if (resourceFormOnly) {
console.error(
'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>.',
);
}
}
return false;
}
switch (props.rel) {
case 'stylesheet': {
const {href, precedence, disabled} = props;
if (__DEV__) {
validateLinkPropsForStyleResource(props);
if (typeof precedence !== 'string' && resourceFormOnly) {
console.error(
'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.',
);
}
}
const {href, precedence, disabled} = props;
return (
typeof href === 'string' &&
typeof precedence === 'string' &&
Expand All @@ -1363,8 +1387,36 @@ export function isHostResourceType(type: string, props: Props): boolean {
// We don't validate because it is valid to use async with onLoad/onError unlike combining
// precedence with these for style resources
const {src, async, onLoad, onError} = props;
if (__DEV__) {
if (async !== true && resourceFormOnly) {
console.error(
'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.',
);
} else if ((onLoad || onError) && resourceFormOnly) {
console.error(
'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>.',
);
}
}
return (async: any) && typeof src === 'string' && !onLoad && !onError;
}
case 'base':
case 'template':
case 'style':
case 'noscript': {
if (__DEV__) {
if (resourceFormOnly) {
console.error(
'Cannot render <%s> outside the main document. Try moving it into the root <head> tag.',
type,
);
}
}
return false;
}
}
return false;
}
Expand Down
13 changes: 12 additions & 1 deletion packages/react-dom-bindings/src/client/validateDOMNesting.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

let validateDOMNesting = () => {};
let updatedAncestorInfo = () => {};
let getResourceFormOnly = () => false;

if (__DEV__) {
// This validation code was written based on the HTML5 parsing spec:
Expand Down Expand Up @@ -153,6 +154,8 @@ if (__DEV__) {

listItemTagAutoclosing: null,
dlItemTagAutoclosing: null,

resourceFormOnly: true,
};

updatedAncestorInfo = function(oldInfo, tag) {
Expand Down Expand Up @@ -180,6 +183,10 @@ if (__DEV__) {
ancestorInfo.dlItemTagAutoclosing = null;
}

if (tag !== '#document' && tag !== 'html') {
ancestorInfo.resourceFormOnly = false;
}

ancestorInfo.current = info;

if (tag === 'form') {
Expand Down Expand Up @@ -472,6 +479,10 @@ if (__DEV__) {
);
}
};

getResourceFormOnly = hostContextDev => {
return hostContextDev.ancestorInfo.resourceFormOnly;
};
}

export {updatedAncestorInfo, validateDOMNesting};
export {updatedAncestorInfo, validateDOMNesting, getResourceFormOnly};
112 changes: 112 additions & 0 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 @@ -268,6 +268,118 @@ describe('ReactDOMFloat', () => {
);
});

function renderSafelyAndExpect(root, children) {
root.render(children);
return expect(() => {
try {
expect(Scheduler).toFlushWithoutYielding();
} catch (e) {
try {
expect(Scheduler).toFlushWithoutYielding();
} catch (f) {}
}
});
}

// @gate enableFloat || !__DEV__
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(
[
'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([
'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([
'Cannot render <style> outside the main document. Try moving it into the root <head> tag.',
'Warning: validateDOMNesting(...): <style> cannot appear as a child of <html>.',
]);

renderSafelyAndExpect(
root,
<>
<html>
<body>foo</body>
</html>
<link rel="stylesheet" href="foo" />
</>,
).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([
'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,
<>
<html>
<script async={true} onLoad={() => {}} href="bar" />
<body>foo</body>
</html>
</>,
).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(
[
'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>.',
],
{withoutStack: 1},
);
});

// @gate enableFloat
it('can acquire a resource after releasing it in the same commit', async () => {
const root = ReactDOMClient.createRoot(container);
Expand Down
16 changes: 13 additions & 3 deletions packages/react-reconciler/src/ReactFiberHostContext.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,25 @@
* @flow
*/

import type {Container} from './ReactFiberHostConfig';
import type {Container, HostContext} from './ReactFiberHostConfig';
import {enableNewReconciler} from 'shared/ReactFeatureFlags';

import {getCurrentRootHostContainer as getCurrentRootHostContainer_old} from './ReactFiberHostContext.old';
import {
getCurrentRootHostContainer as getCurrentRootHostContainer_old,
getHostContext as getHostContext_old,
} from './ReactFiberHostContext.old';

import {getCurrentRootHostContainer as getCurrentRootHostContainer_new} from './ReactFiberHostContext.new';
import {
getCurrentRootHostContainer as getCurrentRootHostContainer_new,
getHostContext as getHostContext_new,
} from './ReactFiberHostContext.new';

export function getCurrentRootHostContainer(): null | Container {
return enableNewReconciler
? getCurrentRootHostContainer_new()
: getCurrentRootHostContainer_old();
}

export function getHostContext(): HostContext {
return enableNewReconciler ? getHostContext_new() : getHostContext_old();
}
4 changes: 2 additions & 2 deletions packages/react-reconciler/src/ReactFiberHostContext.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ function requiredContext<Value>(c: Value | NoContextT): Value {

function getCurrentRootHostContainer(): null | Container {
const container = rootInstanceStackCursor.current;
return container === NO_CONTEXT ? null : (container: any);
return container === NO_CONTEXT ? null : ((container: any): Container);
}

function getRootHostContainer(): Container {
Expand Down Expand Up @@ -106,8 +106,8 @@ function popHostContext(fiber: Fiber): void {
}

export {
getCurrentRootHostContainer,
getHostContext,
getCurrentRootHostContainer,
getRootHostContainer,
popHostContainer,
popHostContext,
Expand Down
4 changes: 2 additions & 2 deletions packages/react-reconciler/src/ReactFiberHostContext.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ function requiredContext<Value>(c: Value | NoContextT): Value {

function getCurrentRootHostContainer(): null | Container {
const container = rootInstanceStackCursor.current;
return container === NO_CONTEXT ? null : (container: any);
return container === NO_CONTEXT ? null : ((container: any): Container);
}

function getRootHostContainer(): Container {
Expand Down Expand Up @@ -106,8 +106,8 @@ function popHostContext(fiber: Fiber): void {
}

export {
getCurrentRootHostContainer,
getHostContext,
getCurrentRootHostContainer,
getRootHostContainer,
popHostContainer,
popHostContext,
Expand Down