Warn if the rendered DOM node is HTMLUnknownElement

[radubrehar]

I've just been bit by

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

and as an after-thought I remembered something about lower-case/upper-case jsx tags. As a good React citizen, I've searched for it on the React site and found it documented here: https://facebook.github.io/react/docs/jsx-in-depth.html

I'm not new to React, but this took me around 10 minutes to trace down. I generally follow the convention, but I just developed a simple centered component for a mostly static site, that simply clones it's children and adds
margin: 0 auto; max-width: 900 to each of them. So I thought of this as a simple utility and therefore I lower-cased it.

I understand this might not change, but it's worth at least adding a warning on uncommon html tag names, or better yet, on lowercase jsx tags that are also local variables.

[jimfb]

We generally treat warnings as "non-fatal errors", meaning that we try to only warn for things "should be fixed". Once upon a time, we enumerated all the "known html attributes", and have been backpedaling that mistake ever since. We allow any lower-case tag, because we can't distinguish it from a valid html tag, and using a new tag that we've never heard of shouldn't be an "error".

With regards to warning if there is a variable in scope: It's very common to say var div = <div style=...>My great content</div>. Now you would have a variable in scope with the same lower-case name as a tag, and this clearly shouldn't generate warnings [or maybe it should :)].

Anyway, If you can think of a way to handle the problem of separating "tags" from "components", I'd be interested in hearing your thoughts.

[radubrehar]

What about adding warnings only for those local vars that hold a reference to classes that extend React.Component? cc @spicyj

doing

var div = <div>My content</div>

//usage
<article>{div}</article>

makes div a React Element
while the following

var centered = class extends React.Component { ... }
<article>
    <centered>
        <p>content here</p>
   </centered>
</article>

makes usage of a React.Component subclass, so there is a clear separation between the two.

[jimfb]

Yeah, I could get behind that idea. I'm curious what @spicyj says.

[jimfb]

It could be a bit expensive though. We would probably want to do the check only for unrecognized html tags (ie. have a whitelist). And we'd probably want to avoid the check in production. The logic for when we would warn starts to get a little messy.

[zpao]

There's nothing to do in production, this is all part of the JSX transform step. Even if we did do something and it ended up in JSXTransformer, that's fine, it's not for production anyway.

This is definitely interesting but might really be quite difficult, we would need to track every variable that might be in scope. I don't think that's possible with most transformers and you really need something like Flow to understand how everything is used (imagine passing function foo around which renders <centered>, centered isn't in scope in the module but it is when you call foo in another module). Even if you just want to capture local scope, that's not perfect and will be slow. You would never be defining a class is actual "local" scope of the render function. You could capture a couple cases (passing a component class via props) but not much else.

Or imagine you have a custom-element called centered which you actually wanted to render, not some local variable. Some other developer came along and did a precalculation and assigned something called centered because it made sense, not realizing it would affect the render. We don't actually know what was intended.

Since there is so much state and intention outside of the transform that we can't capture, I don't think this is realistic to actually do.

[radubrehar]

Transforming <div> to js is done with React.createElement('div', props, ...children)
For the sake of keeping it short, let's assume it is transformed to React.createElement('div', props, children, config) (not proposing a new API, just to show my point). When this transform is performed I'm sure the transformer knows if it's lowercase or uppercase. So if it's lowercase, we could transform to React.createElement('div', props, children, { warnIfComponent: true }), or, for <centered> to React.createElement(centered, props.children, {warnIfComponent: true})

Now, in development React.createElement can check if the first argument is a React Element or a React Component, and if it is, and warnIfComponent is true, it could show a warning.
Seems straight-forward to me, wondering if I'm missing something obvious :) In this way there is no need to track variables in scope, which indeed would be too dirty.

[sophiebits]

I was thinking we could warn if the rendered element is an HTMLUnknownElement:

http://ryanmorr.com/determine-html5-element-support-in-javascript/

[brigand]

@spicyj that would solve problems beyond this +1. The warning can be suppressed with document.registerElement, so there's no noise if the user actually intended it.

[gaearon]

I was thinking we could warn if the rendered element is an HTMLUnknownElement:
http://ryanmorr.com/determine-html5-element-support-in-javascript/

If this works, I think it’s a better solution than transforms especially considering that most people are using Babel today anyway. 👍

[radubrehar]

Thanks @gaearon for the input! I will try to pick this up and solve it next week!

[aweary]

@gaearon @spicyj one issue I see with checking if the node is an instance of HTMLUnknownElement is that jsdom will return instances of HTMLUnkownElement for elements it doesn't support (like SVG) so this would throw false positives in test suites that use it.

[sophiebits]

Hm that's a good point. :(

[kevinSuttle]

@aweary I wonder if the same would be true for Web Components, Brick, Polymer, etc.

[aweary]

@kevinSuttle I would guess it would be, there's an issue open for jsdom regarding web components: jsdom/jsdom#1030

[gaearon]

We do warn now.