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

Add detection for @bind decorators in react-this-binding #392

Closed
connor4312 opened this issue Sep 6, 2017 · 8 comments · Fixed by #534
Closed

Add detection for @bind decorators in react-this-binding #392

connor4312 opened this issue Sep 6, 2017 · 8 comments · Fixed by #534
Labels
Difficulty: Medium People with non-trivial experience in TSLint should be able to send a pull request for this issue. Microsoft Internal Issues related to closed source Microsoft code. Status: Accepting PRs Type: Rule Feature Adding a feature to an existing rule.
Milestone

Comments

@connor4312
Copy link
Member

It's common practice to do something like the following:

import { bind } from 'decko';

class Foo extends Component {
  public render() {
    return <button onclick={this.sayHi}>Greet</button>;
  }

  @bind
  public sayHi() {
    alert('hi');
  }
}

Unfortunately the react-this-binding-issue rule isn't aware of this and gets tripped up.

I propose adding an option like { "bindDecorators": ["bind"] } which looks at decorator names and flags functions with those decorators as having been bound. It can have sensible defaults like ["bind", "Bind"].

@HamletDRC HamletDRC added the Microsoft Internal Issues related to closed source Microsoft code. label Dec 18, 2017
@JoshuaKGoldberg JoshuaKGoldberg added Status: Accepting PRs Difficulty: Medium People with non-trivial experience in TSLint should be able to send a pull request for this issue. Type: Rule Feature Adding a feature to an existing rule. labels Jul 5, 2018
@cyberhck
Copy link
Contributor

cyberhck commented Oct 1, 2018

is this being considered?

@JoshuaKGoldberg
Copy link

@cyberhck absolutely! Everything with the Status: Accepting PRs label is open to being contributed. Feel free to tackle this if you have time! 😊

@cyberhck
Copy link
Contributor

cyberhck commented Oct 1, 2018

I would, but I don't even know how to go about this 😄

@IllusionMH
Copy link
Contributor

@cyberhck if you are still interested in implementation I can provide you with tips about possible implementation.

Otherwise I can take on this issue if there are no more volunteers.

@cyberhck
Copy link
Contributor

cyberhck commented Oct 6, 2018

Yes please, can you provide me some tips? Since this will be my first attempt, I don't know of helper methods, and I don't know how to detect if a function is bound or not.

@IllusionMH
Copy link
Contributor

@cyberhck ok.
First - understanding of AST for TypeScript code would help. You can use https://astexplorer.net/ (need to pick TypeScript as parser in the header) or https://ts-ast-viewer.com/ to see what are relations between nodes (ClassDeclaration, MethodDeclaration, Decorator etc.)

Next - files related to rule: rule source and tests.

In rule source code you'll need to add options parser inside Walker constructor (you can check other rules for example) and in VisitClassDeclaration instead of empty array of bound declarations you want to add list of methods that has decorator with identifier specified in options. I'd propose to move code from AstUtils.getDeclaredMethodNames to this method and process both declaredMethods and boundListeners in the single run, but it up to you.

Test are regular Mocha tests with helpers that allow to compare liter result with array of error objects. For your case you'll need to use TestHelper.assertViolationsWithOptions and pass configuration with list of decorator names that will bind methods.

@cyberhck
Copy link
Contributor

cyberhck commented Oct 9, 2018

I've enabled experimentalDecorators for test data. Will send a PR soonish.
can someone please review :)

@connor4312
Copy link
Member Author

Great job @cyberhck!

@JoshuaKGoldberg JoshuaKGoldberg added this to the 6.0.0 milestone Nov 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Difficulty: Medium People with non-trivial experience in TSLint should be able to send a pull request for this issue. Microsoft Internal Issues related to closed source Microsoft code. Status: Accepting PRs Type: Rule Feature Adding a feature to an existing rule.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants