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

Fix #3596: Add warning for components not following proper case convention #7089

Closed
wants to merge 3 commits into from

Conversation

eblin
Copy link

@eblin eblin commented Jun 20, 2016

This adds a warning for when components are mounted with lower case letters rather than uppercase convention used by react.

React's JSX uses the upper vs. lower case convention to distinguish between local component classes and HTML tags.
Docs

It should however NOT throw warnings for valid components as stated in the #3596 issue dicussion.

The warning is only triggered on __DEV__ environments.

The warning logic checks the following:

  • component being rendered is instanceof HTMLUnknownElement
  • component is not a "whitelisted"/supported html tag, like SVG, etc... which are instances of HTMLUnknownElement, React actually already has a list of these supported tags so we just checked against it. ReactDOMFactories.js
  • component does not contain a hyphen or space character. Valid custom HTML elements must contain hyphen characters so we should not warn about it (See spec), also whitespace in component tag will throw its own error thanks to the "sanitize tag" tests/code already in react.

Let me know if I missed something obvious. Thanks!

@ghost
Copy link

ghost commented Jun 20, 2016

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

!ReactDOMFactories.hasOwnProperty(nextElement.type) : false;

warning(!validElement, 'ReactDOM.render(): Invalid component element. ' +
'Make sure the component starts with an upper-case letter.');
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this should include a reference to which element is causing this warning.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, probably a full component trace using ReactComponentTreeDevtool.getStackAddendumByID(debugID)

Copy link
Contributor

@jimfb jimfb Jun 20, 2016

Choose a reason for hiding this comment

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

I might change the message to something like:

'Unknown element %s. Did you misspell an HTML tag name? If you using a custom component, make sure your custom component starts with an upper-case letter.%s', element.type, ReactComponentTreeDevtool.getStackAddendumByID(debugID)

Copy link
Author

Choose a reason for hiding this comment

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

Agree! I was not too happy with the message itself! Thanks for the suggestion, I'll change it to that.

@ghost
Copy link

ghost commented Jun 20, 2016

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@ghost ghost added the CLA Signed label Jun 20, 2016
var container = document.createElement('container');
var myDiv = <div>React</div>;
var mySvg = <svg><polygon points="-145.6 556.9 104.8-145.6 429"/></svg>;
var centered = React.createClass({
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the warning would fire even without this class being defined, right? This is effectively just catching unknown elements?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can just delete this line.

Copy link
Author

Choose a reason for hiding this comment

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

I added the createClassline to test that the warning was actually triggering but yes feel free to delete it.

This is effectively just catching unknown elements

Correct as long as the element.type passes the criteria:

if (element == null || typeof element.type !== 'string' ||
    element.type.match(/\-|\s+/) !== null ||
    ReactDOMFactories.hasOwnProperty(element.type)) {
    return;
  }

…n. Added stack trace and reference to element which causes the warning.
…ctDOMComponentCaseDevtool -> ReactDOMUnknownComponentDevtool

if (!didWarnAboutCase && invalidElement) {
warning(false,
'Unknown element %s. Did you miss-spell an HTML tag name? ' +
Copy link
Member

Choose a reason for hiding this comment

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

miss-spell

Ironically, this is misspelled. You want "misspell".

@gaearon
Copy link
Collaborator

gaearon commented Apr 14, 2017

Going to close this out. #9163 looks like a more recent attempt at the same thing.

@gaearon gaearon closed this Apr 14, 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