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

[New] add no-namespace rule #2640

Merged
merged 2 commits into from Sep 19, 2021
Merged

Conversation

yacinehmito
Copy link
Contributor

Following the discussion in #1334 (comment), I am submitting the introduction of the rule jsx-no-namespace.

Summary: JSX namespaces are not supported by React. When a JSX namespace is used in a JSX component, the rules jsx-no-undef or jsx-pascal-case raise an error, which is unfortunate because none of them explain what is happening.

This rule is not of critical importance because the error is flagged at runtime by React. Still, I believe it to be a valuable inclusion.

@yacinehmito
Copy link
Contributor Author

Are there any concerns about this Pull Request?

@yacinehmito
Copy link
Contributor Author

@ljharb I am sorry to ping you but I'd like to direct your attention to this month-old PR.
I think it is the proper path forward to really fix #1334

@ljharb
Copy link
Member

ljharb commented Jun 27, 2020

#1334 can't truly be fixed except by modifying jsx-pascal-case somehow. I'll take a look at this PR tho.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Let's make sure we have test cases that also run in babel-eslint, and also in the typescript parser.


## When not to use

If you are not using JSX.
Copy link
Member

Choose a reason for hiding this comment

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

this seems like it shouldn't be jsx-specific, since it can still apply to React.createElement when called with a string literal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am willing to make the linting rule work with React.createElement.
It wouldn't be called jsx-no-namespace anymore though. Do you have a suggestion for a name?

Copy link
Member

Choose a reason for hiding this comment

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

I think just no-namespace would work?

index.js Outdated
@@ -126,6 +127,7 @@ module.exports = {
'react/jsx-key': 2,
'react/jsx-no-comment-textnodes': 2,
'react/jsx-no-duplicate-props': 2,
'react/jsx-no-namespace': 2,
Copy link
Member

Choose a reason for hiding this comment

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

adding anything to the recommended config is a breaking change.

Suggested change
'react/jsx-no-namespace': 2,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remove it then.

@yacinehmito
Copy link
Contributor Author

#1334 can't truly be fixed except by modifying jsx-pascal-case somehow.

Indeed, and I have a draft PR with a potential fix (#2637), but I'd like to get this one sorted out first.

@ljharb ljharb changed the title Introduce jsx-no-namespace [New] Introduce jsx-no-namespace Oct 15, 2020
@ljharb
Copy link
Member

ljharb commented Aug 9, 2021

@yacinehmito are you interested in landing this PR? it seems almost ready, pending the review comments.

@ljharb ljharb marked this pull request as draft August 16, 2021 22:08
@yacinehmito
Copy link
Contributor Author

Hello @ljharb. My apologies, I am not using GitHub much anymore.
I don't think I can find the time to finish this; I am no longer using React and am swamped with other work. My sincere apologies.

Other people are free to build upon this work.

@ljharb
Copy link
Member

ljharb commented Sep 10, 2021

I’ll try to finish it myself then.

@ljharb ljharb reopened this Sep 10, 2021
ljharb and others added 2 commits September 19, 2021 14:24
…agmaImport` utils

This should improve detection in the following rules:
 - `button-has-type`
 - `forbid-elements`
 - `no-adjacent-inline-elements`
 - `no-children-prop`
 - `style-prop-object`
Co-authored-by: Yacine Hmito <yacine.hmito@gmail.com>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
@ljharb ljharb changed the title [New] Introduce jsx-no-namespace [New] add no-namespace rule Sep 19, 2021
@ljharb ljharb removed the question label Sep 19, 2021
@codecov-commenter
Copy link

codecov-commenter commented Sep 19, 2021

Codecov Report

Merging #2640 (53a0d84) into master (76fdffe) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2640   +/-   ##
=======================================
  Coverage   97.43%   97.43%           
=======================================
  Files         111      114    +3     
  Lines        7522     7536   +14     
  Branches     2763     2750   -13     
=======================================
+ Hits         7329     7343   +14     
  Misses        193      193           
Impacted Files Coverage Δ
index.js 100.00% <ø> (ø)
lib/rules/button-has-type.js 96.66% <100.00%> (-0.40%) ⬇️
lib/rules/forbid-elements.js 100.00% <100.00%> (ø)
lib/rules/no-adjacent-inline-elements.js 97.22% <100.00%> (+0.07%) ⬆️
lib/rules/no-children-prop.js 100.00% <100.00%> (ø)
lib/rules/no-namespace.js 100.00% <100.00%> (ø)
lib/rules/style-prop-object.js 100.00% <100.00%> (ø)
lib/util/Components.js 98.87% <100.00%> (-0.11%) ⬇️
lib/util/isCreateElement.js 100.00% <100.00%> (ø)
lib/util/isDestructuredFromPragmaImport.js 100.00% <100.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 76fdffe...53a0d84. Read the comment docs.

@ljharb ljharb marked this pull request as ready for review September 19, 2021 21:45
@ljharb ljharb merged commit 53a0d84 into jsx-eslint:master Sep 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants