Merge pull request #845 from bmish/no-get-optional-chaining

Add `useOptionalChaining` option (default false) to `no-get` rule

Author: bmish

docs/rules/no-get.md

@@ -50,6 +50,10 @@ Examples of **correct** code for this rule:
 const foo = this.someProperty;
 ```
 
+```js
+const foo = this.nested?.path; // Optional chaining can be useful if the nested path can have null or undefined properties in it.
+```
+
 ```js
 const foo = this.get('some.nested.property'); // Allowed if `ignoreNestedPaths` option is enabled.
 ```
@@ -89,6 +93,7 @@ This rule takes an optional object containing:
 
 * `boolean` -- `ignoreGetProperties` -- whether the rule should ignore `getProperties` (default `false`)
 * `boolean` -- `ignoreNestedPaths` -- whether the rule should ignore `this.get('some.nested.property')` (default `false`)
+* `boolean` -- `useOptionalChaining` -- whether the rule should use the [optional chaining operator](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining) `?.` to autofix nested paths such as `this.get('some.nested.property')` to `this.some?.nested?.property` (when this option is off, these nested paths won't be autofixed at all) (default `false`)
 
 ## Related Rules
 

lib/rules/no-get.js

@@ -2,11 +2,19 @@
 
 const types = require('../utils/types');
 const emberUtils = require('../utils/ember');
+const assert = require('assert');
 
-function makeErrorMessageForGet(property, isImportedGet) {
-  return isImportedGet
-    ? `Use \`this.${property}\` instead of \`get(this, '${property}')\``
-    : `Use \`this.${property}\` instead of \`this.get('${property}')\``;
+function makeErrorMessageForGet(property, { isImportedGet, useOptionalChaining } = {}) {
+  const original = isImportedGet ? `get(this, '${property}')` : `this.get('${property}')`;
+
+  const replacements = [
+    `this.${property}`,
+    useOptionalChaining ? `this.${property.replace(/\./g, '?.')}` : undefined,
+  ]
+    .filter(Boolean)
+    .join('` or `');
+
+  return `Use \`${replacements}\` instead of \`${original}\``;
 }
 
 const ERROR_MESSAGE_GET_PROPERTIES =
@@ -16,6 +24,24 @@ const VALID_JS_VARIABLE_NAME_REGEXP = new RegExp('^[a-zA-Z_$][0-9a-zA-Z_$]*$');
 function isValidJSVariableName(str) {
   return VALID_JS_VARIABLE_NAME_REGEXP.test(str);
 }
+function isValidJSPath(str) {
+  return str.split('.').every(isValidJSVariableName);
+}
+
+function fix(node, fixer, path, useOptionalChaining) {
+  if (path.includes('.') && !useOptionalChaining) {
+    // Not safe to autofix nested properties because some properties in the path might be null or undefined.
+    return null;
+  }
+
+  if (!isValidJSPath(path)) {
+    // Do not autofix since the path would not be a valid JS path.
+    return null;
+  }
+
+  // If we get to this point and there are periods present, convert them to the optional chaining operator.
+  return fixer.replaceText(node, `this.${path.replace(/\./g, '?.')}`);
+}
 
 module.exports = {
   makeErrorMessageForGet,
@@ -41,6 +67,10 @@ module.exports = {
             type: 'boolean',
             default: false,
           },
+          useOptionalChaining: {
+            type: 'boolean',
+            default: false,
+          },
         },
         additionalProperties: false,
       },
@@ -50,6 +80,14 @@ module.exports = {
     // Options:
     const ignoreGetProperties = context.options[0] && context.options[0].ignoreGetProperties;
     const ignoreNestedPaths = context.options[0] && context.options[0].ignoreNestedPaths;
+    const useOptionalChaining = context.options[0] && context.options[0].useOptionalChaining;
+
+    if (ignoreNestedPaths && useOptionalChaining) {
+      assert(
+        false,
+        'Do not enable both the `ignoreNestedPaths` and `useOptionalChaining` options on this rule at the same time.'
+      );
+    }
 
     let currentProxyObject = null;
     let currentClassWithUnknownPropertyMethod = null;
@@ -124,19 +162,9 @@ module.exports = {
           const path = node.arguments[0].value;
           context.report({
             node,
-            message: makeErrorMessageForGet(path, false),
+            message: makeErrorMessageForGet(path, { isImportedGet: false, useOptionalChaining }),
             fix(fixer) {
-              if (path.includes('.')) {
-                // Not safe to autofix nested properties because some properties in the path might be null.
-                return null;
-              }
-
-              if (!isValidJSVariableName(path)) {
-                // Do not autofix since the path would not be a valid JS variable name.
-                return null;
-              }
-
-              return fixer.replaceText(node, `this.${path}`);
+              return fix(node, fixer, path, useOptionalChaining);
             },
           });
         }
@@ -153,19 +181,9 @@ module.exports = {
           const path = node.arguments[1].value;
           context.report({
             node,
-            message: makeErrorMessageForGet(path, true),
+            message: makeErrorMessageForGet(path, { isImportedGet: true, useOptionalChaining }),
             fix(fixer) {
-              if (path.includes('.')) {
-                // Not safe to autofix nested properties because some properties in the path might be null.
-                return null;
-              }
-
-              if (!isValidJSVariableName(path)) {
-                // Do not autofix since the path would not be a valid JS variable name.
-                return null;
-              }
-
-              return fixer.replaceText(node, `this.${path}`);
+              return fix(node, fixer, path, useOptionalChaining);
             },
           });
         }

tests/lib/rules/no-get.js

@@ -5,6 +5,7 @@ const RuleTester = require('eslint').RuleTester;
 const { makeErrorMessageForGet, ERROR_MESSAGE_GET_PROPERTIES } = rule;
 
 const ruleTester = new RuleTester({
+  parser: require.resolve('babel-eslint'),
   parserOptions: {
     ecmaVersion: 2015,
     sourceType: 'module',
@@ -156,6 +157,9 @@ ruleTester.run('no-get', rule, {
       }
     }
     `,
+
+    // Optional chaining:
+    'this.foo?.bar',
   ],
   invalid: [
     // **************************
@@ -165,29 +169,46 @@ ruleTester.run('no-get', rule, {
     {
       code: "this.get('foo');",
       output: 'this.foo;',
-      errors: [{ message: makeErrorMessageForGet('foo', false), type: 'CallExpression' }],
+      // Error message intentionally written out to ensure it looks right.
+      errors: [{ message: "Use `this.foo` instead of `this.get('foo')`", type: 'CallExpression' }],
     },
     {
       code: "get(this, 'foo');",
       output: 'this.foo;',
-      errors: [{ message: makeErrorMessageForGet('foo', true), type: 'CallExpression' }],
+      // Error message intentionally written out to ensure it looks right.
+      errors: [{ message: "Use `this.foo` instead of `get(this, 'foo')`", type: 'CallExpression' }],
     },
     {
       code: "this.get('foo').someFunction();",
       output: 'this.foo.someFunction();',
-      errors: [{ message: makeErrorMessageForGet('foo', false), type: 'CallExpression' }],
+      errors: [
+        {
+          message: makeErrorMessageForGet('foo', { isImportedGet: false }),
+          type: 'CallExpression',
+        },
+      ],
     },
     {
       // With invalid JS variable name:
       code: "this.get('foo-bar');",
       output: null,
-      errors: [{ message: makeErrorMessageForGet('foo-bar', false), type: 'CallExpression' }],
+      errors: [
+        {
+          message: makeErrorMessageForGet('foo-bar', { isImportedGet: false }),
+          type: 'CallExpression',
+        },
+      ],
     },
     {
       // With invalid JS variable name:
       code: "get(this, 'foo-bar');",
       output: null,
-      errors: [{ message: makeErrorMessageForGet('foo-bar', true), type: 'CallExpression' }],
+      errors: [
+        {
+          message: makeErrorMessageForGet('foo-bar', { isImportedGet: true }),
+          type: 'CallExpression',
+        },
+      ],
     },
 
     // **************************
@@ -219,12 +240,22 @@ ruleTester.run('no-get', rule, {
     {
       code: "this.get('foo.bar');",
       output: null,
-      errors: [{ message: makeErrorMessageForGet('foo.bar', false), type: 'CallExpression' }],
+      errors: [
+        {
+          message: makeErrorMessageForGet('foo.bar', { isImportedGet: false }),
+          type: 'CallExpression',
+        },
+      ],
     },
     {
       code: "get(this, 'foo.bar');",
       output: null,
-      errors: [{ message: makeErrorMessageForGet('foo.bar', true), type: 'CallExpression' }],
+      errors: [
+        {
+          message: makeErrorMessageForGet('foo.bar', { isImportedGet: true }),
+          type: 'CallExpression',
+        },
+      ],
     },
     {
       code: "this.getProperties('foo.bar');",
@@ -237,6 +268,76 @@ ruleTester.run('no-get', rule, {
       errors: [{ message: ERROR_MESSAGE_GET_PROPERTIES, type: 'CallExpression' }],
     },
 
+    // Nested paths with optional chaining:
+    {
+      code: "this.get('foo.bar');",
+      options: [{ useOptionalChaining: true }],
+      output: 'this.foo?.bar;',
+      errors: [
+        {
+          // Error message intentionally written out to ensure it looks right.
+          message: "Use `this.foo.bar` or `this.foo?.bar` instead of `this.get('foo.bar')`",
+          type: 'CallExpression',
+        },
+      ],
+    },
+    {
+      code: "this.get('very.long.path');",
+      options: [{ useOptionalChaining: true }],
+      output: 'this.very?.long?.path;',
+      errors: [
+        {
+          message: makeErrorMessageForGet('very.long.path', {
+            isImportedGet: false,
+            useOptionalChaining: true,
+          }),
+          type: 'CallExpression',
+        },
+      ],
+    },
+    {
+      code: "get(this, 'foo.bar');",
+      options: [{ useOptionalChaining: true }],
+      output: 'this.foo?.bar;',
+      errors: [
+        {
+          message: makeErrorMessageForGet('foo.bar', {
+            isImportedGet: true,
+            useOptionalChaining: true,
+          }),
+          type: 'CallExpression',
+        },
+      ],
+    },
+    {
+      code: "get(this, 'very.long.path');",
+      options: [{ useOptionalChaining: true }],
+      output: 'this.very?.long?.path;',
+      errors: [
+        {
+          message: makeErrorMessageForGet('very.long.path', {
+            isImportedGet: true,
+            useOptionalChaining: true,
+          }),
+          type: 'CallExpression',
+        },
+      ],
+    },
+    {
+      code: "this.get('foo');", // No nested path.
+      options: [{ useOptionalChaining: true }],
+      output: 'this.foo;',
+      errors: [
+        {
+          message: makeErrorMessageForGet('foo', {
+            isImportedGet: false,
+            useOptionalChaining: true,
+          }),
+          type: 'CallExpression',
+        },
+      ],
+    },
+
     {
       // Reports violation after (classic) proxy class.
       code: `