Repository: ember-cli/eslint-plugin-ember
Pull request #403 (2019-04-07 00:09:53)

Add new `no-get-properties` rule

bmish: @rwjblue thanks for the reminder about proxies! I removed the autofixer as a result. I also updated the documentation to call that out.

However, I would prefer if the rule continues to catch `someForeignObject.getProperties`, because:
* It no longer autofixes
* `es5-getter-ember-codemod` currently converts that (the codemod isn't limited to `this` only)

How does that sound?

Turbo87: I would propose to keep the autofixer, but make the rule configurable. By default it would only work for `this`, but you could either configure other variable names for which it should also work, or you define a wildcard or something like that 🤔 
rwjblue: > es5-getter-ember-codemod currently converts that (the codemod isn't limited to this only)

There are a number of reported issues on that repo around this exact issue. The codemod absolutely should be restricted to `this`.
bmish: @rwjblue thanks for calling that out, I updated this rule to be limited to `this.getProperties` only.

@Turbo87 I would still lean towards leaving the autofixer out because:

* If someone wants to fix violations, they can run the codemod.
* Even when the rule is limited to `this.getProperties` only, it still can easily cause breaking behavior changes because of proxy objects / `unknownProperty`.

However, I'd like to follow-up in a separate PR to add an `catchForeignObjects` boolean configuration property (default `false`) to this rule for those who want to be more aggressive. 

Patch:
diff --git a/README.md b/README.md
index f63fd5b7..a45dc8f4 100644
--- a/README.md
+++ b/README.md
@@ -89,6 +89,7 @@ The `--fix` option on the command line automatically fixes problems reported by
 | :white_check_mark: | [new-module-imports](./docs/rules/new-module-imports.md) |  Use "New Module Imports" from Ember RFC #176 |
 |  | [no-empty-attrs](./docs/rules/no-empty-attrs.md) | Prevents usage of empty attributes in ember data models |
 | :white_check_mark: | [no-function-prototype-extensions](./docs/rules/no-function-prototype-extensions.md) | Prevents usage of Ember's `function` prototype extensions |
+|  | [no-get-properties](./docs/rules/no-get-properties.md) | Disallow unnecessary usage of Ember's `getProperties` function |
 |  | [no-get](./docs/rules/no-get.md) | Disallow unnecessary usage of Ember's `get` function |
 | :white_check_mark: | [no-global-jquery](./docs/rules/no-global-jquery.md) | Prevents usage of global jQuery object |
 |  | [no-jquery](./docs/rules/no-jquery.md) | Disallow any usage of jQuery |
diff --git a/docs/rules/no-get-properties.md b/docs/rules/no-get-properties.md
new file mode 100644
index 00000000..e88875f9
--- /dev/null
+++ b/docs/rules/no-get-properties.md
@@ -0,0 +1,50 @@
+## Disallow unnecessary usage of Ember\'s `getProperties` function
+
+### Rule name: `no-get-properties`
+
+Starting in Ember 3.1, native ES5 getters are available, which eliminates much of the need to use `get` and `getProperties` on Ember objects. In particular, `getProperties` no longer needs to be used with destructuring assignments.
+
+### Rule Details
+
+This rule disallows unnecessarily using `this.getProperties()` with destructuring assignments.
+
+**WARNING**: there are a number of circumstances where `getProperties` still needs to be used, and you may need to manually disable the rule for these:
+
+* Ember proxy objects (`ObjectProxy`, `ArrayProxy`)
+* Objects implementing the `unknownProperty` method
+
+### Examples
+
+Examples of **incorrect** code for this rule:
+
+```js
+const { abc, def } = this.getProperties('abc', 'def');
+```
+
+```js
+import { getProperties } from '@ember/object';
+const { abc, def } = getProperties(this, 'abc', 'def');
+```
+
+Examples of **correct** code for this rule:
+
+```js
+const { abc, def } = this;
+```
+
+```js
+const { foo, barBaz } = this.getProperties('foo', 'bar.baz'); // Allowed because of nested path.
+```
+
+### References
+
+* [JavaScript Destructuring Assignment Spec](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment)
+* [Ember 3.1 Release Notes](https://blog.emberjs.com/2018/04/13/ember-3-1-released.html) describing "ES5 Getters for Computed Properties"
+* [Ember getProperties Spec](https://api.emberjs.com/ember/release/functions/@ember%2Fobject/getProperties)
+* [Ember ES5 Getter RFC](https://github.com/emberjs/rfcs/blob/master/text/0281-es5-getters.md)
+* [es5-getter-ember-codemod](https://github.com/rondale-sc/es5-getter-ember-codemod)
+* [More context](https://github.com/emberjs/ember.js/issues/16148) about the proxy object exception to this rule
+
+### Related Rules
+
+* [no-get](no-get.md)
diff --git a/docs/rules/no-get.md b/docs/rules/no-get.md
index e2c25e5f..a7262585 100644
--- a/docs/rules/no-get.md
+++ b/docs/rules/no-get.md
@@ -42,3 +42,7 @@ const foo = this.get('some.nested.property'); // Allowed because of nested path.
 * [Ember ES5 Getter RFC](https://github.com/emberjs/rfcs/blob/master/text/0281-es5-getters.md)
 * [es5-getter-ember-codemod](https://github.com/rondale-sc/es5-getter-ember-codemod)
 * [More context](https://github.com/emberjs/ember.js/issues/16148) about the proxy object exception to this rule
+
+## Related Rules
+
+* [no-get-properties](no-get-properties.md)
diff --git a/lib/index.js b/lib/index.js
index cc18a112..a1baf5e5 100644
--- a/lib/index.js
+++ b/lib/index.js
@@ -21,6 +21,7 @@ module.exports = {
     'no-ember-testing-in-module-scope': require('./rules/no-ember-testing-in-module-scope'),
     'no-empty-attrs': require('./rules/no-empty-attrs'),
     'no-function-prototype-extensions': require('./rules/no-function-prototype-extensions'),
+    'no-get-properties': require('./rules/no-get-properties'),
     'no-get': require('./rules/no-get'),
     'no-global-jquery': require('./rules/no-global-jquery'),
     'no-invalid-debug-function-arguments': require('./rules/no-invalid-debug-function-arguments'),
diff --git a/lib/recommended-rules.js b/lib/recommended-rules.js
index 93354b74..30d6393e 100644
--- a/lib/recommended-rules.js
+++ b/lib/recommended-rules.js
@@ -23,6 +23,7 @@ module.exports = {
   "ember/no-ember-testing-in-module-scope": "error",
   "ember/no-empty-attrs": "off",
   "ember/no-function-prototype-extensions": "error",
+  "ember/no-get-properties": "off",
   "ember/no-get": "off",
   "ember/no-global-jquery": "error",
   "ember/no-invalid-debug-function-arguments": "off",
@@ -48,4 +49,4 @@ module.exports = {
   "ember/routes-segments-snake-case": "error",
   "ember/use-brace-expansion": "error",
   "ember/use-ember-get-and-set": "off"
-}
+}
\ No newline at end of file
diff --git a/lib/rules/no-get-properties.js b/lib/rules/no-get-properties.js
new file mode 100644
index 00000000..5863fc66
--- /dev/null
+++ b/lib/rules/no-get-properties.js
@@ -0,0 +1,64 @@
+'use strict';
+
+const utils = require('../utils/utils');
+
+//------------------------------------------------------------------------------
+// Rule Definition
+//------------------------------------------------------------------------------
+
+const ERROR_MESSAGE = 'Ember\'s `getProperties` function can be omitted in a destructuring assignment.';
+
+module.exports = {
+  meta: {
+    docs: {
+      description: 'Disallow unnecessary usage of Ember\'s `getProperties` function',
+      category: 'Best Practices',
+      recommended: false
+    },
+    ERROR_MESSAGE
+  },
+
+  create(context) {
+    return {
+      VariableDeclarator(node) {
+        if (!node.id || !utils.isObjectPattern(node.id)) {
+          return;
+        }
+
+        if (!node.init || !utils.isCallExpression(node.init)) {
+          return;
+        }
+
+        if (
+          utils.isMemberExpression(node.init.callee) &&
+          utils.isThisExpression(node.init.callee.object) &&
+          utils.isIdentifier(node.init.callee.property) &&
+          node.init.callee.property.name === 'getProperties' &&
+          validateGetPropertiesArguments(node.init.arguments)
+        ) {
+          // Example: const { abc, def } = this.getProperties('abc', 'def');
+          context.report(node.init, ERROR_MESSAGE);
+        }
+
+        if (
+          utils.isIdentifier(node.init.callee) &&
+          node.init.callee.name === 'getProperties' &&
+          node.init.arguments.length >= 1 &&
+          utils.isThisExpression(node.init.arguments[0]) &&
+          validateGetPropertiesArguments(node.init.arguments.slice(1))
+        ) {
+          // Example: const { abc, def } = getProperties(this, 'abc', 'def');
+          context.report(node.init, ERROR_MESSAGE);
+        }
+      }
+    };
+  }
+};
+
+function validateGetPropertiesArguments(args) {
+  if (args.length === 1 && utils.isArrayExpression(args[0])) {
+    return validateGetPropertiesArguments(args[0].elements);
+  }
+  // We can only handle string arguments without nested property paths.
+  return args.every(argument => utils.isString(argument) && !argument.value.includes('.'));
+}
diff --git a/lib/rules/no-get.js b/lib/rules/no-get.js
index c8b2c10c..474da180 100644
--- a/lib/rules/no-get.js
+++ b/lib/rules/no-get.js
@@ -25,8 +25,7 @@ module.exports = {
           utils.isThisExpression(node.callee.object) &&
           node.callee.property.name === 'get' &&
           node.arguments.length === 1 &&
-          utils.isLiteral(node.arguments[0]) &&
-          typeof node.arguments[0].value === 'string' &&
+          utils.isString(node.arguments[0]) &&
           !node.arguments[0].value.includes('.')
         ) {
           // Example: this.get('foo');
@@ -38,8 +37,7 @@ module.exports = {
           node.callee.name === 'get' &&
           node.arguments.length === 2 &&
           utils.isThisExpression(node.arguments[0]) &&
-          utils.isLiteral(node.arguments[1]) &&
-          typeof node.arguments[1].value === 'string' &&
+          utils.isString(node.arguments[1]) &&
           !node.arguments[1].value.includes('.')
         ) {
           // Example: get(this, 'foo');
diff --git a/lib/rules/no-invalid-debug-function-arguments.js b/lib/rules/no-invalid-debug-function-arguments.js
index 5cfd21bc..2d6cc071 100644
--- a/lib/rules/no-invalid-debug-function-arguments.js
+++ b/lib/rules/no-invalid-debug-function-arguments.js
@@ -44,8 +44,8 @@ function isDebugFunctionWithReversedArgs(node) {
   return (
     isDebugFunction(node) &&
     node.arguments.length >= 2 &&
-    !isString(node.arguments[0]) &&
-    isString(node.arguments[1])
+    !utils.isString(node.arguments[0]) &&
+    utils.isString(node.arguments[1])
   );
 }
 
@@ -59,7 +59,3 @@ function getDebugFunction(node) {
       (!node.callee.property || node.callee.property.name === debugFunction)
   );
 }
-
-function isString(node) {
-  return utils.isTemplateLiteral(node) || (utils.isLiteral(node) && typeof node.value === 'string');
-}
diff --git a/lib/utils/utils.js b/lib/utils/utils.js
index 76429d32..c77b37ca 100644
--- a/lib/utils/utils.js
+++ b/lib/utils/utils.js
@@ -20,6 +20,7 @@ module.exports = {
   isTemplateLiteral,
   isTaggedTemplateExpression,
   isGlobalCallExpression,
+  isString,
   getSize,
   parseCallee,
   parseArgs,
@@ -267,6 +268,10 @@ function isGlobalCallExpression(node, destructuredName, aliases) {
   return !isDestructured && isGlobalCall;
 }
 
+function isString(node) {
+  return isTemplateLiteral(node) || (isLiteral(node) && typeof node.value === 'string');
+}
+
 /**
  * Get size of expression in lines
  *


Test patch:
diff --git a/tests/lib/rules/no-get-properties.js b/tests/lib/rules/no-get-properties.js
new file mode 100644
index 00000000..16821bba
--- /dev/null
+++ b/tests/lib/rules/no-get-properties.js
@@ -0,0 +1,83 @@
+//------------------------------------------------------------------------------
+// Requirements
+//------------------------------------------------------------------------------
+
+const rule = require('../../../lib/rules/no-get-properties');
+const RuleTester = require('eslint').RuleTester;
+
+const { ERROR_MESSAGE } = rule;
+
+//------------------------------------------------------------------------------
+// Tests
+//------------------------------------------------------------------------------
+
+const ruleTester = new RuleTester({
+  parserOptions: {
+    ecmaVersion: 2015,
+    sourceType: 'module'
+  }
+});
+
+ruleTester.run('no-get-properties', rule, {
+  valid: [
+    // Simple variable declaration.
+    'const abc = 123;',
+
+    // Not `this`.
+    "const { abc, def } = myObject.getProperties('abc', 'def');",
+    "const { abc, def } = this.get('something').getProperties('abc', 'def');",
+
+    // Destructuring assignment but no `getProperties`.
+    'const { abc, def } = this;',
+
+    // No destructuring.
+    "const properties = this.getProperties('abc', 'def');",
+    "const properties = this.getProperties(['abc', 'def']);", // With parameters in array.
+    "const properties = getProperties(this, 'abc', 'def');",
+    "const properties = getProperties(this, ['abc', 'def']);", // With parameters in array.
+
+    // Destructuring assignment with nested path.
+    "const { foo, barBaz } = this.getProperties('foo', 'bar.baz');",
+    "const { foo, barBaz } = this.getProperties(['foo', 'bar.baz']);", // With parameters in array.
+    "const { foo, barBaz } = getProperties(this, 'foo', 'bar.baz');",
+    "const { foo, barBaz } = getProperties(this, ['foo', 'bar.baz']);", // With parameters in array.
+
+    // With some wrapping function call.
+    "const { abc, def } = someFunction(this.getProperties('abc', 'def'));",
+
+    // With non-string parameter.
+    'const { abc } = this.getProperties(MY_PROP);',
+    'const { abc } = this.getProperties([MY_PROP]);',
+    'const { abc } = getProperties(this, MY_PROP);',
+    'const { abc } = getProperties(this, [MY_PROP]);'
+  ],
+  invalid: [
+    {
+      code: "const { abc, def } = this.getProperties('abc', 'def');",
+      output: null,
+      errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }]
+    },
+    {
+      code: "const { abc, def } = this.getProperties(['abc', 'def']);", // With parameters in array.
+      output: null,
+      errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }]
+    },
+    {
+      code: "const { abc, def } = getProperties(this, 'abc', 'def');",
+      output: null,
+      errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }]
+    },
+    {
+      code: "const { abc, def } = getProperties(this, ['abc', 'def']);", // With parameters in array.
+      output: null,
+      errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }]
+    },
+
+    // With `let`.
+    {
+      code: "let { abc, def } = this.getProperties('abc', 'def');",
+      output: null,
+      errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }]
+    }
+  ]
+});