Merge pull request #840 from bmish/require-computed-macros-decorator-support

bmish · web-flow · commit 8ae155be35db · 2020-06-07T19:17:30.000-05:00
Add `includeNativeGetters` option (default false) to `require-computed-macros` rule
diff --git a/docs/rules/require-computed-macros.md b/docs/rules/require-computed-macros.md
@@ -11,6 +11,8 @@ It is preferred to use Ember's computed property macros as opposed to manually w
 * Reduced chance of typos
 * Reduced chance of missing dependencies
 
+Note that by default, this rule only applies in classic classes (i.e. `Component.extend({})`) and not in native JavaScript classes with decorators (read more about the `includeNativeGetters` option in the configuration section of this doc).
+
 ## Rule Details
 
 This rule requires using Ember's computed property macros when possible.
@@ -113,6 +115,12 @@ export default Component.extend({
 });
 ```
 
+## Configuration
+
+This rule takes an optional object containing:
+
+* `boolean` -- `includeNativeGetters` -- whether the rule should check and autofix computed properties with native getters (i.e. `@computed() get someProp() {}`) to use computed property macros (default `false`). This is off by default because in the Ember Octane world, the better improvement would be to keep the native getter and use tracked properties instead of computed properties.
+
 ## References
 
 * [Guide](https://guides.emberjs.com/release/object-model/computed-properties/) for computed properties
@@ -121,9 +129,3 @@ export default Component.extend({
 ## Related Rules
 
 * [no-incorrect-computed-macros](no-incorrect-computed-macros.md)
-
-## Help Wanted
-
-| Issue | Link |
-| :-- | :-- |
-| :x: Missing native JavaScript class support | [#560](https://github.com/ember-cli/eslint-plugin-ember/issues/560) |
diff --git a/lib/rules/require-computed-macros.js b/lib/rules/require-computed-macros.js
@@ -113,6 +113,13 @@ function getThisExpressions(nodeLogicalExpression) {
   return arrayOfThisExpressions.reverse();
 }
 
+function makeFix(nodeToReplace, macro, macroArgs) {
+  const isDecoratorUsage = types.isMethodDefinition(nodeToReplace);
+  const prefix = isDecoratorUsage ? '@' : ''; // Decorator usage has @ symbol prefixing computed()
+  const suffix = isDecoratorUsage ? ` ${nodeToReplace.key.name}` : ''; // Decorator usage has property name as suffix.
+  return `${prefix}computed.${macro}(${macroArgs})${suffix}`;
+}
+
 module.exports = {
   meta: {
     type: 'suggestion',
@@ -124,7 +131,18 @@ module.exports = {
         'https://github.com/ember-cli/eslint-plugin-ember/tree/master/docs/rules/require-computed-macros.md',
     },
     fixable: 'code',
-    schema: [],
+    schema: [
+      {
+        type: 'object',
+        properties: {
+          includeNativeGetters: {
+            type: 'boolean',
+            default: false,
+          },
+        },
+        additionalProperties: false,
+      },
+    ],
   },
 
   ERROR_MESSAGE_READS,
@@ -146,7 +164,10 @@ module.exports = {
         message: makeErrorMessage(macro),
         fix(fixer) {
           const text = propertyGetterUtils.nodeToDependentKey(nodeWithThisExpression, context);
-          return fixer.replaceText(nodeComputedProperty, `computed.${macro}('${text}')`);
+          return fixer.replaceText(
+            nodeComputedProperty,
+            makeFix(nodeComputedProperty, macro, `'${text}'`)
+          );
         },
       });
     }
@@ -164,7 +185,7 @@ module.exports = {
           const textRight = sourceCode.getText(nodeBinaryExpression.right);
           return fixer.replaceText(
             nodeComputedProperty,
-            `computed.${macro}('${textLeft}', ${textRight})`
+            makeFix(nodeComputedProperty, macro, `'${textLeft}', ${textRight}`)
           );
         },
       });
@@ -178,7 +199,10 @@ module.exports = {
           const text = getThisExpressions(nodeLogicalExpression)
             .map((node) => propertyGetterUtils.nodeToDependentKey(node, context))
             .join("', '");
-          return fixer.replaceText(nodeComputedProperty, `computed.${macro}('${text}')`);
+          return fixer.replaceText(
+            nodeComputedProperty,
+            makeFix(nodeComputedProperty, macro, `'${text}'`)
+          );
         },
       });
     }
@@ -196,7 +220,7 @@ module.exports = {
           const restOfArgs = nodeCallExpression.arguments.map((arg) => sourceCode.getText(arg));
           return fixer.replaceText(
             nodeComputedProperty,
-            `computed.${macro}('${arg1}', ${restOfArgs.join(', ')})`
+            makeFix(nodeComputedProperty, macro, `'${arg1}', ${restOfArgs.join(', ')}`)
           );
         },
       });
@@ -209,24 +233,42 @@ module.exports = {
           return;
         }
 
-        if (node.arguments.length === 0) {
-          return;
-        }
+        // Options:
+        const includeNativeGetters = context.options[0] && context.options[0].includeNativeGetters;
 
-        const lastArg = node.arguments[node.arguments.length - 1];
-        if (!types.isFunctionExpression(lastArg)) {
+        let getterBody;
+        let nodeToReport;
+        if (
+          node.arguments.length > 0 &&
+          types.isFunctionExpression(node.arguments[node.arguments.length - 1])
+        ) {
+          // Example: computed('dependentKey', function() { return this.x })
+          getterBody = node.arguments[node.arguments.length - 1].body.body;
+          nodeToReport = node;
+        } else if (
+          includeNativeGetters &&
+          types.isDecorator(node.parent) &&
+          types.isMethodDefinition(node.parent.parent) &&
+          node.parent.parent.decorators.length === 1 &&
+          node.parent.parent.kind === 'get' &&
+          types.isFunctionExpression(node.parent.parent.value)
+        ) {
+          // Example: @computed() get someProp() { return this.x; }
+          getterBody = node.parent.parent.value.body.body;
+          nodeToReport = node.parent.parent;
+        } else {
           return;
         }
 
-        if (lastArg.body.body.length !== 1) {
+        if (getterBody.length !== 1) {
           return;
         }
 
-        if (!types.isReturnStatement(lastArg.body.body[0])) {
+        if (!types.isReturnStatement(getterBody[0])) {
           return;
         }
 
-        const statement = lastArg.body.body[0].argument;
+        const statement = getterBody[0].argument;
         if (!statement) {
           return;
         }
@@ -236,35 +278,35 @@ module.exports = {
           statement.operator === '!' &&
           propertyGetterUtils.isSimpleThisExpression(statement.argument)
         ) {
-          reportSingleArg(node, statement.argument, 'not');
+          reportSingleArg(nodeToReport, statement.argument, 'not');
         } else if (types.isLogicalExpression(statement)) {
           if (isSimpleThisExpressionsInsideLogicalExpression(statement, '&&')) {
-            reportLogicalExpression(node, statement, 'and');
+            reportLogicalExpression(nodeToReport, statement, 'and');
           } else if (isSimpleThisExpressionsInsideLogicalExpression(statement, '||')) {
-            reportLogicalExpression(node, statement, 'or');
+            reportLogicalExpression(nodeToReport, statement, 'or');
           }
         } else if (
           types.isBinaryExpression(statement) &&
           types.isLiteral(statement.right) &&
           propertyGetterUtils.isSimpleThisExpression(statement.left)
         ) {
           if (statement.operator === '===') {
-            reportBinaryExpression(node, statement, 'equal');
+            reportBinaryExpression(nodeToReport, statement, 'equal');
           } else if (statement.operator === '>') {
-            reportBinaryExpression(node, statement, 'gt');
+            reportBinaryExpression(nodeToReport, statement, 'gt');
           } else if (statement.operator === '>=') {
-            reportBinaryExpression(node, statement, 'gte');
+            reportBinaryExpression(nodeToReport, statement, 'gte');
           } else if (statement.operator === '<') {
-            reportBinaryExpression(node, statement, 'lt');
+            reportBinaryExpression(nodeToReport, statement, 'lt');
           } else if (statement.operator === '<=') {
-            reportBinaryExpression(node, statement, 'lte');
+            reportBinaryExpression(nodeToReport, statement, 'lte');
           }
         } else if (propertyGetterUtils.isSimpleThisExpression(statement)) {
-          reportSingleArg(node, statement, 'reads');
+          reportSingleArg(nodeToReport, statement, 'reads');
         } else if (isThisPropertyFunctionCall(statement, 'filterBy')) {
-          reportFunctionCall(node, statement, 'filterBy');
+          reportFunctionCall(nodeToReport, statement, 'filterBy');
         } else if (isThisPropertyFunctionCall(statement, 'mapBy')) {
-          reportFunctionCall(node, statement, 'mapBy');
+          reportFunctionCall(nodeToReport, statement, 'mapBy');
         }
       },
     };
diff --git a/tests/lib/rules/require-computed-macros.js b/tests/lib/rules/require-computed-macros.js
@@ -23,7 +23,14 @@ const {
 // Tests
 //------------------------------------------------------------------------------
 
-const ruleTester = new RuleTester();
+const ruleTester = new RuleTester({
+  parser: require.resolve('babel-eslint'),
+  parserOptions: {
+    ecmaVersion: 6,
+    sourceType: 'module',
+    ecmaFeatures: { legacyDecorators: true },
+  },
+});
 
 ruleTester.run('require-computed-macros', rule, {
   valid: [
@@ -91,19 +98,11 @@ ruleTester.run('require-computed-macros', rule, {
     // MAPBY
     "mapBy('children', 'age')",
 
-    // Decorator:
-    {
-      // TODO: this should be an invalid test case.
-      // Still missing native class and decorator support: https://github.com/ember-cli/eslint-plugin-ember/issues/560
-      code: 'class Test { @computed() get someProp() { return this.x; } }',
-      errors: [{ message: ERROR_MESSAGE_READS, type: 'CallExpression' }],
-      parser: require.resolve('babel-eslint'),
-      parserOptions: {
-        ecmaVersion: 6,
-        sourceType: 'module',
-        ecmaFeatures: { legacyDecorators: true },
-      },
-    },
+    // Decorator (these are ignored when the `includeNativeGetters` option is off):
+    "class Test { @computed('x') get someProp() { return this.x; } }",
+    'class Test { @computed() get someProp() { return this.x && this.y; } }',
+    'class Test { @computed() get someProp() { return this.x > 123; } }',
+    "class Test { @computed() get someProp() { return this.children.mapBy('age'); } }",
   ],
   invalid: [
     // READS
@@ -122,6 +121,18 @@ ruleTester.run('require-computed-macros', rule, {
       output: "computed.reads('x.y')",
       errors: [{ message: ERROR_MESSAGE_READS, type: 'CallExpression' }],
     },
+    {
+      code: "computed('x', function() { return this.x; })", // With dependent key.
+      output: "computed.reads('x')",
+      errors: [{ message: ERROR_MESSAGE_READS, type: 'CallExpression' }],
+    },
+    {
+      // Decorator:
+      code: "class Test { @computed('x') get someProp() { return this.x; } }",
+      options: [{ includeNativeGetters: true }],
+      output: "class Test { @computed.reads('x') someProp }",
+      errors: [{ message: ERROR_MESSAGE_READS, type: 'MethodDefinition' }],
+    },
 
     // AND
     {
@@ -144,6 +155,13 @@ ruleTester.run('require-computed-macros', rule, {
       output: "computed.and('x', 'y.z', 'w')",
       errors: [{ message: ERROR_MESSAGE_AND, type: 'CallExpression' }],
     },
+    {
+      // Decorator:
+      code: 'class Test { @computed() get someProp() { return this.x && this.y; } }',
+      options: [{ includeNativeGetters: true }],
+      output: "class Test { @computed.and('x', 'y') someProp }",
+      errors: [{ message: ERROR_MESSAGE_AND, type: 'MethodDefinition' }],
+    },
 
     // OR
     {
@@ -158,6 +176,13 @@ ruleTester.run('require-computed-macros', rule, {
       output: "computed.gt('x', 123)",
       errors: [{ message: ERROR_MESSAGE_GT, type: 'CallExpression' }],
     },
+    {
+      // Decorator:
+      code: 'class Test { @computed() get someProp() { return this.x > 123; } }',
+      options: [{ includeNativeGetters: true }],
+      output: "class Test { @computed.gt('x', 123) someProp }",
+      errors: [{ message: ERROR_MESSAGE_GT, type: 'MethodDefinition' }],
+    },
 
     // GTE
     {
@@ -217,5 +242,12 @@ ruleTester.run('require-computed-macros', rule, {
       output: "computed.mapBy('nested.children', 'age')",
       errors: [{ message: ERROR_MESSAGE_MAP_BY, type: 'CallExpression' }],
     },
+    {
+      // Decorator:
+      code: "class Test { @computed() get someProp() { return this.children.mapBy('age'); } }",
+      options: [{ includeNativeGetters: true }],
+      output: "class Test { @computed.mapBy('children', 'age') someProp }",
+      errors: [{ message: ERROR_MESSAGE_MAP_BY, type: 'MethodDefinition' }],
+    },
   ],
 });
