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

false negative with namespacing/qualifications using jsx-pascal-case #1334

Closed
mfyuce opened this issue Jul 31, 2017 · 17 comments · May be fixed by #2637
Closed

false negative with namespacing/qualifications using jsx-pascal-case #1334

mfyuce opened this issue Jul 31, 2017 · 17 comments · May be fixed by #2637

Comments

@mfyuce
Copy link

mfyuce commented Jul 31, 2017

Using v7.1.0,

<__this.MyComponent/>
<containerClass.MyComponent/>
<containerObject.MyComponent/>

flag an error (e.g Contained JSX component MyComponent must be in PascalCase (react/jsx-pascal-case))

mfyuce pushed a commit to mfyuce/eslint-plugin-react that referenced this issue Jul 31, 2017
… using jsx-pascal

case jsx-eslint#1336 - false postive with names starting with a number using jsx-pascal
[Can besaid it is fixed, but inside it is not accepted anyway.  Seems just fixed the regex]
@ljharb
Copy link
Member

ljharb commented Jul 31, 2017

The docs for that rule don't mention either member expressions (your example) or namespaces, so it's probably an omission.

Both should be accounted for - I believe the spirit of the rule is that anything.PascalCased and anything:PascalCased should both be allowed.

@golopot
Copy link
Contributor

golopot commented May 10, 2019

And anything.camelCase is disallowed, right? Disallowing this will be a breaking change.

@ljharb
Copy link
Member

ljharb commented May 10, 2019

I’d call it a bugfix, not a breaking change.

ljharb pushed a commit to mfyuce/eslint-plugin-react that referenced this issue Dec 30, 2019
… using jsx-pascal

case jsx-eslint#1336 - false postive with names starting with a number using jsx-pascal
[Can besaid it is fixed, but inside it is not accepted anyway.  Seems just fixed the regex]
@yacinehmito
Copy link
Contributor

I am looking to fix this. I think there is a legitimate bug in the jsx-pascal-case rule but before going further I'd need to clarify what the behaviour should be. I tried to look at a spec for how namespaces should be handled but didn't find any authoritative source.

@ljharb Could you tell me what should be allowed / not allowed from this list when the rule is enabled?

  1. __foo.myComponent
  2. __foo.MyComponent
  3. __foo:myComponent
  4. __foo:MyComponent
  5. Foo.myComponent
  6. Foo.MyComponent
  7. Foo:myComponent
  8. Foo:MyComponent

The bug by the way has to do with the fact that when there is foo.bar in the code, we look how foo is formatted, not bar, which seems weird if foo is the namespace and bar the component name. However this might just be an error of understanding on my end.

@ljharb
Copy link
Member

ljharb commented May 9, 2020

@yacinehmito i would expect 2, 4, 6, and 8 to be allowed; and the others not to be.

@yacinehmito
Copy link
Contributor

I also wonder about:

  1. foo:bar
  2. foo.bar

There is some code that suggests that components in all lowercase should always be accepted as they can be plain HTML, but I don't know where the limit lies.
I'm pretty sure foo:bar should be accepted. I don't know about foo.bar.

@ljharb
Copy link
Member

ljharb commented May 9, 2020

No, lowercase components can never be namespaced; i'm pretty sure neither of those are valid.

@yacinehmito
Copy link
Contributor

No, lowercase components can never be namespaced.

Ok. I will update the test cases in #2637 to reflect this then.

Would you humour me though and provide me a source that shows that `svg:circle for example is impossible in React?

yacinehmito added a commit to yacinehmito/eslint-plugin-react that referenced this issue May 9, 2020
@ljharb
Copy link
Member

ljharb commented May 9, 2020

https://itnext.io/using-react-for-xml-svg-470792625278 talks about it - it's not that it's impossible in react, it's that it's impossible in jsx. https://reactjs.org/docs/jsx-in-depth.html is useful as well.

They kind of imply that svg:circle will just be passed to React.createElement, where it'll be treated as html, however. If you want to be certain, whip up a codesandbox that shows what React actually does, and our tests should match that.

@yacinehmito
Copy link
Contributor

Thank you very much for the links.

Should we then error out in the jsx-pascal-case rule or should we ignore that because it's not going to work anyway?
I'd favour the former as the rule might confuse the user as to why the code is not working.

@yacinehmito
Copy link
Contributor

whip up a codesandbox that shows what React actually does

React is not happy about it.

image

@ljharb
Copy link
Member

ljharb commented May 9, 2020

We should warn on anything React won't accept.

@yacinehmito
Copy link
Contributor

yacinehmito commented May 9, 2020

Yes, but should we warn it with a rule that's called jsx-pascal-case? Because in that particular instance, the editor is already telling me something.

This is when I just type it:

image

If I have both the rule react/jsx-no-undef and react/jsx-pascal-case, as a user will I understand what is going on?

Then, if I add a dummy identifier for svg, I have this:

image

Now it's TypeScript that doesn't like it. Again, if I have a syntax error and an error from react/jsx-pascal-case, do I have a clear path forward?

I think this should be addressed with a new rule, that could for example be called jsx-no-namespace. What do you think?

@mfyuce
Copy link
Author

mfyuce commented May 9, 2020

As this typescript issue indicates,

  • namespacing with : is supported by JSX standard (Namespaces are not supported by React [babel-plugin-transform-react-jsx], but Babel correctly parses them [babel-plugin-syntax-jsx]. But tooling seems not supporting it.
  • As it is indicated here, you can React.createElement("image:image", props, children) and it is legal
  • Currently you can
      <svg>
        <circle cx={50} cy={50} r={10} fill="red" />
      </svg>

My humble opinion is namespacing with : (<a:b ...></a:b>) should be supported by jsx, but until tooling is up to date, jsx-no-namespace can only work with dot notation <a.b ...></a.b> or hierarchical notation <a><b> ...</b></a>, but the problem is when the support comes, how it will be faded away? Will it still give an error even if the tooling is supporting it?

@yacinehmito
Copy link
Contributor

yacinehmito commented May 9, 2020

Thank you very much. The TypeScript issue is indeed very helpful to understand the problem.

This make me confident that we should introduce jsx-no-namespace as a rule.

  • It is valid JSX
  • It is only already an error with TypeScript; the linter here would help contextualise that error
  • Without TypeScript it is the only way to statically flag the pattern as problematic with React; this or jsx-pascal-case, which IMO is unsuitable for that purpose.

I'll submit a PR shortly.

@yacinehmito
Copy link
Contributor

PR to add jsx-no-namespace: #2640

@yacinehmito
Copy link
Contributor

the problem is when the support comes, how it will be faded away? Will it still give an error even if the tooling is supporting it?

The linter has ways to detect the version of React that is used. This can inform on whether the rule should be enabled by default or not.

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

Successfully merging a pull request may close this issue.

4 participants