Add useAt option to autofix .lastObject to .at(-1) in no-get …

…rule (#1847) Co-authored-by: Dongyue Zhu <donzhu@linkedin.com>
ember-cli · May 31, 2023 · 957879c · 957879c
1 parent f97d491
commit 957879c
Show file tree

Hide file tree

Showing 3 changed files with 126 additions and 35 deletions.
diff --git a/docs/rules/no-get.md b/docs/rules/no-get.md
@@ -74,7 +74,7 @@ import ObjectProxy from '@ember/object/proxy';
 export default ObjectProxy.extend({
   someFunction() {
     const foo = this.get('propertyInsideProxyObject'); // Allowed because inside proxy object.
-  }
+  },
 });
 ```
 
@@ -85,7 +85,7 @@ export default EmberObject.extend({
   unknownProperty(key) {},
   someFunction() {
     const foo = this.get('property'); // Allowed because inside object implementing `unknownProperty()`.
-  }
+  },
 });
 ```
 
@@ -95,6 +95,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')` (can't be enabled at the same time as `useOptionalChaining`) (default `false`)
+- `boolean` -- `useAt` -- whether the rule should use `at(-1)` [Array.prototype.at()](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/at) to replace `lastObject` (default `false`) (TODO: enable by default once we require Node 16.6)
 - `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 `true`)
 - `boolean` -- `catchSafeObjects` -- whether the rule should catch non-`this` imported usages like `get(foo, 'bar')` (default `true`)
 - `boolean` -- `catchUnsafeObjects` -- whether the rule should catch non-`this` usages like `foo.get('bar')` even though we don't know for sure if `foo` is an Ember object (default `false`)

diff --git a/lib/rules/no-get.js b/lib/rules/no-get.js
@@ -23,7 +23,7 @@ function isValidJSPath(str) {
   return str.split('.').every((part) => isValidJSVariableName(part) || isValidJSArrayIndex(part));
 }
 
-function reportGet({ node, context, path, useOptionalChaining, objectText, sourceCode }) {
+function reportGet({ node, context, path, useAt, useOptionalChaining, objectText, sourceCode }) {
   const isInLeftSideOfAssignmentExpression = utils.isInLeftSideOfAssignmentExpression(node);
   context.report({
     node,
@@ -34,6 +34,7 @@ function reportGet({ node, context, path, useOptionalChaining, objectText, sourc
         fixer,
         path,
         useOptionalChaining,
+        useAt,
         isInLeftSideOfAssignmentExpression,
         objectText,
         sourceCode,
@@ -47,6 +48,7 @@ function fixGet({
   fixer,
   path,
   useOptionalChaining,
+  useAt,
   isInLeftSideOfAssignmentExpression,
   objectText,
   sourceCode,
@@ -63,12 +65,14 @@ function fixGet({
   if (types.isConditionalExpression(path)) {
     const newConsequentExpression = convertLiteralTypePath({
       path: path.consequent.value,
+      useAt,
       useOptionalChaining,
       shouldIgnoreOptionalChaining,
       objectText,
     });
     const newAlternateExpression = convertLiteralTypePath({
       path: path.alternate.value,
+      useAt,
       useOptionalChaining,
       shouldIgnoreOptionalChaining,
       objectText,
@@ -92,6 +96,7 @@ function fixGet({
 
   const replacementPath = convertLiteralTypePath({
     path,
+    useAt,
     useOptionalChaining,
     shouldIgnoreOptionalChaining,
     objectText,
@@ -137,7 +142,7 @@ function fixGetProperties({ fixer, node, objectText, propertyNames }) {
     // When destructuring assignment is in the left side of "=".
     // Example: const { foo, bar } = getProperties(this.obj, "foo", "bar");
     // Expectation:
-    // const const { foo, bar } = this.obj;
+    // const { foo, bar } = this.obj;
 
     return fixer.replaceText(node, `${objectText}`);
   }
@@ -183,6 +188,10 @@ module.exports = {
             type: 'boolean',
             default: false,
           },
+          useAt: {
+            type: 'boolean',
+            default: false,
+          },
         },
         additionalProperties: false,
       },
@@ -192,6 +201,7 @@ module.exports = {
     // Options:
     const ignoreGetProperties = context.options[0] && context.options[0].ignoreGetProperties;
     const ignoreNestedPaths = context.options[0] && context.options[0].ignoreNestedPaths;
+    const useAt = context.options[0] && context.options[0].useAt;
     const useOptionalChaining = context.options[0] && context.options[0].useOptionalChaining;
     const catchSafeObjects = context.options[0] ? context.options[0].catchSafeObjects : true;
     const catchUnsafeObjects = context.options[0] && context.options[0].catchUnsafeObjects;
@@ -295,6 +305,7 @@ module.exports = {
             path: node.arguments[0].value,
             isImportedGet: false,
             useOptionalChaining,
+            useAt,
             objectText: sourceCode.getText(node.callee.object),
           });
         }
@@ -315,6 +326,7 @@ module.exports = {
             path: node.arguments[1].value,
             isImportedGet: true,
             useOptionalChaining,
+            useAt,
             objectText: sourceCode.getText(node.arguments[0]),
           });
         }
@@ -458,6 +470,7 @@ function validateGetPropertiesArguments(args, ignoreNestedPaths) {
 
 function convertLiteralTypePath({
   path,
+  useAt,
   useOptionalChaining,
   shouldIgnoreOptionalChaining,
   objectText,
@@ -476,8 +489,8 @@ function convertLiteralTypePath({
     return null;
   }
 
-  if (path.match(/lastObject/g)?.length > 1) {
-    // Do not autofix when multiple `lastObject` are chained.
+  if (path.match(/lastObject/g)?.length > 1 && !useAt) {
+    // Do not autofix when multiple `lastObject` are chained, and use `at(-1)` is not allowed.
     return null;
   }
 
@@ -493,20 +506,27 @@ function convertLiteralTypePath({
   replacementPath = replacementPath
     .replace(/\.firstObject/g, shouldIgnoreOptionalChaining ? '[0]' : '.[0]') // When `firstObject` is used in the middle of the path. e.g. foo.firstObject
     .replace(/^firstObject\??\./, shouldIgnoreOptionalChaining ? '[0].' : '[0]?.') // When `firstObject` is used at the beginning of the path. e.g. firstObject.bar
-    .replace(/^firstObject$/, '[0]') // When `firstObject` is used as the entire path.
-    .replace(
-      /\??\.lastObject/, // When `lastObject` is used in the middle of the path. e.g. foo.lastObject
-      (_, offset) =>
-        `${shouldIgnoreOptionalChaining ? '' : '?.'}[${objectText}.${replacementPath.slice(
-          0,
-          offset
-        )}.length - 1]`
-    )
-    .replace(
-      /^lastObject\??\./, // When `lastObject` is used at the beginning of the path. e.g. lastObject.bar
-      `[${objectText}.length - 1]${shouldIgnoreOptionalChaining ? '.' : '?.'}`
-    )
-    .replace(/^lastObject$/, `[${objectText}.length - 1]`); // When `lastObject` is used as the entire path.
+    .replace(/^firstObject$/, '[0]'); // When `firstObject` is used as the entire path.
+
+  // eslint-disable-next-line unicorn/prefer-ternary
+  if (useAt) {
+    replacementPath = replacementPath.replace(/lastObject/g, 'at(-1)');
+  } else {
+    replacementPath = replacementPath
+      .replace(
+        /\??\.lastObject/, // When `lastObject` is used in the middle of the path. e.g. foo.lastObject
+        (_, offset) =>
+          `${shouldIgnoreOptionalChaining ? '' : '?.'}[${objectText}.${replacementPath.slice(
+            0,
+            offset
+          )}.length - 1]`
+      )
+      .replace(
+        /^lastObject\??\./, // When `lastObject` is used at the beginning of the path. e.g. lastObject.bar
+        `[${objectText}.length - 1]${shouldIgnoreOptionalChaining ? '.' : '?.'}`
+      )
+      .replace(/^lastObject$/, `[${objectText}.length - 1]`); // When `lastObject` is used as the entire path.
+  }
 
   const objectPathSeparator = replacementPath.startsWith('[') ? '' : '.';
 

diff --git a/tests/lib/rules/no-get.js b/tests/lib/rules/no-get.js
@@ -709,6 +709,19 @@ ruleTester.run('no-get', rule, {
         },
       ],
     },
+    {
+      // `firstObject` used in multiple places in a path.
+      // And the result of get() is chained (getResultIsChained=true).
+      code: "this.get('firstObject.foo.firstObject.bar')[123];",
+      output: 'this[0].foo[0].bar[123];',
+      options: [{ useOptionalChaining: true }],
+      errors: [
+        {
+          message: ERROR_MESSAGE_GET,
+          type: 'CallExpression',
+        },
+      ],
+    },
     {
       // `firstObject` used in the middle of a path.
       // And the resolved path of `get` is NOT chained (getResultIsChained=false).
@@ -765,8 +778,8 @@ ruleTester.run('no-get', rule, {
       // `lastObject` used in the middle of a path.
       // And the result of get() is chained (getResultIsChained=true).
       code: "this.get('foo.lastObject.bar')[123];",
-      output: 'this.foo[this.foo.length - 1].bar[123];',
-      options: [{ useOptionalChaining: true }],
+      output: 'this.foo.at(-1).bar[123];',
+      options: [{ useOptionalChaining: true, useAt: true }],
       errors: [
         {
           message: ERROR_MESSAGE_GET,
@@ -778,8 +791,8 @@ ruleTester.run('no-get', rule, {
       // `lastObject` used at the beginning of a path.
       // And the result of get() is chained (getResultIsChained=true).
       code: "this.get('lastObject.bar')[123];",
-      output: 'this[this.length - 1].bar[123];',
-      options: [{ useOptionalChaining: true }],
+      output: 'this.at(-1).bar[123];',
+      options: [{ useOptionalChaining: true, useAt: true }],
       errors: [
         {
           message: ERROR_MESSAGE_GET,
@@ -791,8 +804,8 @@ ruleTester.run('no-get', rule, {
       // `lastObject` used as the entire path.
       // And the result of get() is chained (getResultIsChained=true).
       code: "this.get('lastObject')[123];",
-      output: 'this[this.length - 1][123];',
-      options: [{ useOptionalChaining: true }],
+      output: 'this.at(-1)[123];',
+      options: [{ useOptionalChaining: true, useAt: true }],
       errors: [
         {
           message: ERROR_MESSAGE_GET,
@@ -804,8 +817,8 @@ ruleTester.run('no-get', rule, {
       // `lastObject` used in the middle of a path.
       // And the resolved path of `get` is NOT chained (getResultIsChained=false).
       code: "this.get('foo.lastObject.bar');",
-      output: 'this.foo?.[this.foo.length - 1]?.bar;',
-      options: [{ useOptionalChaining: true }],
+      output: 'this.foo?.at(-1)?.bar;',
+      options: [{ useOptionalChaining: true, useAt: true }],
       errors: [
         {
           message: ERROR_MESSAGE_GET,
@@ -817,8 +830,8 @@ ruleTester.run('no-get', rule, {
       // `lastObject` used at the beginning of a path.
       // And the resolved path of `get` is NOT chained (getResultIsChained=false).
       code: "this.get('lastObject.bar');",
-      output: 'this[this.length - 1]?.bar;',
-      options: [{ useOptionalChaining: true }],
+      output: 'this.at(-1)?.bar;',
+      options: [{ useOptionalChaining: true, useAt: true }],
       errors: [
         {
           message: ERROR_MESSAGE_GET,
@@ -827,11 +840,11 @@ ruleTester.run('no-get', rule, {
       ],
     },
     {
-      // `lastObject` used in the middle of a path.
-      // When multiple `lastObject` are chained, it won't auto-fix.
+      // multiple `lastObject` used in the middle of a path.
+      // And the result of get() is chained (getResultIsChained=true).
       code: "this.get('foo.lastObject.bar.lastObject')[123];",
-      output: null,
-      options: [{ useOptionalChaining: true }],
+      output: 'this.foo.at(-1).bar.at(-1)[123];',
+      options: [{ useOptionalChaining: true, useAt: true }],
       errors: [
         {
           message: ERROR_MESSAGE_GET,
@@ -843,8 +856,65 @@ ruleTester.run('no-get', rule, {
       // `lastObject` used at the beginning of a path.
       // And the result of get() is chained (getResultIsChained=true).
       code: "this.get('lastObject.bar.lastObject')[123];",
+      output: 'this.at(-1).bar.at(-1)[123];',
+      options: [{ useOptionalChaining: true, useAt: true }],
+      errors: [
+        {
+          message: ERROR_MESSAGE_GET,
+          type: 'CallExpression',
+        },
+      ],
+    },
+
+    {
+      // useAt = false.
+      // `lastObject` used at the beginning of a path.
+      // And the resolved path of `get` is NOT chained (getResultIsChained=false).
+      code: "this.get('lastObject.bar');",
+      output: 'this[this.length - 1]?.bar;',
+      options: [{ useOptionalChaining: true, useAt: false }],
+      errors: [
+        {
+          message: ERROR_MESSAGE_GET,
+          type: 'CallExpression',
+        },
+      ],
+    },
+    {
+      // useAt = false.
+      // `lastObject` used as the entire path.
+      // And the result of get() is chained (getResultIsChained=true).
+      code: "this.get('lastObject')[123];",
+      output: 'this[this.length - 1][123];',
+      options: [{ useOptionalChaining: true, useAt: false }],
+      errors: [
+        {
+          message: ERROR_MESSAGE_GET,
+          type: 'CallExpression',
+        },
+      ],
+    },
+    {
+      //  useAt = false.
+      // multiple `lastObject` used in the middle of a path.
+      // And the result of get() is chained (getResultIsChained=true).
+      code: "this.get('foo.lastObject.bar.lastObject')[123];",
       output: null,
-      options: [{ useOptionalChaining: true }],
+      options: [{ useOptionalChaining: true, useAt: false }],
+      errors: [
+        {
+          message: ERROR_MESSAGE_GET,
+          type: 'CallExpression',
+        },
+      ],
+    },
+    {
+      //  useAt = false.
+      // `lastObject` used in the middle of a path.
+      // And the resolved path of `get` is NOT chained (getResultIsChained=false).
+      code: "this.get('foo.lastObject.bar');",
+      output: 'this.foo?.[this.foo.length - 1]?.bar;',
+      options: [{ useOptionalChaining: true, useAt: false }],
       errors: [
         {
           message: ERROR_MESSAGE_GET,