-
Notifications
You must be signed in to change notification settings - Fork 31
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: better require-closing-tags
support
#188
Merged
Merged
Changes from all commits
Commits
Show all changes
29 commits
Select commit
Hold shift + click to select a range
04d8d49
add test case covering non-void self-closing tag when self-closing is…
seleb 1fbfe9a
don't close non-void elements
seleb 2d3a515
add test case covering math self close exception
seleb da72952
clarify names
seleb f3b10f8
re-implement foreign context check
seleb 26ff11b
foreign context doesn't include the opening node itself
seleb 6be6f3f
rewrite custom element checks to enforce the rule when possible
seleb 4e054dc
update custom tag tests
seleb 28cb880
add custom tag test for children
seleb 621cbbe
replace custom tag check with regex + custom pattern option
seleb 604a3ee
add tests for custom pattern option
seleb 21973f1
add test for preferred self closing custom tag with no children
seleb 18af2d5
Update require-closing-tags.md
seleb f8102c8
add `mspace` to spellcheck
seleb c1b4fdb
prettier
seleb 6b3a20f
`customPattern` string -> `customPatterns` string array
seleb 48e51d3
format
seleb fc25c1c
lint
seleb 88157f8
Update docs/rules/require-closing-tags.md
seleb 94a5e73
update test to expect removal of closing tag when fixing
seleb 3b10e93
remove closing tag in fixed output
seleb e2db05f
update fixes
seleb 363b5f1
update tests to reflect new independent options spec
seleb 71c72da
replace `allowSelfClosingCustom` + `customPatterns` with independent …
seleb 0ca6eeb
Update require-closing-tags.md
seleb 951289c
format
seleb 6df37eb
update default to disallow self-closing custom tags
seleb 3ada9c8
update docs
seleb 8665ff4
format
seleb File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,6 +31,7 @@ | |
"noembed", | ||
"roletype", | ||
"nextid", | ||
"screenreader" | ||
"screenreader", | ||
"mspace" | ||
] | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Just a suggestion. Feel free to ignore it if you think it's better now. :)
Instead of adding a new option, what do you think about making the existing allowSelfClosingCustom option accept a regex array as well?
allowSelfClosingCustom: true
: allows self closing for all custom components.allowSelfClosingCustom: false
: disallows self closing for all custom components.allowSelfClosingCustom: [":"]
: allows only custom components that match the specified regex.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.
i don't think "all custom components" has a valid definition here: without the user defining what the custom components are, the rule can only make assumptions about what they look like
it would be possible to collapse
allowSelfClosingCustom
andcustomPatterns
into a single option that just accepts the regex array of custom patterns though, and then treat the options like:selfClosing
: whether void/custom elements should be self closingcustomPatterns
: which elements are included inselfClosing
checksthis is probably more intuitive, and would address the ambiguous interpretation of the rule pointed out in your other comment here https://github.com/yeonjuan/html-eslint/pull/188/files/fc25c1c51e9cd1a74950661d8f0f70d829ecbcfb#r1626087825
would collapsing the options be preferred, or is there a good reason to keep the configuration for void tags and custom tags separate?
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.
Yes, because the two options are semantically different. void elements are elements that can't have children, while custom tags can have children depending on the implementation.
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.
I also believe that the options should not depends on each other as little as possible.
If an options depends on the other, users will be able to set useless options, such as
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.
this is captured by the current implementation: if self-closing is enforced, but a custom tag has children, it won't be considered an error (test case)
i think i'm not quite following the intent here, so to clarify: my assumption was that custom elements should match the prescribed behaviour for void elements (i.e. they should either enforce self-closing, or prevent self-closing), and an exception was made for when they have children (since it's unknown whether children are valid, and this may be independent of whether the tag can self-close).
is the actual intent as follows?
i can update the implementation to reflect that if so, but i don't think the current option names are clear if that's the case
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.
Yes. 👍
Why? because using self-close in a custom tag is not standard HTML.
This option was added to be useful in exceptional cases, such as with the template engine.
That's why I think it should work independently of the
selfClosing
option, which covers “void-element” (standard HTML).@seleb What about the
selfClosingCustomPatterns
option? I was thinking of forcing self-closing if the regular expression in selfClosingCustomPatterns is matched (if there are no children), and disabling self-closing otherwise.This is similar to “customPatterns” in the current PR. However, it works independently of “selfClosing”.
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.
using
selfClosingCustomPatterns
that way makes sense to me, i'll try to update this later today to meet that spec