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

A more descriptive createElement warning #7402

Closed
wants to merge 10 commits into from
Closed

Conversation

jin
Copy link

@jin jin commented Aug 2, 2016

See #7307

This PR creates a doc to explain the possible causes of a createElement warning when used with invalid types.

@gaearon gaearon self-assigned this Aug 2, 2016
@ghost ghost added the CLA Signed label Aug 2, 2016
@@ -192,7 +192,8 @@ var ReactElementValidator = {
validType,
'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',
'(for composite components). See ' +
'https://facebook.github.io/react/warning/create-element-types.html%s',
Copy link
Member

Choose a reason for hiding this comment

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

I haven't read the actual proposal but assuming we move forward, 2 things:

  1. Please use a short url, we can hook that up later. Let's say fb.me/react-warning-create-element or something similar.
  2. The url should be the last thing in the message so we should have the %s before it.

Copy link
Author

@jin jin Aug 21, 2016

Choose a reason for hiding this comment

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

👍 will do

@jamesblight
Copy link

The original issue mentions showing a better message when the type is undefined. Currently the same error message is shown when the type is null, undefined, bool or a number.

You could check for a type of undefined and show a better error message like:

Warning: React.createEelement: type should not be undefined.
It should be a string (for DOM elements) or a ReactClass (for composite components).
Did you mistype an import or forget to export your component?

@Jessidhia
Copy link
Contributor

or a React.Component, or a React.PureComponent, or a SFC...

There are lots of things that could be valid components :(

@@ -192,7 +192,8 @@ var ReactElementValidator = {
validType,
'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',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of just adding a link here, I would rather add a separate condition checking specifically for undefined and add a more descriptive message for that case. This is the case with most bundlers using Babel: you get undefined when you misspell an import.

…ement.

Main changes:

For an `undefined` component, the following message is shown:

```
Warning: React.createElement: undefined is an invalid value for a type. It should be a string (for DOM elements), ReactClass or React.Component (for composite components). Did you mistype an import or forget to export your component? See fb.me/react-warning-create-element for more information.
```

For other types, the value is shown with a shorter message:

```
Warning: React.createElement: 42 is an invalid value for a type. It should be a string (for DOM elements), ReactClass or React.Component (for composite components). See fb.me/react-warning-create-element for more information.
```

Also added a new example case of an invalid usage of React.createElement.
@jin
Copy link
Author

jin commented Aug 28, 2016

Addressed everyone's feedback (thanks!). The error message now shows the value of the invalid type. Also, if it's undefined, an additional message to verify import/export correctness is also displayed (thanks @jamesblight)

Docs has also been updated with one more example, showcasing an invalid usage of createElement.

@ghost ghost added the CLA Signed label Aug 28, 2016
getDeclarationErrorAddendum()
);
var validType = true;
var warnInvalidType = function(inputValue) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It can be expensive to create closures in such a hot path. Can you please hoist that function outside createElement()?

'It should be a string (for DOM elements), ReactClass or '+
'React.Component (for composite components). ' +
'Did you mistype an import or forget to export your component?' +
'%s See fb.me/react-warning-create-element for more information.',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please write http://fb.me/... instead.

@gaearon
Copy link
Collaborator

gaearon commented Sep 3, 2016

Thanks! I left a few comments.

@ghost ghost added the CLA Signed label Sep 3, 2016
@jin
Copy link
Author

jin commented Sep 4, 2016

@gaearon Done - thank you for the comments!

false,
'React.createElement: undefined is an invalid element type. ' +
'Did you mistype an import or forget to export your component? ' +
'It should be a string (for DOM elements), component ' +
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unnecessary extra indentation here and below. Let's keep lines aligned.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.004%) to 87.196% when pulling 056666d on jin:master into a09d158 on facebook:master.

let Foo, Bar;

if (false) {
Foo = () => <div />;
Copy link
Collaborator

Choose a reason for hiding this comment

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

People reading this might not understand arrow functions. I would avoid them here.

@gaearon
Copy link
Collaborator

gaearon commented Jan 9, 2017

This was fixed in #8612, and is being further improved in #8495.
Thanks for starting the effort!

@gaearon gaearon closed this Jan 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants