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

feat: better require-closing-tags support #188

Merged
merged 29 commits into from
Jun 8, 2024
Merged
Show file tree
Hide file tree
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 May 29, 2024
1fbfe9a
don't close non-void elements
seleb May 29, 2024
2d3a515
add test case covering math self close exception
seleb May 29, 2024
da72952
clarify names
seleb May 29, 2024
f3b10f8
re-implement foreign context check
seleb May 29, 2024
26ff11b
foreign context doesn't include the opening node itself
seleb May 29, 2024
6be6f3f
rewrite custom element checks to enforce the rule when possible
seleb May 29, 2024
4e054dc
update custom tag tests
seleb May 29, 2024
28cb880
add custom tag test for children
seleb May 29, 2024
621cbbe
replace custom tag check with regex + custom pattern option
seleb May 29, 2024
604a3ee
add tests for custom pattern option
seleb May 29, 2024
21973f1
add test for preferred self closing custom tag with no children
seleb May 29, 2024
18af2d5
Update require-closing-tags.md
seleb May 29, 2024
f8102c8
add `mspace` to spellcheck
seleb May 29, 2024
c1b4fdb
prettier
seleb May 29, 2024
6b3a20f
`customPattern` string -> `customPatterns` string array
seleb Jun 2, 2024
48e51d3
format
seleb Jun 2, 2024
fc25c1c
lint
seleb Jun 2, 2024
88157f8
Update docs/rules/require-closing-tags.md
seleb Jun 4, 2024
94a5e73
update test to expect removal of closing tag when fixing
seleb Jun 4, 2024
3b10e93
remove closing tag in fixed output
seleb Jun 4, 2024
e2db05f
update fixes
seleb Jun 4, 2024
363b5f1
update tests to reflect new independent options spec
seleb Jun 6, 2024
71c72da
replace `allowSelfClosingCustom` + `customPatterns` with independent …
seleb Jun 6, 2024
0ca6eeb
Update require-closing-tags.md
seleb Jun 6, 2024
951289c
format
seleb Jun 6, 2024
6df37eb
update default to disallow self-closing custom tags
seleb Jun 7, 2024
3ada9c8
update docs
seleb Jun 7, 2024
8665ff4
format
seleb Jun 7, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .cspell.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
"noembed",
"roletype",
"nextid",
"screenreader"
"screenreader",
"mspace"
]
}
31 changes: 18 additions & 13 deletions docs/rules/require-closing-tags.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,15 @@ Examples of **correct** code for this rule:

### Options

This rule has an object option for [Void Elements](https://html.spec.whatwg.org/multipage/syntax.html#void-elements).
This rule has an object option for [Void Elements](https://html.spec.whatwg.org/multipage/syntax.html#void-elements) and custom element patterns.

- `"selfClosing": "never"`: (default) disallow using self closing tag on [Void Elements](https://html.spec.whatwg.org/multipage/syntax.html#void-elements).

- `"selfClosing": "always"`: enforce using self closing tag on [Void Elements](https://html.spec.whatwg.org/multipage/syntax.html#void-elements).

- `"allowSelfClosingCustom": false`: (default) disallow self-closing for the custom tags.
- `"selfClosingCustomPatterns": []`: (default) disallow self-closing for custom tags.

- `"allowSelfClosingCustom": true`: allow self-closing for the custom tags.
- `"selfClosingCustomPatterns": ["-"]`: enforce self-closing for tags matching any of an array of strings representing regular expression pattern (e.g. tags including `-` in the name).

#### selfClosing : "never"

Expand Down Expand Up @@ -76,32 +76,37 @@ Examples of **correct** code for the `{ "selfClosing": "always" }` option:
<base />
```

#### "allowSelfClosingCustom": false
#### selfClosingCustomPatterns: ["-"]

Examples of **incorrect** code for the `{ "allowSelfClosingCustom": false }` option:
Examples of **incorrect** code for the `{ "selfClosingCustomPatterns": ["-"] }` option:

<!-- prettier-ignore -->
```html,incorrect
<custom-tag />
<custom-tag> </custom-tag>
```

Examples of **correct** code for the `{ "allowSelfClosingCustom": false }` option:
Examples of **correct** code for the `{ "selfClosingCustomPatterns": ["-"] }` option:

<!-- prettier-ignore -->
```html,correct
<custom-tag> </custom-tag>
<custom-tag>children</custom-tag>
<custom-tag />
```

#### "allowSelfClosingCustom": true
#### selfClosingCustomPatterns: []

Examples of **correct** code for the `{ "allowSelfClosingCustom": true }` option:
Examples of **incorrect** code for the `{ "allowSelfClosingCustom": [] }` option:

<!-- prettier-ignore -->
```html,correct
<!-- both allowed -->
```html,incorrect
<custom-tag />
```

Examples of **correct** code for the `{ "allowSelfClosingCustom": [] }` option:

<!-- prettier-ignore -->
```html,correct
<custom-tag> </custom-tag>
<custom-tag />
```

## Further Reading
Expand Down
61 changes: 45 additions & 16 deletions packages/eslint-plugin/lib/rules/require-closing-tags.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,11 @@ module.exports = {
selfClosing: {
enum: ["always", "never"],
},
allowSelfClosingCustom: {
type: "boolean",
selfClosingCustomPatterns: {
type: "array",
items: {
type: "string",
},
},
Copy link
Owner

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.

Copy link
Contributor Author

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

@yeonjuan yeonjuan Jun 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

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

},
additionalProperties: false,
Expand All @@ -49,14 +52,21 @@ module.exports = {
},

create(context) {
const shouldSelfClose =
/** @type {string[]} */
const foreignContext = [];
const shouldSelfCloseVoid =
context.options && context.options.length
? context.options[0].selfClosing === "always"
: false;
const allowSelfClosingCustom =
context.options && context.options.length
? context.options[0].allowSelfClosingCustom === true
: false;
/** @type {string[]} */
const selfClosingCustomPatternsOption =
(context.options &&
context.options.length &&
context.options[0].selfClosingCustomPatterns) ||
[];
const selfClosingCustomPatterns = selfClosingCustomPatternsOption.map(
(i) => new RegExp(i)
);

/**
* @param {TagNode} node
Expand Down Expand Up @@ -91,7 +101,10 @@ module.exports = {
if (!fixable) {
return null;
}
return fixer.replaceText(node.openEnd, " />");
const fixes = [];
fixes.push(fixer.replaceText(node.openEnd, " />"));
if (node.close) fixes.push(fixer.remove(node.close));
return fixes;
},
});
}
Expand All @@ -115,17 +128,33 @@ module.exports = {
return {
Tag(node) {
const isVoidElement = VOID_ELEMENTS_SET.has(node.name);
if (
node.selfClosing &&
allowSelfClosingCustom &&
node.name.indexOf("-") !== -1
) {
checkVoidElement(node, true, false);
} else if (node.selfClosing || isVoidElement) {
checkVoidElement(node, shouldSelfClose, isVoidElement);
const isSelfClosingCustomElement = !!selfClosingCustomPatterns.some(
(i) => node.name.match(i)
);
const isForeign = foreignContext.length > 0;
const shouldSelfCloseCustom =
isSelfClosingCustomElement && !node.children.length;
const shouldSelfCloseForeign = node.selfClosing;
const shouldSelfClose =
(isVoidElement && shouldSelfCloseVoid) ||
(isSelfClosingCustomElement && shouldSelfCloseCustom) ||
(isForeign && shouldSelfCloseForeign);
const canSelfClose =
isVoidElement || isSelfClosingCustomElement || isForeign;
if (node.selfClosing || canSelfClose) {
checkVoidElement(node, shouldSelfClose, canSelfClose);
} else if (node.openEnd.value !== "/>") {
checkClosingTag(node);
}
if (["svg", "math"].includes(node.name)) foreignContext.push(node.name);
},
/**
* @param {TagNode} node
*/
"Tag:exit"(node) {
if (node.name === foreignContext[foreignContext.length - 1]) {
foreignContext.pop();
}
},
};
},
Expand Down
63 changes: 56 additions & 7 deletions packages/eslint-plugin/tests/rules/require-closing-tags.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,39 +23,47 @@ ruleTester.run("require-closing-tags", rule, {
code: `<custom-tag> </custom-tag>`,
options: [
{
allowSelfClosingCustom: false,
selfClosingCustomPatterns: [],
},
],
},
{
code: `<custom-tag/>`,
options: [
{
allowSelfClosingCustom: true,
selfClosingCustomPatterns: ["-"],
},
],
},
{
code: `<custom-tag />`,
options: [
{
allowSelfClosingCustom: true,
selfClosingCustomPatterns: ["-"],
},
],
},
{
code: `<custom-tag> </custom-tag>`,
options: [
{
allowSelfClosingCustom: true,
selfClosingCustomPatterns: ["-"],
},
],
},
{
code: `<custom-tag id="foo" />`,
options: [
{
allowSelfClosingCustom: true,
selfClosingCustomPatterns: ["-"],
},
],
},
{
code: `<custom-tag>children</custom-tag>`,
options: [
{
selfClosingCustomPatterns: ["-"],
},
],
},
Expand All @@ -75,6 +83,20 @@ ruleTester.run("require-closing-tags", rule, {
<circle />
</svg>
</body>
`,
options: [
{
selfClosing: "always",
},
],
},
{
code: `
<body>
<math>
1<mspace width="100px" />2
</math>
</body>
`,
options: [
{
Expand Down Expand Up @@ -142,7 +164,7 @@ ruleTester.run("require-closing-tags", rule, {
code: `<custom-tag />`,
options: [
{
allowSelfClosingCustom: false,
selfClosingCustomPatterns: [],
},
],
errors: [
Expand All @@ -155,7 +177,34 @@ ruleTester.run("require-closing-tags", rule, {
code: `<custom-tag id="foo" />`,
options: [
{
allowSelfClosingCustom: false,
selfClosingCustomPatterns: [],
},
],
errors: [
{
messageId: "unexpected",
},
],
},
{
code: `<custom-tag></custom-tag>`,
options: [
{
selfClosingCustomPatterns: ["-"],
},
],
output: "<custom-tag />",
errors: [
{
messageId: "missingSelf",
},
],
},
{
code: `<div />`,
options: [
{
selfClosing: "always",
},
],
output: null,
Expand Down
Loading