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

Support/Document :has() selectors (per esquery update) #14076

Closed
brettz9 opened this issue Feb 7, 2021 · 15 comments
Closed

Support/Document :has() selectors (per esquery update) #14076

brettz9 opened this issue Feb 7, 2021 · 15 comments
Assignees
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

Comments

@brettz9
Copy link
Contributor

brettz9 commented Feb 7, 2021

The version of ESLint you are using.

7.19.0

The problem you want to solve.

esquery has just published a new release with the support for custom visitor keys (that may contribute to fixing #13639 ), which brings parser-independent support including for has:() selectors.

However, https://github.com/eslint/eslint/blob/master/docs/developer-guide/selectors.md does not indicate support for :has(), and per #13639 (comment) there may be some work to do to be able to support them:

ESLint selectors are using esquery, but we're also doing some optimizations and calculating specificity in NodeEventGenerator, so it's possible that some additional work has to be done before we can say that ESLint fully supports new selectors.

(Note that in addition to :has() the "subject" indicator is also supported, but per estools/esquery#61 , that implementation is buggy while the :has() one was working enough to be used as a base for the former–though with one issue on relative keys mentioned there.)

Your take on the correct solution to problem.

No special take. Ideally would provide support for the selectors and update the docs.

Are you willing to submit a pull request to implement this change?

No.

@brettz9 brettz9 added core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint triage An ESLint team member will look at this issue soon labels Feb 7, 2021
@mdjermanovic mdjermanovic removed the triage An ESLint team member will look at this issue soon label Feb 13, 2021
@mdjermanovic mdjermanovic added this to Evaluating in Triage Feb 13, 2021
@mdjermanovic
Copy link
Member

I think this could be very useful for configuring rules such as no-restricted-syntax, but we should probably add a note that it traverses subtrees, so a rule written in a way that uses ESLint's traversal instead of :has() may have a much better performance.

@mdjermanovic
Copy link
Member

I'll champion this.

@mdjermanovic mdjermanovic self-assigned this Mar 3, 2021
@nzakas
Copy link
Member

nzakas commented Apr 15, 2021

@mdjermanovic whats the next step here? Do we need an RFC?

@mdjermanovic
Copy link
Member

@mdjermanovic whats the next step here?

The proposal is waiting for more opinions from the team.

Do we need an RFC?

Hm, it looked like a small change so I wasn't thinking about RFC. It's indeed a change in the core, so by our policy it would need an RFC. What do you think?

@nzakas
Copy link
Member

nzakas commented Apr 16, 2021

Okay, be sure to tag in other folks when you’re looking for more opinions. That way we know what is stopping progress.

I’m 👍. I was just concerned about the note about NodeEventGenerator and didn’t know if we needed some investigation as to the changes necessary. If it’s really just a plug and play change with minimal other code necessary, I think we can do it without an RFC.

@nzakas nzakas moved this from Evaluating to Feedback Needed in Triage May 25, 2021
@nzakas
Copy link
Member

nzakas commented May 29, 2021

@btmills can you chime in on this?

@btmills btmills added the accepted There is consensus among the team that this change meets the criteria for inclusion label May 29, 2021
@btmills btmills moved this from Feedback Needed to Ready to Implement in Triage May 29, 2021
@btmills
Copy link
Member

btmills commented May 29, 2021

👍, marking as accepted and moving to ready to implement. I don’t see a need for an RFC.

@snitin315
Copy link
Contributor

Can I work on this one?

@mdjermanovic
Copy link
Member

@snitin315 sure, thanks!

@mdjermanovic
Copy link
Member

while the :has() one was working enough to be used as a base for the former–though with one issue on relative keys mentioned there

@brettz9 the issue on relative keys is that syntax such as :has( > Identifier) isn't supported yet?

@brettz9
Copy link
Contributor Author

brettz9 commented Jun 25, 2021

@mdjermanovic Right. A PR in estools/esquery#61 attempted to offer it, but the review comments were never completed.

@mdjermanovic
Copy link
Member

Is it expected behavior that selectors in the :has() list can match the element itself, or only its descendants?

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

1 // error

demo

@brettz9
Copy link
Contributor Author

brettz9 commented Jun 25, 2021

Hmmm. The tests don't refer to self-referential elements, and the only other reference I see is in the README comparing it to the CSS pseudo-selector: https://drafts.csswg.org/selectors-4/#has-pseudo . So I'm not sure.

In the latest esquery on master/1.4.0, it doesn't error but returns the same Literal as itself.

I wouldn't expect this to work myself, but that's just my own sense.

@mdjermanovic
Copy link
Member

In the latest esquery on master/1.4.0, it doesn't error but returns the same Literal as itself.

Sorry, by "error" I meant a linting problem, i.e., the selector in the no-restricted-syntax config matches literal 1.

I filed an issue in esquery repo: estools/esquery#122

@snitin315 snitin315 moved this from Ready to Implement to Pull Request Opened in Triage Aug 7, 2021
@nzakas
Copy link
Member

nzakas commented Sep 25, 2021

Based on implementation issues, we decided not to move forward with this feature. The selector has performance issues and we would prefer it not be used in rules.

@nzakas nzakas closed this as completed Sep 25, 2021
Triage automation moved this from Pull Request Opened to Complete Sep 25, 2021
@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
Archived in project
Triage
Complete
Development

Successfully merging a pull request may close this issue.

5 participants