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

Repo: enable eslint-plugin/no-property-in-node #8226

Open
JoshuaKGoldberg opened this issue Jan 8, 2024 · 3 comments
Open

Repo: enable eslint-plugin/no-property-in-node #8226

JoshuaKGoldberg opened this issue Jan 8, 2024 · 3 comments
Labels
accepting prs Go ahead, send a pull request that resolves this issue repo maintenance things to do with maintenance of the repo, and not with code/docs

Comments

@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commented Jan 8, 2024

Suggestion

Sometimes folks will use an 'in' property check in a rule to narrow the type of a node:

declare const node: ts.BinaryExpression | ts.ConditionalExpression;

if ('condition' in node) {
  node
  // ^? const node: ts.ConditionalExpression
}

I've been advising to prefer using less "dynamic" logical gates: existing type predicate functions, direct .kind/.type checks, and/or having different functions for different node selectors.

declare const node: ts.BinaryExpression | ts.ConditionalExpression;

// or:
// if (node.kind === ts.SyntaxKind.ConditionalExpression) {
if (ts.isConditionalExpression(condition)) {
  node
  // ^? const node: ts.ConditionalExpression
}

My reasoning has been that "dynamic" logical gates, beyond being a potential deoptimization in JS perf, are susceptible to the types changing unexpectedly. E.g. a new version of TypeScript might add a new node type to a built-in union that adds a case previously meant to be caught by the rule.

My understanding of in checks is that they're generally reserved for cases where the more "static" ways of checking aren't available.

Shall we encode this as a lint rule?

Similarly positioned to #4796: if eslint-community/eslint-plugin-eslint-plugin#416 is accepted, we can instead enable it in eslint-plugin-eslint-plugin.

Edit: eslint-community/eslint-plugin-eslint-plugin#326, actually 😄

@JoshuaKGoldberg JoshuaKGoldberg added triage Waiting for maintainers to take a look repo maintenance things to do with maintenance of the repo, and not with code/docs labels Jan 8, 2024
@JoshuaKGoldberg
Copy link
Member Author

Marking as blocked on eslint-community/eslint-plugin-eslint-plugin#326, in that once that exists we'll take it in.

@JoshuaKGoldberg JoshuaKGoldberg added blocked by external API Blocked by a tool we depend on exposing an API, such as TypeScript's Type Relationship API blocked by another issue Issues which are not ready because another issue needs to be resolved first and removed triage Waiting for maintainers to take a look blocked by external API Blocked by a tool we depend on exposing an API, such as TypeScript's Type Relationship API labels Feb 4, 2024
@Josh-Cena
Copy link
Member

@JoshuaKGoldberg since eslint-community/eslint-plugin-eslint-plugin#433 exists, we just need to enable it right?

@JoshuaKGoldberg
Copy link
Member Author

Right, yes!

@JoshuaKGoldberg JoshuaKGoldberg added accepting prs Go ahead, send a pull request that resolves this issue and removed blocked by another issue Issues which are not ready because another issue needs to be resolved first labels Jun 1, 2024
@Josh-Cena Josh-Cena changed the title Repo: New internal lint rule: don't 'in' check AST nodes Repo: enable eslint-plugin/no-property-in-node Jun 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepting prs Go ahead, send a pull request that resolves this issue repo maintenance things to do with maintenance of the repo, and not with code/docs
Projects
None yet
Development

No branches or pull requests

2 participants