Skip to content

feat: support TypeScript syntax in no-invalid-this rule #19532

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

Merged
merged 20 commits into from
Apr 9, 2025

Conversation

Tanujkanti4441
Copy link
Contributor

Prerequisites checklist

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

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

What changes did you make? (Give an overview)

Added support of TypeScript syntax in no-invalid-this rule. such as

rule will not report this when it's type is checked in functions.
this will be allowed in Accessor Property of class field initializers.

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

Refs #19173.

@Tanujkanti4441 Tanujkanti4441 requested a review from a team as a code owner March 18, 2025 13:29
@github-project-automation github-project-automation bot moved this to Needs Triage in Triage Mar 18, 2025
@eslint-github-bot eslint-github-bot bot added the feature This change adds a new feature to ESLint label Mar 18, 2025
@github-actions github-actions bot added the rule Relates to ESLint's core rules label Mar 18, 2025
Copy link

netlify bot commented Mar 18, 2025

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit 9835e18
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/67f232b87ae5570008da6c67
😎 Deploy Preview https://deploy-preview-19532--docs-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@nzakas nzakas moved this from Needs Triage to Implementing in Triage Mar 18, 2025
@nzakas
Copy link
Member

nzakas commented Mar 18, 2025

Please take a look at the test failures.

@Tanujkanti4441 Tanujkanti4441 marked this pull request as draft March 18, 2025 14:48
@Tanujkanti4441 Tanujkanti4441 marked this pull request as ready for review March 20, 2025 08:30
@snitin315 snitin315 added the accepted There is consensus among the team that this change meets the criteria for inclusion label Mar 21, 2025
snitin315
snitin315 previously approved these changes Mar 22, 2025
Copy link
Contributor

@snitin315 snitin315 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Leaving this open for a second review.

@snitin315 snitin315 moved this from Implementing to Second Review Needed in Triage Mar 22, 2025
## When Not To Use It

If you don't want to be notified about usage of `this` keyword outside of classes or class-like objects, you can safely disable this rule.

The TypeScript compiler already checks for the issues addressed by this ESLint rule. Therefore, enabling this rule in new TypeScript projects is generally unnecessary. You should only enable it if you prefer ESLint's error messages over the TypeScript compiler's or if you're using it to lint JavaScript code.
Copy link
Member

Choose a reason for hiding this comment

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

We already have meta fields for this purpose. For example:

handled_by_typescript: true
extra_typescript_info: Note that the compiler will not catch the `Object.assign()` case. Thus, if you use `Object.assign()` in your codebase, this rule will still provide some value.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, this rule's docs already have these fields specified, so this paragraph seems like an unnecessary duplicate.

https://eslint.org/docs/latest/rules/no-invalid-this#handled_by_typescript

Comment on lines 145 to 155
AccessorProperty(node) {
stack.push({
init: true,
node,
valid: true,
});
},

"AccessorProperty:exit"() {
stack.pop();
},
Copy link
Member

Choose a reason for hiding this comment

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

I think this would be a false negative:

function foo() {
  class C {
    accessor [this.a] = foo;
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Leaving open for @snitin315 to re-review.

Copy link
Contributor

@snitin315 snitin315 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@snitin315 snitin315 merged commit db650a0 into eslint:main Apr 9, 2025
32 checks passed
@github-project-automation github-project-automation bot moved this from Second Review Needed to Complete in Triage Apr 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

4 participants