feat: better require-closing-tags support

[seleb]

Hey there! I ran into an issue using require-closing-tags with the selfClosing: "always" option where non-void elements were not being checked correctly. For example:

"@html-eslint/require-closing-tags": ["error", { "selfClosing": "always" }],

<div />

In this case <div /> is incorrect (div is not a void element and cannot self-close), but the linter would not catch it.

The scope of this fix ended up being larger than expected: custom tags which did not use - as part of their naming convention and tags in a foreign context were not explicitly supported, and instead seemed to be handled via this bug. To compensate, I've re-implemented a version of the stack-based approach for handling foreign context seen in earlier implementations of the rule, and added a customPattern regex option to allow users to expand the list of allowed elements to include their own self-closing tags (this addresses #183 as a side effect).

I've configured customPattern's default to "-" since that matches the previous implementation of the custom element check, but because custom tags are now being enforced instead of ignored and also include a check for child nodes (unlike void elements, it can't be assumed that self-closing tags cannot have children since the implementation is up to the user), this is still a breaking change.

[yeonjuan]

@seleb
Hi. Thanks for your contribution. Before I give a detailed review, I have an opinion about the customPattern option.
How about making it possible to set multiple regexes?

customPatterns: ["..", ".."]

We already use array regexes as options in no-restricted-attrs, no-restricted-attr-values, etc. and I think this saves us the trouble of writing complex one regex.

• https://html-eslint.org/docs/rules/no-restricted-attrs
• https://html-eslint.org/docs/rules/no-restricted-attr-values

[seleb]

sure, i'll update it to use an array instead

[yeonjuan]

@seleb Thanks for the contribution, I have a few comments. :)

[yeonjuan]

The output seems to be incorrect. It should be <custom-tag />.

[seleb]

the rule fixes the opening tag to be self-closing, but it doesn't change the other end; i believe this is a known issue described in #171 (it looks a little strange here, but since the component could contain children it probably makes sense to leave the rest of the code unchanged and let the user clean that up themselves?)

[yeonjuan]

This is somewhat different from issue #171. The autofixed output should be correct HTML. If it changes to self closing, we need to remove the closing tag. I think we could use fixer.remove(nodeOrToken).

[yeonjuan]

This looks like another breaking change.
I think the allowSelfClosingCustom option should work independently of selfClosing.

<! -- selfClosing: "never" allowSelfClosingCustom: true -->
<custom-tag /> <!-- ok -->
<img /> <!-- error -->

[yeonjuan]

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.

[seleb]

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 and customPatterns 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 closing
• customPatterns: which elements are included in selfClosing checks

this 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?

[yeonjuan]

is there a good reason to keep the configuration for void tags and custom tags separate?

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.

[yeonjuan]

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

selfClosing: never
customPatterns: ["-"] // uselss

[seleb]

void elements are elements that can't have children, while custom tags can have children depending on the implementation

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?

• prescribe whether void elements should self-close or not
• prescribe a set of custom elements which should self-close
• prescribe a set of custom elements which should never self-close

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

[yeonjuan]

prescribe whether void elements should self-close or not
prescribe a set of custom elements which should self-close
prescribe a set of custom elements which should never self-close

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).

• Option to allow any element as self-closing #99

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

selfClosing: never | always
selfClosingCustomPatterns: ["-"]

This is similar to “customPatterns” in the current PR. However, it works independently of “selfClosing”.

[seleb]

using selfClosingCustomPatterns that way makes sense to me, i'll try to update this later today to meet that spec

[yeonjuan]

LGTM!
I think the default value for selfClosingCustomPatterns should be undefined or an empty array, because as I mentioned before, self-closing of custom elements is non-standard.
What do you think?
If you agree, I'd be happy to modify it myself

[seleb]

LGTM! I think the default value for selfClosingCustomPatterns should be undefined or an empty array, because as I mentioned before, self-closing of custom elements is non-standard. What do you think? If you agree, I'd be happy to modify it myself

sounds good, updated!

[yeonjuan]

LGTM! Thanks for your work. 🎉