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

Rule banning complex ESQuery selectors #339

Open
JoshuaKGoldberg opened this issue Feb 10, 2023 · 3 comments
Open

Rule banning complex ESQuery selectors #339

JoshuaKGoldberg opened this issue Feb 10, 2023 · 3 comments

Comments

@JoshuaKGoldberg
Copy link
Contributor

Moving discussion from typescript-eslint/typescript-eslint#6444 (comment) here, and quoting @bradzacher:

now a days though I tend to feel that leaning too heavily into the custom DSL makes code less accessible and harder to understand / debug.
[specifiers.0] is one of those parts of the DSL that's super unclear as to what it's doing, IMO.

I think leveraging super clear parts of the DSL (prop existence, parents) are great and easy to understand, but for anything more complex it's better to write the clearer logic.

Is there any consensus/desire here to limit ESQuery node types?

Proposed "bad":

return {
  ImportDeclaration[importKind!="type"][specifiers.0](node) {
}

Proposed "good":

return {
  ImportDeclaration[importKind!="type"](node) {
    if (!node.specifiers.length) {
      return;
    }
}
@bmish
Copy link
Member

bmish commented Feb 10, 2023

We'd welcome a rule for this, as some plugin authors may prefer to avoid complex selectors. However, I would lean toward any such rule to be optional and not recommended. It feels too opinionated to be in the recommended config. I wouldn't want to prevent plugin authors from taking full advantage of selectors if they prefer that style.

@DMartens
Copy link

I would like to contribute this rule but I think there needs to be discussion about what a complex selector is.
Personally I would only allow node types (e.g. FunctionDeclaration) and node classes (e.g. :function) combined with , or an optional :exit.

Valid Examples:

  • ClassDeclaration, ClassExpression
  • :function:exit
  • onCodePathStart

Invalid examples:

  • CallExpression[callee.type="MemberExpression"]
  • any selector provided by a computed property with a non-statically resolvable key (using utils.getKeyName)
  • "good" example above: but the condition could be part of the early return
  • wildstars *: never seen this selector in an actual rule
  • only an attribute [attr]: probably will lead to an unforeseen error for AST nodes unknown to the rule author

But as basically none of the selector syntax is used, it should be clear what and what is not allowed (versus different rule options, e.g. allowing maximally one attribute selector, ...). This also makes other rules for node selectors unnecessary (e.g. formatting or whether a selector can be simplified). Further this allows the callback parameters to be easily typeable (see #340)

I tried the above rules on my own rules and found no cases where I did not find it preferrable.
What do you think?

@JoshuaKGoldberg
Copy link
Contributor Author

Yeah it's a good question. typescript-eslint/typescript-eslint#4065 shows @auvred's really nifty library, magic-query. So I personally think "whatever is reasonable to parse in the type system" is a good delineation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants