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.

Sign up for GitHub

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

Jump to bottom

Update: support :has() selector (fixes #14076) #14674

Closed
wants to merge 4 commits into from
Closed

Update: support :has() selector (fixes #14076) #14674

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5c1da0a
New: support `:has()` selector
snitin315 Jun 5, 2021
12296b5
Docs: update
snitin315 Jun 5, 2021
6e45a2a
Chore: more test
snitin315 Jun 8, 2021
793b961
Docs: update
snitin315 Jun 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions docs/developer-guide/selectors.md
View file
Open in desktop
Expand Up @@ -43,6 +43,7 @@ The following selectors are supported:
* following sibling: `VariableDeclaration ~ VariableDeclaration`
* adjacent sibling: `ArrayExpression > Literal + SpreadElement`
* negation: `:not(ForStatement)`
* has: `:has(ForStatement)`
* matches-any: `:matches([attr] > :first-child, :last-child)`
* class of AST node: `:statement`, `:expression`, `:declaration`, `:function`, or `:pattern`

Expand Down Expand Up @@ -88,6 +89,8 @@ If two or more selectors match the same node, their listeners will be called in

If multiple selectors have equal specificity, their listeners will be called in alphabetical order for that node.

Using `:has()` traverses subtrees, so a rule written in a way that uses ESLint's traversal instead of `:has()` may have a much better performance.

### Restricting syntax with selectors

With the [no-restricted-syntax](/docs/rules/no-restricted-syntax.md) rule, you can restrict the usage of particular syntax in your code. For example, you can use the following configuration to disallow using `if` statements that do not have block statements as their body:
Expand Down
2 changes: 2 additions & 0 deletions lib/linter/node-event-generator.js
View file
Open in desktop
Expand Up @@ -121,6 +121,7 @@ function countClassAttributes(parsedSelector) {

case "compound":
case "not":
case "has":
case "matches":
return parsedSelector.selectors.reduce((sum, childSelector) => sum + countClassAttributes(childSelector), 0);

Expand Down Expand Up @@ -150,6 +151,7 @@ function countIdentifiers(parsedSelector) {

case "compound":
case "not":
case "has":
case "matches":
return parsedSelector.selectors.reduce((sum, childSelector) => sum + countIdentifiers(childSelector), 0);

Expand Down
18 changes: 18 additions & 0 deletions tests/lib/linter/node-event-generator.js
View file
Open in desktop
Expand Up @@ -187,6 +187,24 @@ describe("NodeEventGenerator", () => {
]
);

assertEmissions(
"foo",
["*:has(ExpressionStatement)"],
ast => [
["*:has(ExpressionStatement)", ast], // Program
["*:has(ExpressionStatement)", ast.body[0]] // ExpressionStatement
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this is the intended behavior. Let's wait to see if this could be fixed in esquery if it turns out that it is a bug.

estools/esquery#122

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's wait 👍

Copy link
Member

Choose a reason for hiding this comment

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

Given that there’s no response on the issue, how can we proceed here? Can we merge?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/cc @mdjermanovic what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how to proceed, there's still no response on estools/esquery#122. If this isn't intended behavior, maybe we shouldn't be promoting this feature yet.

This esquery feature is intended to match CSS :has(), but that is an optional CSS feature and it seems that none of the browsers support it due to performance concerns, so there's nowhere to check how that really works.

My interpretation of the spec text is that in the following example ArrayExpression:has(ArrayExpression) shouldn't match the inner ArrayExpression because ArrayExpression ArrayExpression doesn't match any of its child elements.

/* eslint no-restricted-syntax: ["error",
    "ArrayExpression ArrayExpression", 
    "ArrayExpression:has(ArrayExpression)"   
]*/

[
  []
]

But it does, as in this online demo. Maybe it is the correct behavior, but I'm not sure where could we verify it.

Copy link
Member

Choose a reason for hiding this comment

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

If browsers don’t support it, I think that’s a good argument that ESLint doesn’t need to support it.

Copy link
Member

Choose a reason for hiding this comment

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

It should generally be avoided in rules, for performance reasons, as there may be a better way to write the rule.

It can be useful for user-defined selectors, like those in no-restricted-syntax, where all the user can do is to write a selector and there may be no alternative with the same functionality.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that's a strong enough use case to be worth the effort for this feature. no-restricted-syntax is a convenience, and for anything more complicated, I'm fine with telling people to write a custom rule.

I'd say let's close this.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with closing. I already had concerns that this selector might be overused in rules, and now when it looks like it doesn't work as expected, it seems better that we don't include it.

It already can be used in ESLint (only the specificity calculations are wrong), but we don't need to officially promote it.

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 agree with closing as well 👍

]
);

assertEmissions(
"foo++",
["*:not(*:has(ExpressionStatement))"],
ast => [
["*:not(*:has(ExpressionStatement))", ast.body[0].expression], // UpdateExpression
["*:not(*:has(ExpressionStatement))", ast.body[0].expression.argument] // Identifier
]
);

assertEmissions(
"foo()",
["CallExpression[callee.name='foo']"],
Expand Down
10 changes: 10 additions & 0 deletions tests/lib/rules/no-restricted-syntax.js
View file
Open in desktop
Expand Up @@ -29,6 +29,7 @@ ruleTester.run("no-restricted-syntax", rule, {
{ code: "({ foo: 1, bar: 2 })", options: ["Property > Literal.key"] },
{ code: "A: for (;;) break;", options: ["BreakStatement[label]"] },
{ code: "function foo(bar, baz) {}", options: ["FunctionDeclaration[params.length>2]"] },
{ code: "var foo = 42;", options: [":has(AssignmentExpression)"] },

// object format
{ code: "var foo = 42;", options: [{ selector: "ConditionalExpression" }] },
Expand Down Expand Up @@ -115,6 +116,15 @@ ruleTester.run("no-restricted-syntax", rule, {
options: [{ selector: "FunctionDeclaration[params.length>2]", message: "custom error message." }],
errors: [{ messageId: "restrictedSyntax", data: { message: "custom error message." }, type: "FunctionDeclaration" }]
},
{
code: "foo += 100;",
options: [{ selector: ":has(AssignmentExpression)" }],
errors: [
{ messageId: "restrictedSyntax", data: { message: "Using ':has(AssignmentExpression)' is not allowed." }, type: "Program" },
{ messageId: "restrictedSyntax", data: { message: "Using ':has(AssignmentExpression)' is not allowed." }, type: "ExpressionStatement" },
{ messageId: "restrictedSyntax", data: { message: "Using ':has(AssignmentExpression)' is not allowed." }, type: "AssignmentExpression" }
]
},

// with object format, the custom message may contain the string '{{selector}}'
{
Expand Down