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

Improve error messages for invalid element types #8612

Merged
merged 1 commit into from
Dec 21, 2016
Merged

Improve error messages for invalid element types #8612

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
2 changes: 1 addition & 1 deletion scripts/fiber/tests-passing.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ src/isomorphic/classic/element/__tests__/ReactElementValidator-test.js
* does not warn when the element is directly in rest args
* does not warn when the array contains a non-element
* should give context for PropType errors in nested components.
* gives a helpful error when passing null, undefined, boolean, or number
* gives a helpful error when passing invalid types
* should check default prop values
* should not check the default for explicit null
* should check declared prop types
Expand Down
32 changes: 25 additions & 7 deletions src/isomorphic/classic/element/ReactElementValidator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,13 +191,31 @@ var ReactElementValidator = {
// We warn in this case but don't throw. We expect the element creation to
// succeed and there will likely be errors in render.
if (!validType) {
warning(
false,
'React.createElement: type should not be null, undefined, boolean, or ' +
'number. It should be a string (for DOM elements) or a ReactClass ' +
'(for composite components).%s',
getDeclarationErrorAddendum()
);
if (
typeof type !== 'function' &&
typeof type !== 'string'
) {
var info = '';
if (
type === undefined ||
typeof type === 'object' &&
type !== null &&
Object.keys(type).length === 0
) {
info +=
' You likely forgot to export your component from the file ' +
'it\'s defined in.';
}
info += getDeclarationErrorAddendum();
warning(
false,
'React.createElement: type is invalid -- expected a string (for ' +
'built-in components) or a class/function (for composite ' +
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: we don't use "composite" in docs much, maybe "user-defined"?

Copy link

Choose a reason for hiding this comment

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

"custom components" ?

Copy link

Choose a reason for hiding this comment

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

Custom components makes me think of web components (custom web elements). I think user-defined sounds better 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Should “composite components” start becoming part of normal React users lexicon? Even though it may be a bit foreign when a developer first sees and experiences it, it can present an opportunity to start learning about Composite and Host components and how React models things a bit more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just keeping what was already there…

'components) but got: %s.%s',
type == null ? type : typeof type,
info,
);
}
}

var element = ReactElement.createElement.apply(this, arguments);
Expand Down
58 changes: 36 additions & 22 deletions src/isomorphic/classic/element/__tests__/ReactElementValidator-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,35 +286,49 @@ describe('ReactElementValidator', () => {
);
});

it('gives a helpful error when passing null, undefined, boolean, or number', () => {
it('gives a helpful error when passing invalid types', () => {
spyOn(console, 'error');
React.createElement(undefined);
React.createElement(null);
React.createElement(true);
React.createElement(123);
expectDev(console.error.calls.count()).toBe(4);
React.createElement({x: 17});
React.createElement({});
expectDev(console.error.calls.count()).toBe(6);
expectDev(console.error.calls.argsFor(0)[0]).toBe(
'Warning: React.createElement: type should not be null, undefined, ' +
'boolean, or number. It should be a string (for DOM elements) or a ' +
'ReactClass (for composite components).'
'Warning: React.createElement: 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.'
);
expectDev(console.error.calls.argsFor(1)[0]).toBe(
'Warning: React.createElement: type should not be null, undefined, ' +
'boolean, or number. It should be a string (for DOM elements) or a ' +
'ReactClass (for composite components).'
'Warning: React.createElement: type is invalid -- expected a string ' +
'(for built-in components) or a class/function (for composite ' +
'components) but got: null.'
);
expectDev(console.error.calls.argsFor(2)[0]).toBe(
'Warning: React.createElement: type should not be null, undefined, ' +
'boolean, or number. It should be a string (for DOM elements) or a ' +
'ReactClass (for composite components).'
'Warning: React.createElement: type is invalid -- expected a string ' +
'(for built-in components) or a class/function (for composite ' +
'components) but got: boolean.'
);
expectDev(console.error.calls.argsFor(3)[0]).toBe(
'Warning: React.createElement: type should not be null, undefined, ' +
'boolean, or number. It should be a string (for DOM elements) or a ' +
'ReactClass (for composite components).'
'Warning: React.createElement: type is invalid -- expected a string ' +
'(for built-in components) or a class/function (for composite ' +
'components) but got: number.'
);
expectDev(console.error.calls.argsFor(4)[0]).toBe(
'Warning: React.createElement: type is invalid -- expected a string ' +
'(for built-in components) or a class/function (for composite ' +
'components) but got: object.'
);
expectDev(console.error.calls.argsFor(5)[0]).toBe(
'Warning: React.createElement: 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.'
);
React.createElement('div');
expectDev(console.error.calls.count()).toBe(4);
expectDev(console.error.calls.count()).toBe(6);
});

it('includes the owner name when passing null, undefined, boolean, or number', () => {
Expand All @@ -333,10 +347,9 @@ describe('ReactElementValidator', () => {
);
expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toBe(
'Warning: React.createElement: type should not be null, undefined, ' +
'boolean, or number. It should be a string (for DOM elements) or a ' +
'ReactClass (for composite components). Check the render method of ' +
'`ParentComp`.'
'Warning: React.createElement: type is invalid -- expected a string ' +
'(for built-in components) or a class/function (for composite ' +
'components) but got: null. Check the render method of `ParentComp`.'
);
});

Expand Down Expand Up @@ -534,9 +547,10 @@ describe('ReactElementValidator', () => {
void <Foo>{[<div />]}</Foo>;
expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toBe(
'Warning: React.createElement: type should not be null, undefined, ' +
'boolean, or number. It should be a string (for DOM elements) or a ' +
'ReactClass (for composite components).'
'Warning: React.createElement: 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.'
);
});

Expand Down
29 changes: 17 additions & 12 deletions src/isomorphic/modern/element/__tests__/ReactJSXElementValidator-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,21 +256,26 @@ describe('ReactJSXElementValidator', () => {
void <True />;
void <Num />;
expectDev(console.error.calls.count()).toBe(4);
expectDev(console.error.calls.argsFor(0)[0]).toContain(
'type should not be null, undefined, boolean, or number. It should be ' +
'a string (for DOM elements) or a ReactClass (for composite components).'
expectDev(console.error.calls.argsFor(0)[0]).toBe(
'Warning: React.createElement: 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.'
);
expectDev(console.error.calls.argsFor(1)[0]).toContain(
'type should not be null, undefined, boolean, or number. It should be ' +
'a string (for DOM elements) or a ReactClass (for composite components).'
expectDev(console.error.calls.argsFor(1)[0]).toBe(
'Warning: React.createElement: type is invalid -- expected a string ' +
'(for built-in components) or a class/function (for composite ' +
'components) but got: null.'
);
expectDev(console.error.calls.argsFor(2)[0]).toContain(
'type should not be null, undefined, boolean, or number. It should be ' +
'a string (for DOM elements) or a ReactClass (for composite components).'
expectDev(console.error.calls.argsFor(2)[0]).toBe(
'Warning: React.createElement: type is invalid -- expected a string ' +
'(for built-in components) or a class/function (for composite ' +
'components) but got: boolean.'
);
expectDev(console.error.calls.argsFor(3)[0]).toContain(
'type should not be null, undefined, boolean, or number. It should be ' +
'a string (for DOM elements) or a ReactClass (for composite components).'
expectDev(console.error.calls.argsFor(3)[0]).toBe(
'Warning: React.createElement: type is invalid -- expected a string ' +
'(for built-in components) or a class/function (for composite ' +
'components) but got: number.'
);
void <Div />;
expectDev(console.error.calls.count()).toBe(4);
Expand Down
24 changes: 21 additions & 3 deletions src/renderers/shared/fiber/ReactFiber.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,11 @@ function createFiberFromElementType(type : mixed, key : null | string) : Fiber {
} else if (typeof type === 'string') {
fiber = createFiber(HostComponent, key);
fiber.type = type;
} else if (typeof type === 'object' && type !== null) {
} else if (
typeof type === 'object' &&
type !== null &&
typeof type.tag === 'number'
) {
// Currently assumed to be a continuation and therefore is a fiber already.
// TODO: The yield system is currently broken for updates in some cases.
// The reified yield stores a fiber, but we don't know which fiber that is;
Expand All @@ -343,12 +347,26 @@ function createFiberFromElementType(type : mixed, key : null | string) : Fiber {
// There is probably a clever way to restructure this.
fiber = ((type : any) : Fiber);
} else {
let info = '';
if (__DEV__) {
if (
type === undefined ||
typeof type === 'object' &&
type !== null &&
Object.keys(type).length === 0
) {
info +=
' You likely forgot to export your component from the file ' +
'it\'s defined in.';
}
}
// TODO: Stack also includes owner name in the message.
invariant(
false,
'Element type is invalid: expected a string (for built-in components) ' +
'or a class/function (for composite components) but got: %s.',
'or a class/function (for composite components) but got: %s.%s',
type == null ? type : typeof type,
// TODO: Stack also includes owner name in the message.
info,
);
}
return fiber;
Expand Down
12 changes: 9 additions & 3 deletions src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -752,7 +752,9 @@ describe('ReactIncrementalErrorHandling', () => {
ReactNoop.flush();
expect(ReactNoop.getChildren()).toEqual([span(
'Element type is invalid: expected a string (for built-in components) or ' +
'a class/function (for composite components) but got: undefined.'
'a class/function (for composite components) but got: undefined. ' +
'You likely forgot to export your component from the file it\'s ' +
'defined in.'
)]);
expect(console.error.calls.count()).toBe(1);
});
Expand Down Expand Up @@ -794,7 +796,9 @@ describe('ReactIncrementalErrorHandling', () => {
ReactNoop.flush();
expect(ReactNoop.getChildren()).toEqual([span(
'Element type is invalid: expected a string (for built-in components) or ' +
'a class/function (for composite components) but got: undefined.'
'a class/function (for composite components) but got: undefined. ' +
'You likely forgot to export your component from the file it\'s ' +
'defined in.'
)]);
expect(console.error.calls.count()).toBe(1);
});
Expand All @@ -807,7 +811,9 @@ describe('ReactIncrementalErrorHandling', () => {
ReactNoop.flush();
}).toThrowError(
'Element type is invalid: expected a string (for built-in components) or ' +
'a class/function (for composite components) but got: undefined.'
'a class/function (for composite components) but got: undefined. ' +
'You likely forgot to export your component from the file it\'s ' +
'defined in.'
);

ReactNoop.render(<span prop="hi" />);
Expand Down
7 changes: 5 additions & 2 deletions src/renderers/shared/shared/__tests__/ReactComponent-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,9 @@ describe('ReactComponent', () => {
var X = undefined;
expect(() => ReactTestUtils.renderIntoDocument(<X />)).toThrowError(
'Element type is invalid: expected a string (for built-in components) ' +
'or a class/function (for composite components) but got: undefined.'
'or a class/function (for composite components) but got: undefined. ' +
'You likely forgot to export your component from the file it\'s ' +
'defined in.'
);

var Y = null;
Expand All @@ -358,7 +360,8 @@ describe('ReactComponent', () => {
expect(() => ReactTestUtils.renderIntoDocument(<Foo />)).toThrowError(
'Element type is invalid: expected a string (for built-in components) ' +
'or a class/function (for composite components) but got: undefined. ' +
'Check the render method of `Foo`.'
'You likely forgot to export your component from the file it\'s ' +
'defined in. Check the render method of `Foo`.'
);

// One warning for each element creation
Expand Down
35 changes: 27 additions & 8 deletions src/renderers/shared/stack/reconciler/instantiateReactComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,14 +72,33 @@ function instantiateReactComponent(node, shouldHaveDebugID) {
instance = ReactEmptyComponent.create(instantiateReactComponent);
} else if (typeof node === 'object') {
var element = node;
invariant(
element && (typeof element.type === 'function' ||
typeof element.type === 'string'),
'Element type is invalid: expected a string (for built-in components) ' +
'or a class/function (for composite components) but got: %s.%s',
element.type == null ? element.type : typeof element.type,
getDeclarationErrorAddendum(element._owner)
);
var type = element.type;
if (
typeof type !== 'function' &&
typeof type !== 'string'
) {
var info = '';
if (__DEV__) {
if (
type === undefined ||
typeof type === 'object' &&
type !== null &&
Object.keys(type).length === 0
) {
info +=
' You likely forgot to export your component from the file ' +
'it\'s defined in.';
}
}
info += getDeclarationErrorAddendum(element._owner);
invariant(
false,
'Element type is invalid: expected a string (for built-in components) ' +
'or a class/function (for composite components) but got: %s.%s',
type == null ? type : typeof type,
info,
);
}

// Special case string values
if (typeof element.type === 'string') {
Expand Down