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

@yacinehmito yacinehmito commented May 9, 2020

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

@yacinehmito yacinehmito commented May 18, 2020

Are there any concerns about this Pull Request?

@yacinehmito
Copy link
Contributor Author

@yacinehmito yacinehmito commented Jun 18, 2020

@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
Collaborator

@ljharb 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
Collaborator

@ljharb ljharb left a comment

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
Collaborator

@ljharb ljharb Jun 27, 2020

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

@yacinehmito yacinehmito Jun 27, 2020

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
Collaborator

@ljharb ljharb Jun 28, 2020

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
Collaborator

@ljharb ljharb Jun 27, 2020

adding anything to the recommended config is a breaking change.

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

Copy link
Contributor Author

@yacinehmito yacinehmito Jun 27, 2020

I'll remove it then.

@yacinehmito
Copy link
Contributor Author

@yacinehmito yacinehmito commented Jun 27, 2020

#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
Collaborator

@ljharb 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 Aug 16, 2021
@yacinehmito
Copy link
Contributor Author

@yacinehmito yacinehmito commented Sep 10, 2021

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
Collaborator

@ljharb 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 Sep 19, 2021
…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 force-pushed the jsx-no-namespace branch from fbd325a to 1e4dd5b Sep 19, 2021
@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 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 force-pushed the jsx-no-namespace branch from 1e4dd5b to 53a0d84 Sep 19, 2021
@ljharb ljharb marked this pull request as ready for review Sep 19, 2021
ljharb
ljharb approved these changes Sep 19, 2021
@ljharb ljharb merged commit 53a0d84 into yannickcr:master Sep 19, 2021
123 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants