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

Rule proposal: warn on <lowerCaseFirstLetterInJSX /> when a binding with this name exists in scope #726

Open
gaearon opened this issue Jul 30, 2016 · 12 comments

Comments

@gaearon
Copy link
Collaborator

gaearon commented Jul 30, 2016

React currently treats JSX types starting with lowercase letter as Web Components. This is documented but beginners often miss that section. I think in 99% cases using a Web Component is not what people want, and it trips them bad.

I propose the following heuristic. If we see <lowerCaseFirstLetterInJSX />, this is not one of the known HTML tags, and a binding with the same name already exists in scope (e.g. as an import, class, function, etc), we are pretty certain the user meant to use their React component, and not a Web Component.

This would not warn:

function MyApp() {
  return <lol /> // no such binding in scope, assuming Web Component
}
function MyApp() {
  var i = 0;
  return <i /> // binding exists but `i` is a known HTML tag
}

This would warn:

import { lol } from 'react-bootstrap'

function MyApp() {
  return <lol /> // likely not what you meant
}
var lol = React.createClass(...)

function MyApp() {
  return <lol /> // likely not what you meant
}

I think flagging any binding is fair game because even if you do intend to use Web Components, having a variable named the same way in the same file is super confusing and just asking for misinterpretation.


I originally filed this as facebook/create-react-app#299, and @alexzherdev would be interested in implementing it.

@gaearon
Copy link
Collaborator Author

gaearon commented Jul 30, 2016

Amended the proposal in response to feedback from @vjeux.

@ljharb
Copy link
Member

ljharb commented Jul 30, 2016

With Web Components, would you not be importing them, or are they implicitly global?

@gaearon
Copy link
Collaborator Author

gaearon commented Jul 30, 2016

They are global. (At least React has no mechanism of using anything other than strings or React components as JSX element types.)

@ljharb
Copy link
Member

ljharb commented Jul 31, 2016

Gotcha.

This seems like it might be more useful as a generic rule that only allows lowercased HTML tag names in JSX, along with a user-provided "allow" list for whatever Web Components they need to use?

@alexzherdev
Copy link
Contributor

alexzherdev commented Jul 31, 2016

I've looked at the history of jsx-pascal-case, and I think it used to do something similar to what is suggested, but stopped doing it here: #329
That issue has the example of a "label" variable, which, being an HTML element name, would be addressed by the whitelist, but there was also an SVG example. I'm not that familiar with SVG, but I guess it also has a limited set of element names, so hypothetically those could be included in the whitelist as well.
The commit that closed that issue went a different way. It added the /^[a-z]|-/ regex to rule out web components with a dash, and anything starting with a lowercase letter. I'm thinking over what would happen if we reverted that logic and replaced it with a whitelist of known names as proposed.

If found <lowerCaseFirstLetter />:
a) it's in the whitelist (div, i, use, path etc.) => ok
b) it's not in the whitelist =>
b.1) there is a corresponding variable in scope => warn
b.2) there's no variable => you're probably using a web component => ok

@yannickcr
Copy link
Member

It would requires us to keep an up to date whitelist but I think it should not be too hard to maintain such list.

Maybe we could do a generic rule about component naming? (and replace jsx-pascal-case which is very specific).

@gaearon
Copy link
Collaborator Author

gaearon commented Jul 31, 2016

What kind of generic rule?

@yannickcr
Copy link
Member

Validate component naming in JSX (must start with an uppercase letter) and having some options to restrict allowed names (pascalCase for example)

@gaearon
Copy link
Collaborator Author

gaearon commented Jul 31, 2016

I’m not sure I understand your proposal. How would you tell between a React component and a Web Component without a whitelist (and/or a heuristic) like in my proposal?

@vjeux
Copy link

vjeux commented Jul 31, 2016

Web components must have a dash somewhere in their name fyi

@yannickcr
Copy link
Member

yannickcr commented Jul 31, 2016

@gaearon Ho, I realize my first message was not very clear.

My suggestion was to do your proposal (with the whitelist) and add what does jsx-pascal-case as an option for this rule.

@ljharb
Copy link
Member

ljharb commented Feb 2, 2022

I'm not sure if this is still relevant.

Unfortunately, we don't currently have a rule that warns on <foo /> for a foo value that isn't a valid HTML element - such a rule would be useful, but since React usually warns on it and it's an obvious mistake, it hasn't been that necessary.

I think such a rule, with an explicit allow list for the web components that users have knowingly globally registered, would be reasonable. It would be most ideal if the list of valid HTML tag names was shared by us and React core - ideally through a common package.

@gaearon if you think this rule would still be valuable, would it be possible to make such a list available?

@alexzherdev are you still willing to implement this rule, assuming the previous question is a yes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

5 participants