palantir / tslint-react

:orange_book: Lint rules related to React & JSX for TSLint.
Apache License 2.0
749 stars 76 forks source link

jsx-no-bind bug? #201

Closed ksrb closed 5 years ago

ksrb commented 5 years ago
class Test extends Component {
  handleClick = () => {}

  render() {
    return (
      <div>
        <div onClick={this.handleClick.bind(this)}></div> {/* <-- This gets flagged */}
        <div onClick={this.handleClick.bind(this, "")}></div> {/* <-- This doesn't get flagged */}
      </div>
    );
  }
}

I understand that this.handleClick.bind(this) is redundant as the handleClick is already bound, but the rule isn't behaving the way I expected. It seems to be ignoring any bind with arguments past the first one. I expect the rule to flag both of above statements.

Version info:

    "tslint": "5.12.1",
    "typescript": "3.2.2",
marianhlavac commented 5 years ago

Confirmed, I too think it should be flagged.

onChange={this.handleSetPerPage.bind(this)}  // jsx-no-bind
onChange={this.handleSetPerPage.bind(this, 1)} // nothing
"dependencies": {
    "react": "^16.7.0",
    "typescript": "^3.2.4"
  },
  "devDependencies": {
    "tslint": "^5.12.1",
    "tslint-config-standard": "^8.0.1",
    "tslint-react": "^3.6.0"
  }
adidahiya commented 5 years ago

The rule explicitly allows .bind usage which is not simply .bind(this), because it might be necessary to use the additional arguments (which make .bind no longer functionally equivalent to () =>)

ksrb commented 5 years ago

Perhaps a quick documentation change?

adidahiya commented 5 years ago

It's not a rare false positive, the rule is functioning as intended. There is a semantic difference between .bind(this) and .bind(this, whatever)

hs-matt commented 5 years ago

While I know that tslint-react doesn't claim to be equivalent to eslint-plugin-react, there is a pretty good justification for not using bind, even if the multiple argument version is used, and in fact the eslint version of jsx-no-bind does check for this case: https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-no-bind.md

Since there seems to be a difference of opinion, perhaps we could just add an option to this rule?