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

react-bind-this-issue doesn't detect bind decorators defined after usage #813

Closed
cyberhck opened this issue Feb 7, 2019 · 2 comments 路 Fixed by #852 or microsoft/rushstack#1802
Closed
Labels
Difficulty: Easy Someone with little to no experience in TSLint should be able to send a pull request for this issue. Status: Accepting PRs Type: Bug
Milestone

Comments

@cyberhck
Copy link
Contributor

cyberhck commented Feb 7, 2019

Bug Report

  • tslint-microsoft-contrib version: latest
  • TSLint version: latest
  • TypeScript version: latest
  • Running TSLint via: (pick one) CLI

In PR #534 , I introduced a feature, but it contains a missing feature A.K.A 馃悰

It does half the work, it only detects bound methods above usage, and not the ones below usages.

TypeScript code being linted

class Component extends React.Component {
    public render(): JSX.Element {
        return (
            <button onClick={this.handleClick}></button>
        );
    }
    @autobind
    private handleClick(): void {
        console.info("clicked");
    }
}

with tslint.json configuration:

{
  "rules": {
    "react-this-binding-issue": [true, {"bind-decorators": ["autobind"]}]
  }
}

Actual behavior

Shouldn't report violation

Expected behavior

Reports violation

However the same code works if I do something like this:

class Component extends React.Component {
    @autobind
    private handleClick(): void {
        console.info("clicked");
    }
    public render(): JSX.Element {
        return (
            <button onClick={this.handleClick}></button>
        );
    }
}

Difference is that the method went up.

And I know what happened.

On the source code for react-this-binding-issue we're looking for method definitions, and JSX tags, and when we see methods, we push it to array if it's bound, but that method isn't encountered yet, it isn't listed in the bound methods, hence it'll report violation.

I don't know how I missed this when I created this PR and looks like no one has noticed till now 馃槃

Any suggestions are welcome, if I find some free time, I'll try to come up with a solution.

@cyberhck cyberhck changed the title react-bind-this-issue react-bind-this-issue has a 馃悰 Feb 7, 2019
@cyberhck cyberhck changed the title react-bind-this-issue has a 馃悰 react-bind-this-issue doesn't detect bind decorators defined after usage Feb 7, 2019
@IllusionMH
Copy link
Contributor

Good catch 馃槃

I think it is possible not to add methods to list when they are walked, but instead compose list of all bound/unbound methods as soon walker reaches ClassDeclaration node.
So it basically would get members property, iterate over it and use existing logic for MethodDeclatation's (check if there are bind decorators), PropertyDeclarations (mark lambdas as bound), and Constructor (check if there are binding in it).

And then continue walking over class AST to find usage of unbound methods.

@IllusionMH IllusionMH added Type: Bug Status: Accepting PRs Difficulty: Easy Someone with little to no experience in TSLint should be able to send a pull request for this issue. labels Feb 7, 2019
@cyberhck
Copy link
Contributor Author

cyberhck commented Feb 9, 2019

I'm not sure if I can get free time to do this, if someone else wants to give it a try, please do, I'll try if I find free time, do mention before starting to work (or assign yourself :) )

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Difficulty: Easy Someone with little to no experience in TSLint should be able to send a pull request for this issue. Status: Accepting PRs Type: Bug
Projects
None yet
3 participants