OSSForks/react
1
0
Fork 0
mirror of https://github.com/zebrajr/react.git synced 2025-12-06 12:20:20 +01:00
Code Issues Actions 5 Packages Projects Releases Wiki Activity

Warn for invalid type in renderer with the correct RSC stack (#30102)

Browse Source 
This is all behind the `enableOwnerStacks` flag.

This is a follow up to #29088. In that I moved type validation into the
renderer since that's the one that knows what types are allowed.
However, I only removed it from `React.createElement` and not the JSX
which was an oversight.

However, I also noticed that for invalid types we don't have the right
stack trace for throws because we're not yet inside the JSX element that
itself is invalid. We should use its stack for the stack trace. That's
the reason it's enough to just use the throw now because we can get a
good stack trace from the owner stack. This is fixed by creating a fake
Throw Fiber that gets assigned the right stack.

Additionally, I noticed that for certain invalid types like the most
common one `undefined` we error in Flight so a missing import in RSC
leads to a generic error. Instead of erroring on the Flight side we
should just let anything that's not a Server Component through to the
client and then let the Client renderer determine whether it's a valid
type or not. Since we now have owner stacks through the server too, this
will still be able to provide a good stack trace on the client that
points to the server in that case.

<img width="571" alt="Screenshot 2024-06-25 at 6 46 35 PM"
src="https://github.com/facebook/react/assets/63648/6812c24f-e274-4e09-b4de-21deda9ea1d4">

To get the best stack you have to expand the little icon and the regular
stack is noisy [due to this Chrome
bug](https://issues.chromium.org/issues/345248263) which makes it a
little harder to find but once that's fixed it might be easier.
This commit is contained in:
Sebastian Markbåge 2024-06-27 18:10:09 +02:00 committed by GitHub
parent ffec9ec5b5
commit e02baf6c92
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
12 changed files with 249 additions and 182 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

22
packages/react-client/src/__tests__/ReactFlight-test.js vendored
View File

@ -692,14 +692,22 @@ describe('ReactFlight', () => {
const transport = ReactNoopFlightServer.render(<ServerComponent />);
await act(async () => {
const rootModel = await ReactNoopFlightClient.read(transport);
ReactNoop.render(rootModel);
});
expect(ReactNoop).toMatchRenderedOutput('Loading...');
spyOnDevAndProd(console, 'error').mockImplementation(() => {});
await load();
expect(console.error).toHaveBeenCalledTimes(1);
await expect(async () => {
await act(async () => {
const rootModel = await ReactNoopFlightClient.read(transport);
ReactNoop.render(rootModel);
});
}).rejects.toThrow(
__DEV__
? 'Element type is invalid: expected a string (for built-in components) or a class/function ' +
'(for composite components) but got: <div />. ' +
'Did you accidentally export a JSX literal instead of a component?'
: 'Element type is invalid: expected a string (for built-in components) or a class/function ' +
'(for composite components) but got: object.',
);
expect(ReactNoop).toMatchRenderedOutput(null);
});
it('can render a lazy element', async () => {

84
packages/react-dom/src/__tests__/ReactComponent-test.js vendored
View File

@ -14,6 +14,7 @@ let ReactDOM;
let ReactDOMClient;
let ReactDOMServer;
let act;
let assertConsoleErrorDev;
describe('ReactComponent', () => {
beforeEach(() => {
@ -24,6 +25,8 @@ describe('ReactComponent', () => {
ReactDOMClient = require('react-dom/client');
ReactDOMServer = require('react-dom/server');
act = require('internal-test-utils').act;
assertConsoleErrorDev =
require('internal-test-utils').assertConsoleErrorDev;
});
// @gate !disableLegacyMode
@ -131,8 +134,6 @@ describe('ReactComponent', () => {
// @gate !disableStringRefs
it('string refs do not detach and reattach on every render', async () => {
spyOnDev(console, 'error').mockImplementation(() => {});
let refVal;
class Child extends React.Component {
componentDidUpdate() {
@ -171,6 +172,8 @@ describe('ReactComponent', () => {
root.render(<Parent />);
});
assertConsoleErrorDev(['contains the string ref']);
expect(refVal).toBe(undefined);
await act(() => {
root.render(<Parent showChild={true} />);
@ -511,19 +514,25 @@ describe('ReactComponent', () => {
});
it('throws usefully when rendering badly-typed elements', async () => {
const container = document.createElement('div');
const root = ReactDOMClient.createRoot(container);
const X = undefined;
let container = document.createElement('div');
let root = ReactDOMClient.createRoot(container);
await expect(
expect(async () => {
await act(() => {
root.render(<X />);
});
}).toErrorDev(
'React.jsx: type is invalid -- expected a string (for built-in components) ' +
'or a class/function (for composite components) but got: undefined.',
),
).rejects.toThrowError(
const XElement = <X />;
if (gate(flags => !flags.enableOwnerStacks)) {
assertConsoleErrorDev(
[
'React.jsx: type is invalid -- expected a string (for built-in components) ' +
'or a class/function (for composite components) but got: undefined.',
],
{withoutStack: true},
);
}
await expect(async () => {
await act(() => {
root.render(XElement);
});
}).rejects.toThrowError(
'Element type is invalid: expected a string (for built-in components) ' +
'or a class/function (for composite components) but got: undefined.' +
(__DEV__
@ -533,21 +542,44 @@ describe('ReactComponent', () => {
);
const Y = null;
container = document.createElement('div');
root = ReactDOMClient.createRoot(container);
await expect(
expect(async () => {
await act(() => {
root.render(<Y />);
});
}).toErrorDev(
'React.jsx: type is invalid -- expected a string (for built-in components) ' +
'or a class/function (for composite components) but got: null.',
),
).rejects.toThrowError(
const YElement = <Y />;
if (gate(flags => !flags.enableOwnerStacks)) {
assertConsoleErrorDev(
[
'React.jsx: type is invalid -- expected a string (for built-in components) ' +
'or a class/function (for composite components) but got: null.',
],
{withoutStack: true},
);
}
await expect(async () => {
await act(() => {
root.render(YElement);
});
}).rejects.toThrowError(
'Element type is invalid: expected a string (for built-in components) ' +
'or a class/function (for composite components) but got: null.',
);
const Z = true;
const ZElement = <Z />;
if (gate(flags => !flags.enableOwnerStacks)) {
assertConsoleErrorDev(
[
'React.jsx: type is invalid -- expected a string (for built-in components) ' +
'or a class/function (for composite components) but got: boolean.',
],
{withoutStack: true},
);
}
await expect(async () => {
await act(() => {
root.render(ZElement);
});
}).rejects.toThrowError(
'Element type is invalid: expected a string (for built-in components) ' +
'or a class/function (for composite components) but got: boolean.',
);
});
it('includes owner name in the error about badly-typed elements', async () => {

32
packages/react-dom/src/__tests__/ReactDOMServerIntegrationElements-test.js vendored
View File

@ -987,11 +987,13 @@ describe('ReactDOMServerIntegration', () => {
expect(() => {
EmptyComponent = <EmptyComponent />;
}).toErrorDev(
'React.jsx: type is invalid -- expected a string ' +
'(for built-in components) or a class/function (for composite ' +
'components) but got: object. You likely forgot to export your ' +
"component from the file it's defined in, or you might have mixed up " +
'default and named imports.',
gate(flags => flags.enableOwnerStacks)
? []
: 'React.jsx: type is invalid -- expected a string ' +
'(for built-in components) or a class/function (for composite ' +
'components) but got: object. You likely forgot to export your ' +
"component from the file it's defined in, or you might have mixed up " +
'default and named imports.',
{withoutStack: true},
);
await render(EmptyComponent);
@ -1011,9 +1013,11 @@ describe('ReactDOMServerIntegration', () => {
expect(() => {
NullComponent = <NullComponent />;
}).toErrorDev(
'React.jsx: type is invalid -- expected a string ' +
'(for built-in components) or a class/function (for composite ' +
'components) but got: null.',
gate(flags => flags.enableOwnerStacks)
? []
: 'React.jsx: type is invalid -- expected a string ' +
'(for built-in components) or a class/function (for composite ' +
'components) but got: null.',
{withoutStack: true},
);
await render(NullComponent);
@ -1029,11 +1033,13 @@ describe('ReactDOMServerIntegration', () => {
expect(() => {
UndefinedComponent = <UndefinedComponent />;
}).toErrorDev(
'React.jsx: type is invalid -- expected a string ' +
'(for built-in components) or a class/function (for composite ' +
'components) but got: undefined. You likely forgot to export your ' +
"component from the file it's defined in, or you might have mixed up " +
'default and named imports.',
gate(flags => flags.enableOwnerStacks)
? []
: 'React.jsx: type is invalid -- expected a string ' +
'(for built-in components) or a class/function (for composite ' +
'components) but got: undefined. You likely forgot to export your ' +
"component from the file it's defined in, or you might have mixed up " +
'default and named imports.',
{withoutStack: true},
);

57
packages/react-dom/src/__tests__/ReactLegacyErrorBoundaries-test.internal.js
View File

@ -13,6 +13,7 @@ let PropTypes;
let React;
let ReactDOM;
let act;
let assertConsoleErrorDev;
// TODO: Refactor this test once componentDidCatch setState is deprecated.
describe('ReactLegacyErrorBoundaries', () => {
@ -42,6 +43,8 @@ describe('ReactLegacyErrorBoundaries', () => {
ReactDOM = require('react-dom');
React = require('react');
act = require('internal-test-utils').act;
assertConsoleErrorDev =
require('internal-test-utils').assertConsoleErrorDev;
log = [];
@ -2099,32 +2102,38 @@ describe('ReactLegacyErrorBoundaries', () => {
const Y = undefined;
await expect(async () => {
await expect(async () => {
const container = document.createElement('div');
await act(() => {
ReactDOM.render(<X />, container);
});
}).rejects.toThrow('got: null');
}).toErrorDev(
'React.jsx: type is invalid -- expected a string ' +
'(for built-in components) or a class/function ' +
'(for composite components) but got: null.',
{withoutStack: 1},
);
const container = document.createElement('div');
await act(() => {
ReactDOM.render(<X />, container);
});
}).rejects.toThrow('got: null');
if (gate(flags => !flags.enableOwnerStacks)) {
assertConsoleErrorDev(
[
'React.jsx: type is invalid -- expected a string ' +
'(for built-in components) or a class/function ' +
'(for composite components) but got: null.',
],
{withoutStack: true},
);
}
await expect(async () => {
await expect(async () => {
const container = document.createElement('div');
await act(() => {
ReactDOM.render(<Y />, container);
});
}).rejects.toThrow('got: undefined');
}).toErrorDev(
'React.jsx: type is invalid -- expected a string ' +
'(for built-in components) or a class/function ' +
'(for composite components) but got: undefined.',
{withoutStack: 1},
);
const container = document.createElement('div');
await act(() => {
ReactDOM.render(<Y />, container);
});
}).rejects.toThrow('got: undefined');
if (gate(flags => !flags.enableOwnerStacks)) {
assertConsoleErrorDev(
[
'React.jsx: type is invalid -- expected a string ' +
'(for built-in components) or a class/function ' +
'(for composite components) but got: undefined.',
],
{withoutStack: true},
);
}
});
// @gate !disableLegacyMode

6
packages/react-reconciler/src/ReactChildFiber.js vendored
View File

@ -220,6 +220,9 @@ function validateFragmentProps(
// For unkeyed root fragments there's no Fiber. We create a fake one just for
// error stack handling.
fiber = createFiberFromElement(element, returnFiber.mode, 0);
if (__DEV__) {
fiber._debugInfo = currentDebugInfo;
}
fiber.return = returnFiber;
}
runWithFiberInDEV(
@ -242,6 +245,9 @@ function validateFragmentProps(
// For unkeyed root fragments there's no Fiber. We create a fake one just for
// error stack handling.
fiber = createFiberFromElement(element, returnFiber.mode, 0);
if (__DEV__) {
fiber._debugInfo = currentDebugInfo;
}
fiber.return = returnFiber;
}
runWithFiberInDEV(fiber, () => {

10
packages/react-reconciler/src/ReactFiber.js vendored
View File

@ -485,6 +485,7 @@ export function createHostRootFiber(
return createFiber(HostRoot, null, null, mode);
}
// TODO: Get rid of this helper. Only createFiberFromElement should exist.
export function createFiberFromTypeAndProps(
type: any, // React$ElementType
key: null | string,
@ -650,11 +651,18 @@ export function createFiberFromTypeAndProps(
typeString = type === null ? 'null' : typeof type;
}
throw new Error(
// The type is invalid but it's conceptually a child that errored and not the
// current component itself so we create a virtual child that throws in its
// begin phase. This is the same thing we do in ReactChildFiber if we throw
// but we do it here so that we can assign the debug owner and stack from the
// element itself. That way the error stack will point to the JSX callsite.
fiberTag = Throw;
pendingProps = new Error(
'Element type is invalid: expected a string (for built-in ' +
'components) or a class/function (for composite components) ' +
`but got: ${typeString}.${info}`,
);
resolvedType = null;
}
}
}

23
packages/react-reconciler/src/__tests__/ErrorBoundaryReconciliation-test.internal.js
View File

@ -6,6 +6,7 @@ describe('ErrorBoundaryReconciliation', () => {
let ReactTestRenderer;
let span;
let act;
let assertConsoleErrorDev;
beforeEach(() => {
jest.resetModules();
@ -13,6 +14,8 @@ describe('ErrorBoundaryReconciliation', () => {
ReactTestRenderer = require('react-test-renderer');
React = require('react');
act = require('internal-test-utils').act;
assertConsoleErrorDev =
require('internal-test-utils').assertConsoleErrorDev;
DidCatchErrorBoundary = class extends React.Component {
state = {error: null};
componentDidCatch(error) {
@ -58,15 +61,17 @@ describe('ErrorBoundaryReconciliation', () => {
);
});
expect(renderer).toMatchRenderedOutput(<span prop="BrokenRender" />);
await expect(async () => {
await act(() => {
renderer.update(
<ErrorBoundary fallbackTagName={fallbackTagName}>
<BrokenRender fail={true} />
</ErrorBoundary>,
);
});
}).toErrorDev(['invalid', 'invalid']);
await act(() => {
renderer.update(
<ErrorBoundary fallbackTagName={fallbackTagName}>
<BrokenRender fail={true} />
</ErrorBoundary>,
);
});
if (gate(flags => !flags.enableOwnerStacks)) {
assertConsoleErrorDev(['invalid', 'invalid']);
}
const Fallback = fallbackTagName;
expect(renderer).toMatchRenderedOutput(<Fallback prop="ErrorBoundary" />);
}

42
packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js
View File

@ -19,6 +19,7 @@ let assertLog;
let waitForAll;
let waitFor;
let waitForThrow;
let assertConsoleErrorDev;
describe('ReactIncrementalErrorHandling', () => {
beforeEach(() => {
@ -28,6 +29,8 @@ describe('ReactIncrementalErrorHandling', () => {
ReactNoop = require('react-noop-renderer');
Scheduler = require('scheduler');
act = require('internal-test-utils').act;
assertConsoleErrorDev =
require('internal-test-utils').assertConsoleErrorDev;
const InternalTestUtils = require('internal-test-utils');
assertLog = InternalTestUtils.assertLog;
@ -1237,11 +1240,15 @@ describe('ReactIncrementalErrorHandling', () => {
<BrokenRender />
</ErrorBoundary>,
);
await expect(async () => await waitForAll([])).toErrorDev([
'React.jsx: type is invalid -- expected a string',
// React retries once on error
'React.jsx: type is invalid -- expected a string',
]);
await waitForAll([]);
if (gate(flags => !flags.enableOwnerStacks)) {
assertConsoleErrorDev([
'React.jsx: type is invalid -- expected a string',
// React retries once on error
'React.jsx: type is invalid -- expected a string',
]);
}
expect(ReactNoop).toMatchRenderedOutput(
<span
prop={
@ -1288,11 +1295,14 @@ describe('ReactIncrementalErrorHandling', () => {
<BrokenRender fail={true} />
</ErrorBoundary>,
);
await expect(async () => await waitForAll([])).toErrorDev([
'React.jsx: type is invalid -- expected a string',
// React retries once on error
'React.jsx: type is invalid -- expected a string',
]);
await waitForAll([]);
if (gate(flags => !flags.enableOwnerStacks)) {
assertConsoleErrorDev([
'React.jsx: type is invalid -- expected a string',
// React retries once on error
'React.jsx: type is invalid -- expected a string',
]);
}
expect(ReactNoop).toMatchRenderedOutput(
<span
prop={
@ -1310,10 +1320,14 @@ describe('ReactIncrementalErrorHandling', () => {
it('recovers from uncaught reconciler errors', async () => {
const InvalidType = undefined;
expect(() => ReactNoop.render(<InvalidType />)).toErrorDev(
'React.jsx: type is invalid -- expected a string',
{withoutStack: true},
);
ReactNoop.render(<InvalidType />);
if (gate(flags => !flags.enableOwnerStacks)) {
assertConsoleErrorDev(
['React.jsx: type is invalid -- expected a string'],
{withoutStack: true},
);
}
await waitForThrow(
'Element type is invalid: expected a string (for built-in components) or ' +
'a class/function (for composite components) but got: undefined.' +

91
packages/react-server/src/ReactFlightServer.js vendored
View File

@ -110,7 +110,6 @@ import {
} from 'shared/ReactSymbols';
import {
describeValueForErrorMessage,
describeObjectForErrorMessage,
isSimpleObject,
jsxPropsParents,
@ -1501,19 +1500,11 @@ function renderElement(
jsxChildrenParents.set(props.children, type);
}
}
if (typeof type === 'function') {
if (isClientReference(type) || isOpaqueTemporaryReference(type)) {
// This is a reference to a Client Component.
return renderClientElement(
task,
type,
key,
props,
owner,
stack,
validated,
);
}
if (
typeof type === 'function' &&
!isClientReference(type) &&
!isOpaqueTemporaryReference(type)
) {
// This is a Server Component.
return renderFunctionComponent(
request,
@ -1525,43 +1516,27 @@ function renderElement(
stack,
validated,
);
} else if (typeof type === 'string') {
// This is a host element. E.g. HTML.
return renderClientElement(task, type, key, props, owner, stack, validated);
} else if (typeof type === 'symbol') {
if (type === REACT_FRAGMENT_TYPE && key === null) {
// For key-less fragments, we add a small optimization to avoid serializing
// it as a wrapper.
const prevImplicitSlot = task.implicitSlot;
if (task.keyPath === null) {
task.implicitSlot = true;
}
const json = renderModelDestructive(
request,
task,
emptyRoot,
'',
props.children,
);
task.implicitSlot = prevImplicitSlot;
return json;
}
// This might be a built-in React component. We'll let the client decide.
// Any built-in works as long as its props are serializable.
return renderClientElement(task, type, key, props, owner, stack, validated);
} else if (type != null && typeof type === 'object') {
if (isClientReference(type)) {
// This is a reference to a Client Component.
return renderClientElement(
task,
type,
key,
props,
owner,
stack,
validated,
);
} else if (type === REACT_FRAGMENT_TYPE && key === null) {
// For key-less fragments, we add a small optimization to avoid serializing
// it as a wrapper.
const prevImplicitSlot = task.implicitSlot;
if (task.keyPath === null) {
task.implicitSlot = true;
}
const json = renderModelDestructive(
request,
task,
emptyRoot,
'',
props.children,
);
task.implicitSlot = prevImplicitSlot;
return json;
} else if (
type != null &&
typeof type === 'object' &&
!isClientReference(type)
) {
switch (type.$$typeof) {
case REACT_LAZY_TYPE: {
let wrappedType;
@ -1615,11 +1590,21 @@ function renderElement(
validated,
);
}
case REACT_ELEMENT_TYPE: {
// This is invalid but we'll let the client determine that it is.
if (__DEV__) {
// Disable the key warning that would happen otherwise because this
// element gets serialized inside an array. We'll error later anyway.
type._store.validated = 1;
}
}
}
}
throw new Error(
`Unsupported Server Component type: ${describeValueForErrorMessage(type)}`,
);
// For anything else, try it on the client instead.
// We don't know if the client will support it or not. This might error on the
// client or error during serialization but the stack will point back to the
// server.
return renderClientElement(task, type, key, props, owner, stack, validated);
}
function pingTask(request: Request, task: Task): void {

14
packages/react/src/__tests__/ReactElementValidator-test.internal.js
View File

@ -515,11 +515,15 @@ describe('ReactElementValidator', () => {
expect(() => {
void (<Foo>{[<div />]}</Foo>);
}).toErrorDev(
'React.jsx: type is invalid -- expected a string ' +
'(for built-in components) or a class/function (for composite ' +
'components) but got: undefined. You likely forgot to export your ' +
"component from the file it's defined in, or you might have mixed up " +
'default and named imports.',
gate(flags => flags.enableOwnerStacks)
? []
: [
'React.jsx: type is invalid -- expected a string ' +
'(for built-in components) or a class/function (for composite ' +
'components) but got: undefined. You likely forgot to export your ' +
"component from the file it's defined in, or you might have mixed up " +
'default and named imports.',
],
{withoutStack: true},
);
});

29
packages/react/src/__tests__/ReactJSXElementValidator-test.js
View File

@ -215,35 +215,6 @@ describe('ReactJSXElementValidator', () => {
);
});
it('gives a helpful error when passing null, undefined, or boolean', () => {
const Undefined = undefined;
const Null = null;
const True = true;
const Div = 'div';
expect(() => void (<Undefined />)).toErrorDev(
'React.jsx: type is invalid -- expected a string ' +
'(for built-in components) or a class/function (for composite ' +
'components) but got: undefined. You likely forgot to export your ' +
"component from the file it's defined in, or you might have mixed up " +
'default and named imports.',
{withoutStack: true},
);
expect(() => void (<Null />)).toErrorDev(
'React.jsx: type is invalid -- expected a string ' +
'(for built-in components) or a class/function (for composite ' +
'components) but got: null.',
{withoutStack: true},
);
expect(() => void (<True />)).toErrorDev(
'React.jsx: type is invalid -- expected a string ' +
'(for built-in components) or a class/function (for composite ' +
'components) but got: boolean.',
{withoutStack: true},
);
// No error expected
void (<Div />);
});
it('warns for fragments with illegal attributes', async () => {
class Foo extends React.Component {
render() {

21
packages/react/src/jsx/ReactJSXElement.js
View File

@ -559,9 +559,14 @@ function jsxDEVImpl(
debugTask,
) {
if (__DEV__) {
if (!isValidElementType(type)) {
if (!enableOwnerStacks && !isValidElementType(type)) {
// This is an invalid element type.
//
// We warn here so that we can get better stack traces but with enableOwnerStacks
// enabled we don't need this because we get good stacks if we error in the
// renderer anyway. The renderer is the only one that knows what types are valid
// for this particular renderer so we let it error there instead.
//
// We warn in this case but don't throw. We expect the element creation to
// succeed and there will likely be errors in render.
let info = '';
@ -604,6 +609,9 @@ function jsxDEVImpl(
// errors. We don't want exception behavior to differ between dev and
// prod. (Rendering will throw with a helpful message and as soon as the
// type is fixed, the key warnings will appear.)
// When enableOwnerStacks is on, we no longer need the type here so this
// comment is no longer true. Which is why we can run this even for invalid
// types.
const children = config.children;
if (children !== undefined) {
if (isStaticChildren) {
@ -1103,6 +1111,17 @@ export function cloneElement(element, config, children) {
*/
function validateChildKeys(node, parentType) {
if (__DEV__) {
if (enableOwnerStacks) {
// When owner stacks is enabled no warnings happens. All we do is
// mark elements as being in a valid static child position so they
// don't need keys.
if (isValidElement(node)) {
if (node._store) {
node._store.validated = 1;
}
}
return;
}
if (typeof node !== 'object' || !node) {
return;
}