Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

📎 Implement noThisAlias, @typescript-eslint/no-this-alias #4481

Closed
Conaclos opened this issue May 15, 2023 · 7 comments · Fixed by biomejs/biome#23
Closed

📎 Implement noThisAlias, @typescript-eslint/no-this-alias #4481

Conaclos opened this issue May 15, 2023 · 7 comments · Fixed by biomejs/biome#23
Labels
A-Linter Area: linter good first issue Good for newcomers Help wanted Help would be really appreciated I-Normal Implementation: normal understanding of the tool and awareness task A task, an action that needs to be performed

Comments

@Conaclos
Copy link
Contributor

Description

Implement the recommended ESLint rule @typescript-eslint/no-this-alias.

Note that, we should accept restructuring assignment such as const { props, state } = this.

@Conaclos Conaclos added good first issue Good for newcomers Help wanted Help would be really appreciated task A task, an action that needs to be performed A-Linter Area: linter I-Easy Implementation: easy task, usually a good fit for new contributors labels May 15, 2023
@ematipico
Copy link
Contributor

I have mixed feelings about this rule. I don't know how the TS eslint rule works, but in the web there are valid cases where this rule could create noise.

Should the rule take in consideration those cases? What does this rule prevent?

@Conaclos
Copy link
Contributor Author

Conaclos commented May 16, 2023

I think that the main reason of the existence of this rule is to detect cases where () => this could be used instead of function() { self }. In this regard, we could opt for a more specific rule such as useArrowFunctionThis? And then accepting this aliasing in all other cases?

@ematipico
Copy link
Contributor

I think that the main reason of the existence of this rule is to detect cases where () => this could be used instead of function() { self }.

That's my main concern 😅

Given the following code:

class B {
  foo() {
    const self = this;
    window.addEventListener('click', function(evt) {
        // do something with `this`
    })
  }
}

How should the rule behave in this case? Suggesting a .bind? Doing nothing?

@Conaclos
Copy link
Contributor Author

In the example do you mean // do something with `self` ?

@ematipico
Copy link
Contributor

Yeah sorry :)

@Conaclos
Copy link
Contributor Author

Conaclos commented May 17, 2023

In this case, the function should be replaced by an arrow lambda and self should be replaced by this.

Thinking a bit more about this rule, this could be decomposed into two rules:

  • useArrowFunction: suggest using arrow function instead of function expression when possible (i.e. no this use inside the function body).
  • noUselessThisAlias: remove useless aliasing of this.

@ematipico
Copy link
Contributor

ematipico commented May 17, 2023

Actually, sorry. I was wrong. I meant this in the comment.

The event listener binds the scope of this to the element that triggered the event, and maybe the user needs it. Maybe they need to read information from the HTML element (not the evt) and save it in the class.

Thinking a bit more about this rule, this could be decomposed into two rules:

* `useArrowLambda`: suggest using arrow lambda instead of function lambda when possible (i.e. no `this` use inside the lambda).

* `noUselessThisAlias`: remove useless aliasing of `this`.

I like your proposal very much! Narrow use cases, one rule for each one!

@Conaclos Conaclos added I-Normal Implementation: normal understanding of the tool and awareness and removed I-Easy Implementation: easy task, usually a good fit for new contributors labels Jun 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Linter Area: linter good first issue Good for newcomers Help wanted Help would be really appreciated I-Normal Implementation: normal understanding of the tool and awareness task A task, an action that needs to be performed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants