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

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

Closed
wants to merge 4 commits into from
Closed

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

wants to merge 4 commits into from

Conversation

snitin315
Copy link
Contributor

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[X] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[X] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Fixes #14076
Add support for :has() selector.

Is there anything you'd like reviewers to focus on?

No

@eslint-github-bot eslint-github-bot bot added the triage An ESLint team member will look at this issue soon label Jun 5, 2021
@snitin315 snitin315 changed the title New: support :has() selector (fixes #14076) Update: support :has() selector (fixes #14076) Jun 5, 2021
@snitin315 snitin315 marked this pull request as ready for review June 9, 2021 03:24
@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint and removed triage An ESLint team member will look at this issue soon labels Jun 24, 2021
["*: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 👍

@nzakas
Copy link
Member

nzakas commented Sep 25, 2021

Closing per discussion.

@nzakas nzakas closed this Sep 25, 2021
@snitin315 snitin315 deleted the has-selector branch September 25, 2021 00:48
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Mar 25, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Mar 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support/Document :has() selectors (per esquery update)
3 participants