-
-
Notifications
You must be signed in to change notification settings - Fork 449
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
feat(lint): add checkHtmlElements
option for useSelfClosingElements
#3697
base: main
Are you sure you want to change the base?
feat(lint): add checkHtmlElements
option for useSelfClosingElements
#3697
Conversation
html
option for useSelfClosingElements
html
option for useSelfClosingElements
html
option for useSelfClosingElements
html
option for useSelfClosingElements
html
option for useSelfClosingElements
CodSpeed Performance ReportMerging #3697 will degrade performances by 6.24%Comparing Summary
Benchmarks breakdown
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution.
A couple of important things should be addressed:
- Documentation; I left various tips
- Changelog line for
CHANGELOG.md
/// { | ||
/// "//":"...", | ||
/// "options": { | ||
/// "html": true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, the name html
is not descriptive enough. A name should be explanatory enough so users can understand what it does just by reading it.
I think we should name it checkHtmlElements
, what do you think?
Regarding the structure of the docs, it should be like this:
### Options
#### `nameOfTheOoption`
Default: `true`
<Explanation of the option> (e.g. This option allows to control whether this rule should check HTML elements or not)
<Explanation of an example> (e.g. In the following example, when the option is set to `false`, this happens)
```json
{
"//": "example of the configuration"
}
```
```jsx,ignore
// example of code that will pass, use the `ignore` directive in the code block, so the codegen won't execute the code
<img src="something.png" >
```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback. I think we can proceed with checkHtmlElements
!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in c633de8
Please let me know if it's acceptable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A possible alternative name: ignoreHtmlElements
.
This has the advantage to make its default false
.
#[derive(Clone, Debug, Deserialize, Deserializable, Eq, PartialEq, Serialize)] | ||
#[cfg_attr(feature = "schemars", derive(JsonSchema))] | ||
pub struct UseSelfClosingElementsOptions { | ||
pub html: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The options must be documented. The documentation will be part of the configuration JSON schema, this means that users will be able to see the description when their IDE will autocomplete the option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. I'll add the documentation too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in c633de8
Please let me know if it's acceptable.
@abidjappie, are you still interested in bringing this feature to the finish line? |
@ematipico sorry, I'll have a look at this one this week! |
6961fdc
to
c633de8
Compare
I still need to investigate how to do the |
Updated 👍 |
95c434e
to
3604605
Compare
…ts - feature parity for html option in jsx-self-closing-comp
3604605
to
9e5086a
Compare
html
option for useSelfClosingElements
checkHtmlElements
option for useSelfClosingElements
/// | ||
/// ## Options | ||
/// | ||
/// #### `checkHtmlElements` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nits
/// #### `checkHtmlElements` | |
/// ### `checkHtmlElements` |
/// | ||
/// This option allows you to specify whether or not to check native HTML elements. | ||
/// | ||
/// In the following example, when the option is set to "false", it will not self close native HTML elements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A suggestion because the following example is set to true.
/// In the following example, when the option is set to "false", it will not self close native HTML elements. | |
/// In the following example, when the option is set to "true", it will check for self-closing native HTML elements. |
Summary
Fix #3364
This PR implements the
checkHtmlElements
option provided by Eslint Stylisticjsx-self-closing-comp
(ashtml
).By setting
checkHtmlElements
to false, it will not enforce this rule onhtml
elements.Implementation
It checks whether the
JsxOpeningElement
contains aJsxName
i.e. it is not aJsxReferenceIdentifier
etc. and assumes that it is a valid html element implicitly. If this assumption does not hold, we may need an alternative implementation.Limitations
This does not cover the void element case - i.e. it will not automatically lint the void element correctly. It simply disables the rule.
Test Plan
Added the
validCheckHtmlElementsFalse
andinvalidCheckHtmlElementsFalse
test cases.