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-invalid-html-attribute rule #2863

Merged
merged 1 commit into from Nov 2, 2021

Conversation

Nokel81
Copy link
Contributor

@Nokel81 Nokel81 commented Nov 30, 2020

This rule (and fixer) checks the "rel" value to make sure that it correctly relates to the tag that the "rel" property is on.

Co-authored-by: Sebastian Malton <sebastian@malton.name>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
lib/rules/jsx-no-invalid-rel.js Outdated Show resolved Hide resolved
tests/lib/rules/jsx-no-invalid-rel.js Outdated Show resolved Hide resolved
lib/rules/jsx-no-invalid-rel.js Outdated Show resolved Hide resolved
tests/lib/rules/jsx-no-invalid-rel.js Outdated Show resolved Hide resolved
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.

Plus all of @golopot's comments.

docs/rules/jsx-no-invalid-rel.md Outdated Show resolved Hide resolved
tests/lib/rules/jsx-no-invalid-rel.js Outdated Show resolved Hide resolved
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.

Are there other HTML element attributes that this type of rule might work on?

It might be nicer to make this rule a generic no-invalid-html-attribute rule (one that also works on React.createElement, and thus isn't jsx-specific), with an option for a list of attributes to check (that would default to ['rel']). That way, we could add support for a new attribute in a semver-minor without needing to add an entirely new rule.

Thoughts?

docs/rules/jsx-no-invalid-rel.md Outdated Show resolved Hide resolved
lib/rules/jsx-no-invalid-rel.js Outdated Show resolved Hide resolved
@ljharb ljharb marked this pull request as draft December 30, 2020 04:14
@Nokel81
Copy link
Contributor Author

Nokel81 commented Jan 7, 2021

@ljharb Good idea about making this a more generic rule, though I don't know if I want to make it working with React.createElement in this PR.

@Nokel81 Nokel81 marked this pull request as ready for review January 22, 2021 17:21
@Nokel81
Copy link
Contributor Author

Nokel81 commented Jan 22, 2021

@ljharb I have changed the name of the rule and added the option as you said. While reworking the internals a bit (though I haven't tested the code with non-rel items)

@ljharb ljharb changed the title [New Rule]: jsx-no-invalid-rel [New Rule]: jsx-no-invalid-html-attribute Jan 22, 2021
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.

Thanks, this is looking much better

docs/rules/no-invalid-html-attribute.md Outdated Show resolved Hide resolved
lib/rules/no-invalid-html-attribute.js Outdated Show resolved Hide resolved
lib/rules/no-invalid-html-attribute.js Outdated Show resolved Hide resolved
lib/rules/no-invalid-html-attribute.js Outdated Show resolved Hide resolved
@Nokel81
Copy link
Contributor Author

Nokel81 commented Jan 22, 2021

I know that there are some failing tests for some older versions of eslint. Will look at them soon.

@ljharb ljharb marked this pull request as draft January 22, 2021 23:46
@ljharb
Copy link
Member

ljharb commented Jan 22, 2021

No rush, please mark as ready for review once tests are passing.

@Nokel81 Nokel81 marked this pull request as ready for review January 25, 2021 16:44
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.

Thanks, this looks great! (sorry for the review delay)

This rule isn't named "jsx-", but it doesn't check React.createElement. I think it probably should, though. Is that something you'd mind adding?

@Nokel81 Nokel81 changed the title [New Rule]: jsx-no-invalid-html-attribute [New Rule]: no-invalid-html-attribute Mar 23, 2021
@Nokel81
Copy link
Contributor Author

Nokel81 commented Mar 23, 2021

@ljharb I have added checking React.createElement.

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.

Are there other attributes besides "rel" we can add here?

@codecov-commenter
Copy link

codecov-commenter commented Jun 22, 2021

Codecov Report

Merging #2863 (ce77747) into master (9f1d618) will decrease coverage by 0.06%.
The diff coverage is 93.10%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2863      +/-   ##
==========================================
- Coverage   97.22%   97.15%   -0.07%     
==========================================
  Files         110      111       +1     
  Lines        7312     7428     +116     
  Branches     2669     2701      +32     
==========================================
+ Hits         7109     7217     +108     
- Misses        203      211       +8     
Impacted Files Coverage Δ
index.js 100.00% <ø> (ø)
lib/rules/no-invalid-html-attribute.js 93.10% <93.10%> (ø)

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 9f1d618...ce77747. Read the comment docs.

@Nokel81
Copy link
Contributor Author

Nokel81 commented Jun 22, 2021

Maybe https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/autocomplete but I think that could easily be done in a separate PR.

@ljharb
Copy link
Member

ljharb commented Jun 22, 2021

Rebased again; I tweaked the test. <Foo rel="" /> must always be valid - this rule should only be checking HTML elements, never custom elements.

A few tests are still failing. @Nokel81?

@Nokel81
Copy link
Contributor Author

Nokel81 commented Jun 22, 2021

Well since you just changed the logic or tests (cannot tell). I was whitelisting elements whereas I think you want me to blacklist html elements that aren't on the list.

@ljharb
Copy link
Member

ljharb commented Jun 22, 2021

All I did is move the two "Foo" tests from invalid to valid.

@ljharb
Copy link
Member

ljharb commented Jun 22, 2021

I'm fine with an inclusion-list-based approach, but it should ignore custom elements entirely.

@ljharb ljharb marked this pull request as draft August 9, 2021 20:56
@Nokel81 Nokel81 marked this pull request as ready for review November 1, 2021 14:30
@Nokel81
Copy link
Contributor Author

Nokel81 commented Nov 1, 2021

@ljharb Sorry it took so long, this PR has been updated to ignore non-HTML tags.

@ljharb ljharb changed the title [New Rule]: no-invalid-html-attribute [New] add no-invalid-html-attribute rule Nov 2, 2021
@ljharb
Copy link
Member

ljharb commented Nov 2, 2021

@Nokel81 i enabled all the test cases and used the messageId pattern and rebased.

@ljharb ljharb merged commit 9ea7449 into jsx-eslint:master Nov 2, 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

4 participants