feat(ReactThisBindingIssueRule): allow decorators to bind

README.md

@@ -145,7 +145,7 @@ Rule Name   | Description | Since
 `react-anchor-blank-noopener`   | For security reasons, anchor tags with target="_blank" should also include rel="noopener noreferrer". In order to restrict the behavior window.opener access, the original page needs to add a rel="noopener" attribute to any link that has target="_blank". However, Firefox does not support that tag, so you should actually use rel="noopener noreferrer" for full coverage. For more info see: [The target="_blank" vulnerability by example](https://dev.to/ben/the-targetblank-vulnerability-by-example)| 2.0.11
 `react-iframe-missing-sandbox`  | React iframes must specify a sandbox attribute. If specified as an empty string, this attribute enables extra restrictions on the content that can appear in the inline frame. The value of the attribute can either be an empty string (all the restrictions are applied), or a space-separated list of tokens that lift particular restrictions. You many not use both allow-scripts and allow-same-origin at the same time, as that allows the embedded document to programmatically remove the sandbox attribute in some scenarios. | 2.0.10
 `react-no-dangerous-html`       | Do not use React's dangerouslySetInnerHTML API. This rule finds usages of the dangerouslySetInnerHTML API (but not any JSX references). For more info see the [react-no-dangerous-html Rule wiki page](https://github.com/Microsoft/tslint-microsoft-contrib/wiki/react-no-dangerous-html-Rule). | 0.0.2
-`react-this-binding-issue`      | Several errors can occur when using React and React.Component subclasses. When using React components you must be careful to correctly bind the 'this' reference on any methods that you pass off to child components as callbacks. For example, it is common to define a private method called 'onClick' and then specify `onClick={this.onClick}` as a JSX attribute. If you do this then the 'this' reference will be undefined when your private method is invoked. The React documentation suggests that you bind the 'this' reference on all of your methods within the constructor: `this.onClick = this.onClick.bind(this);`. This rule will create a violation if 1) a method reference is passed to a JSX attribute without being bound in the constructor. And 2) a method is bound multiple times in the constructor. Another issue that can occur is binding the 'this' reference to a function within the render() method. For example, many people will create an anonymous lambda within the JSX attribute to avoid the 'this' binding issue: `onClick={() => { this.onClick(); }}`. The problem with this is that a new instance of an anonymous function is created every time render() is invoked. When React compares virtual DOM properties within shouldComponentUpdate() then the onClick property will look like a new property and force a re-render. You should avoid this pattern because creating function instances within render methods breaks any logic within shouldComponentUpdate() methods. This rule creates violations if 1) an anonymous function is passed as a JSX attribute. And 2) if a function instantiated in local scope is passed as a JSX attribute. This rule can be configured via the "allow-anonymous-listeners" parameter. If you want to suppress violations for the anonymous listener scenarios then configure that rule like this: `"react-this-binding-issue": [ true, { 'allow-anonymous-listeners': true } ]` | 2.0.8, 2.0.9
+`react-this-binding-issue`      | Several errors can occur when using React and React.Component subclasses. When using React components you must be careful to correctly bind the 'this' reference on any methods that you pass off to child components as callbacks. For example, it is common to define a private method called 'onClick' and then specify `onClick={this.onClick}` as a JSX attribute. If you do this then the 'this' reference will be undefined when your private method is invoked. The React documentation suggests that you bind the 'this' reference on all of your methods within the constructor: `this.onClick = this.onClick.bind(this);`. This rule will create a violation if 1) a method reference is passed to a JSX attribute without being bound in the constructor. And 2) a method is bound multiple times in the constructor. Another issue that can occur is binding the 'this' reference to a function within the render() method. For example, many people will create an anonymous lambda within the JSX attribute to avoid the 'this' binding issue: `onClick={() => { this.onClick(); }}`. The problem with this is that a new instance of an anonymous function is created every time render() is invoked. When React compares virutual DOM properties within shouldComponentUpdate() then the onClick property will look like a new property and force a re-render. You should avoid this pattern because creating function instances within render methods breaks any logic within shouldComponentUpdate() methods. This rule creates violations if 1) an anonymous function is passed as a JSX attribute. And 2) if a function instantiated in local scope is passed as a JSX attribute. This rule can be configured via the "allow-anonymous-listeners" parameter. If you want to suppress violations for the anonymous listener scenarios then configure that rule like this: `"react-this-binding-issue": [ true, { 'allow-anonymous-listeners': true } ]`, you can also pass in array of string which can act as bind method decorators: `"react-this-bind-issue": [true, {'bind-decorators': ['autobind']}]` | 2.0.8, 2.0.9
 `react-tsx-curly-spacing`       | Consistently use spaces around the brace characters of JSX attributes. You can either allow or ban spaces between the braces and the values they enclose. <br/><br/>One of the two following options are required:<br/>* "always" enforces a space inside of curly braces (default)<br/>* "never" disallows spaces inside of curly braces<br/><br/>By default, braces spanning multiple lines are not allowed with either setting. If you want to allow them you can specify an additional allowMultiline property with the value false. <br/><br/>Examples: <br/>* "react-tsx-curly-spacing": [true, "always"]<br/>* "react-tsx-curly-spacing": [true, "never"]<br/>* "react-tsx-curly-spacing": [true, "never", {"allowMultiline": false}]<br/><br/>References<br/>* [eslint-plugin-react jsx-curly-spacing rule](https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-curly-spacing.md) <br/>* [tslint-react jsx-curly-spacing rule](https://github.com/palantir/tslint-react)  | 2.0.14
 `react-unused-props-and-state`  | Remove unneeded properties defined in React Props and State interfaces. Any interface named Props or State is defined as a React interface. All fields in these interfaces must be referenced. This rule can be configured with regexes to match custom Props and State interface names. <br/><br/>Example for including all interfaces ending with Props or State: <br/>*[ true, { 'props-interface-regex': 'Props$', 'state-interface-regex': 'State$' } ]* | 2.0.10
 `underscore-consistent-invocation` | Enforce a consistent usage of the _ functions. By default, invoking underscore functions should begin with wrapping a variable in an underscore instance: `_(list).map(...)`. An alternative is to prefer using the static methods on the _ variable: `_.map(list, ...)`. The rule accepts single parameter called 'style' which can be the value 'static' or 'instance': `[true, { "style": "static" }]`| 2.0.10

src/reactThisBindingIssueRule.ts

@@ -10,8 +10,9 @@ import {ExtendedMetadata} from './utils/ExtendedMetadata';
 const FAILURE_ANONYMOUS_LISTENER: string = 'A new instance of an anonymous method is passed as a JSX attribute: ';
 const FAILURE_DOUBLE_BIND: string = 'A function is having its \'this\' reference bound twice in the constructor: ';
 const FAILURE_UNBOUND_LISTENER: string = 'A class method is passed as a JSX attribute without having the \'this\' ' +
-    'reference bound in the constructor: ';
+    'reference bound: ';
 
+const optionExamples: any = [true, {'bind-decorators': ['autobind']}];
 /**
  * Implementation of the react-this-binding-issue rule.
  */
@@ -22,7 +23,23 @@ export class Rule extends Lint.Rules.AbstractRule {
         type: 'maintainability',
         description: 'When using React components you must be careful to correctly bind the `this` reference ' +
                      'on any methods that you pass off to child components as callbacks.',
-        options: null,
+        options: {
+            type: 'object',
+            properties: {
+                'allow-anonymous-listeners': {
+                    type: 'boolean'
+                },
+                'bind-decorators': {
+                    type: 'list',
+                    listType: {
+                        anyOf: {
+                            type: 'string'
+                        }
+                    }
+                }
+            }
+        },
+        optionExamples,
         optionsDescription: '',
         typescriptOnly: true,
         issueClass: 'Non-SDL',
@@ -44,6 +61,7 @@ export class Rule extends Lint.Rules.AbstractRule {
 class ReactThisBindingIssueRuleWalker extends ErrorTolerantWalker {
 
     private allowAnonymousListeners: boolean = false;
+    private allowedDecorators: string[] = [];
     private boundListeners: string[] = [];
     private declaredMethods: string[] = [];
     private scope: Scope | null = null;
@@ -53,6 +71,17 @@ class ReactThisBindingIssueRuleWalker extends ErrorTolerantWalker {
         this.getOptions().forEach((opt: any) => {
             if (typeof(opt) === 'object') {
                 this.allowAnonymousListeners = opt['allow-anonymous-listeners'] === true;
+                if (opt['bind-decorators']) {
+                    const allowedDecorators: any[] = opt['bind-decorators'];
+                    if (
+                        !Array.isArray(allowedDecorators)
+                        || allowedDecorators.some(decorator => typeof decorator !== 'string')
+                    ) {
+                        throw new Error('one or more members of bind-decorators is invalid, string required.');
+                    }
+                    // tslint:disable-next-line:prefer-type-cast
+                    this.allowedDecorators = allowedDecorators;
+                }
             }
         });
     }
@@ -86,11 +115,28 @@ class ReactThisBindingIssueRuleWalker extends ErrorTolerantWalker {
     protected visitMethodDeclaration(node: ts.MethodDeclaration): void {
         // reset variable scope when we encounter a method. Start tracking variables that are instantiated
         // in scope so we can make sure new function instances are not passed as JSX attributes
+        if (this.isMethodBoundWithDecorators(node, this.allowedDecorators)) {
+            this.boundListeners = this.boundListeners.concat('this.' + node.name.getText());
+        }
         this.scope = new Scope(null);
         super.visitMethodDeclaration(node);
         this.scope = null;
     }
 
+    private isMethodBoundWithDecorators(node: ts.MethodDeclaration, allowedDecorators: string[]): boolean {
+        if (!(allowedDecorators.length > 0 && node.decorators && node.decorators.length > 0)) {
+            return false;
+        }
+        return node.decorators.some((decorator) => {
+            if (decorator.kind !== ts.SyntaxKind.Decorator) {
+                return false;
+            }
+            const source = node.getSourceFile();
+            const text = decorator.expression.getText(source);
+            return this.allowedDecorators.indexOf(text) !== -1;
+        });
+    }
+
     protected visitArrowFunction(node: ts.ArrowFunction): void {
         if (this.scope != null) {
             this.scope = new Scope(this.scope);

src/tests/ReactThisBindingIssueRuleTests.ts

@@ -11,6 +11,22 @@ describe('reactThisBindingIssueRule', () : void => {
         const file : string = 'test-data/ReactThisBinding/ReactThisBindingIssue-passing.tsx';
 
         TestHelper.assertViolations(ruleName, file, [ ]);
+
+        const fileWithDecorator : string = 'test-data/ReactThisBinding/ReactThisBindingIssueWithDecorator-passing.tsx';
+        TestHelper.assertNoViolationWithOptions(ruleName, [true, {'bind-decorators': ['autobind']}], fileWithDecorator);
+    });
+
+    it('should fail if decorator is not whitelisted in config', () => {
+        const fileWithDecorator : string = 'test-data/ReactThisBinding/ReactThisBindingIssueWithDecorator-passing.tsx';
+        const expectedMsg = `A class method is passed as a JSX attribute without having the 'this' reference bound: `;
+        TestHelper.assertViolationsWithOptions(ruleName, [true], fileWithDecorator, [
+            {
+                ruleName: 'react-this-binding-issue',
+                name: fileWithDecorator,
+                failure: expectedMsg + 'this.listener1',
+                startPosition: {line: 24, character: 22}
+            }
+        ]);
     });
 
     it('should fail on double binding', () : void => {
@@ -32,15 +48,13 @@ describe('reactThisBindingIssueRule', () : void => {
 
         TestHelper.assertViolations(ruleName, file, [
             {
-                "failure": "A class method is passed as a JSX attribute without having the 'this' reference " +
-                "bound in the constructor: this.listener",
+                "failure": "A class method is passed as a JSX attribute without having the 'this' reference bound: this.listener",
                 "name": "test-data/ReactThisBinding/ReactThisBindingIssue-unbound.tsx",
                 "ruleName": "react-this-binding-issue",
                 "startPosition": { "character": 22, "line": 11  }
             },
             {
-                "failure": "A class method is passed as a JSX attribute without having the 'this' reference " +
-                "bound in the constructor: this.listener",
+                "failure": "A class method is passed as a JSX attribute without having the 'this' reference bound: this.listener",
                 "name": "test-data/ReactThisBinding/ReactThisBindingIssue-unbound.tsx",
                 "ruleName": "react-this-binding-issue",
                 "startPosition": { "character": 28, "line": 14 }

test-data/ReactThisBinding/ReactThisBindingIssue-passing.tsx

@@ -27,9 +27,9 @@ export class MyComponent extends BaseClass {
 
     public render() {
         return <div
-            onClick={this.listener1}>
-            onMouseOver={this.listener2}>
-            onMouseDown={this.listener3}>
+            onClick={this.listener1}
+            onMouseOver={this.listener2}
+            onMouseDown={this.listener3}
             onMouseMove={this.notALocalMethod}> // this is not a real method, so it is OK
         </div>;
     }

test-data/ReactThisBinding/ReactThisBindingIssueWithDecorator-passing.tsx

@@ -0,0 +1,29 @@
+import React = require('react');
+
+export class BaseClass extends React.Component<{}, {}> {
+    public notALocalMethod(): void {
+    }
+}
+declare const autobind: MethodDecorator;
+
+export class MyComponent extends BaseClass {
+
+    @autobind
+    private listener1(): void {
+    }
+
+    @autobind
+    private listener2(): void {
+    }
+
+    // this listener is a lambda, so it has its 'this' reference bound correctly.
+    private listener3: () => void = (): void => {};
+
+    public render() {
+        return <div
+            onClick={this.listener1}
+            onMouseDown={this.listener3}
+            onMouseMove={this.notALocalMethod}> // this is not a real method, so it is OK
+        </div>;
+    }
+}

tsconfig.testdata.json

@@ -1,6 +1,7 @@
 {
     "compilerOptions": {
         "declaration": true,
+        "experimentalDecorators": true,
         "jsx": "react",
         "module": "commonjs",
         "outDir": "dist/test-data",