Repository: ember-cli/eslint-plugin-ember
Pull request #1640 (2022-10-21 13:43:14)

Avoid in-place sorting in `sortBy` autofixer in `no-array-prototype-extensions` rule
## Summary
Bug fixes for `sortBy` autofixer.

## Description
This PR contains the following changes
- **Bug:** The ember's `sortBy` method on the array will shallow clone the array and then run sort on the cloned copy of the array. However, the cloning part of the array got missed out in the original implementation of the auto-fixer. Used spread operator to clone the array.
- **Optimization:** When a single argument is passed to sortBy, there is no need to store the compare function output and instead directly return the output of compare function.

tgvrssanthosh: @bmish Can you please review?

Patch:
diff --git a/lib/rules/no-array-prototype-extensions.js b/lib/rules/no-array-prototype-extensions.js
index 1a82df06..4bacfb38 100644
--- a/lib/rules/no-array-prototype-extensions.js
+++ b/lib/rules/no-array-prototype-extensions.js
@@ -424,28 +424,35 @@ function applyFix(callExpressionNode, fixer, context, options = {}) {
         // default to `compare` if the `compare` hasn't already been imported.
         const importedGetName = options.importedGetName ?? 'get';
 
-        const compareValueCheckText = `const compareValue = ${importedCompareName}(${importedGetName}(a, key), ${importedGetName}(b, key));
-            if (compareValue) {
-              return compareValue;
-            }`;
         const argsText = callArgs.map((arg) => sourceCode.getText(arg)).join(', ');
 
-        // Loop through keys if more than one of them.
-        const sortFn =
-          callArgs.length === 1
-            ? `(a, b) => {
-            const key = ${argsText};
-            ${compareValueCheckText}
+        let sortFn;
+        const comparisonFnInString = (key) =>
+          `${importedCompareName}(${importedGetName}(a, ${key}), ${importedGetName}(b, ${key}))`;
+        if (callArgs.length === 1 && callArgs[0].type !== 'SpreadElement') {
+          sortFn =
+            callArgs[0].type === 'Identifier' || callArgs[0].type === 'Literal'
+              ? `(a, b) => ${comparisonFnInString(argsText)}`
+              : `(a, b) => {
+              const key = ${argsText};
+              return ${comparisonFnInString('key')};
+            }`;
+        } else {
+          // Loop through keys if there are more than one argument
+          sortFn = `(a, b) => {
+            for (const key of [${argsText}]) {
+              const compareValue = ${comparisonFnInString('key')};
+              if (compareValue) {
+                return compareValue;
+              }
+            }
             return 0;
-          }`
-            : `(a, b) => {
-          for (const key of [${argsText}]) {
-            ${compareValueCheckText}
-          }
-          return 0;
-        }`;
+          }`;
+        }
 
         fixes.push(
+          // Duplicate array
+          fixer.replaceText(calleeObj, `[...${sourceCode.getText(calleeObj)}]`),
           fixer.replaceText(calleeProp, 'sort'),
           // Replacing the content starting from open parenthesis to close parenthesis
           fixer.replaceTextRange([openParenToken.range[0], closeParenToken.range[1]], `(${sortFn})`)


Test patch:
diff --git a/tests/lib/rules/no-array-prototype-extensions.js b/tests/lib/rules/no-array-prototype-extensions.js
index b3fef633..26db42ec 100644
--- a/tests/lib/rules/no-array-prototype-extensions.js
+++ b/tests/lib/rules/no-array-prototype-extensions.js
@@ -832,27 +832,47 @@ import { set as s } from '@custom/object';
       code: "something.sortBy('abc', 'def')",
       output: `import { get } from '@ember/object';
 import { compare } from '@ember/utils';
-something.sort((a, b) => {
-          for (const key of ['abc', 'def']) {
-            const compareValue = compare(get(a, key), get(b, key));
-            if (compareValue) {
-              return compareValue;
+[...something].sort((a, b) => {
+            for (const key of ['abc', 'def']) {
+              const compareValue = compare(get(a, key), get(b, key));
+              if (compareValue) {
+                return compareValue;
+              }
             }
-          }
-          return 0;
-        })`,
+            return 0;
+          })`,
       errors: [{ messageId: 'main', type: 'CallExpression' }],
     },
     {
-      // Single argument.
+      // Single call expression argument.
       code: 'something.sortBy(getKey())',
       output: `import { get } from '@ember/object';
 import { compare } from '@ember/utils';
-something.sort((a, b) => {
-            const key = getKey();
-            const compareValue = compare(get(a, key), get(b, key));
-            if (compareValue) {
-              return compareValue;
+[...something].sort((a, b) => {
+              const key = getKey();
+              return compare(get(a, key), get(b, key));
+            })`,
+      errors: [{ messageId: 'main', type: 'CallExpression' }],
+    },
+    {
+      // Single non CallExpression argument.
+      code: "something.sortBy('abc')",
+      output: `import { get } from '@ember/object';
+import { compare } from '@ember/utils';
+[...something].sort((a, b) => compare(get(a, 'abc'), get(b, 'abc')))`,
+      errors: [{ messageId: 'main', type: 'CallExpression' }],
+    },
+    {
+      // Spread element as argument
+      code: 'something.sortBy(...abc)',
+      output: `import { get } from '@ember/object';
+import { compare } from '@ember/utils';
+[...something].sort((a, b) => {
+            for (const key of [...abc]) {
+              const compareValue = compare(get(a, key), get(b, key));
+              if (compareValue) {
+                return compareValue;
+              }
             }
             return 0;
           })`,
@@ -863,15 +883,15 @@ something.sort((a, b) => {
       code: "something.sortBy('abc', def)",
       output: `import { get } from '@ember/object';
 import { compare } from '@ember/utils';
-something.sort((a, b) => {
-          for (const key of ['abc', def]) {
-            const compareValue = compare(get(a, key), get(b, key));
-            if (compareValue) {
-              return compareValue;
+[...something].sort((a, b) => {
+            for (const key of ['abc', def]) {
+              const compareValue = compare(get(a, key), get(b, key));
+              if (compareValue) {
+                return compareValue;
+              }
             }
-          }
-          return 0;
-        })`,
+            return 0;
+          })`,
       errors: [{ messageId: 'main', type: 'CallExpression' }],
     },
     {
@@ -880,15 +900,15 @@ something.sort((a, b) => {
       something.sortBy('abc', 'def')`,
       output: `import { get } from '@ember/object';
 import { compare } from '@ember/utils';
-      something.sort((a, b) => {
-          for (const key of ['abc', 'def']) {
-            const compareValue = compare(get(a, key), get(b, key));
-            if (compareValue) {
-              return compareValue;
+      [...something].sort((a, b) => {
+            for (const key of ['abc', 'def']) {
+              const compareValue = compare(get(a, key), get(b, key));
+              if (compareValue) {
+                return compareValue;
+              }
             }
-          }
-          return 0;
-        })`,
+            return 0;
+          })`,
       errors: [{ messageId: 'main', type: 'CallExpression' }],
     },
     {
@@ -897,15 +917,15 @@ import { compare } from '@ember/utils';
       something.sortBy('abc', 'def')`,
       output: `import { get } from '@ember/object';
 import { compare as comp } from '@ember/utils';
-      something.sort((a, b) => {
-          for (const key of ['abc', 'def']) {
-            const compareValue = comp(get(a, key), get(b, key));
-            if (compareValue) {
-              return compareValue;
+      [...something].sort((a, b) => {
+            for (const key of ['abc', 'def']) {
+              const compareValue = comp(get(a, key), get(b, key));
+              if (compareValue) {
+                return compareValue;
+              }
             }
-          }
-          return 0;
-        })`,
+            return 0;
+          })`,
       errors: [{ messageId: 'main', type: 'CallExpression' }],
     },
     {
@@ -915,15 +935,15 @@ import { compare as comp } from '@ember/utils';
       output: `import { get } from '@ember/object';
 import { compare } from '@ember/utils';
 import { compare as comp } from '@custom/utils';
-      something.sort((a, b) => {
-          for (const key of ['abc', 'def']) {
-            const compareValue = compare(get(a, key), get(b, key));
-            if (compareValue) {
-              return compareValue;
+      [...something].sort((a, b) => {
+            for (const key of ['abc', 'def']) {
+              const compareValue = compare(get(a, key), get(b, key));
+              if (compareValue) {
+                return compareValue;
+              }
             }
-          }
-          return 0;
-        })`,
+            return 0;
+          })`,
       errors: [{ messageId: 'main', type: 'CallExpression' }],
     },
     {
@@ -934,15 +954,15 @@ import { compare as comp } from '@custom/utils';
       output: `import { compare } from '@ember/utils';
 import { compare as comp } from '@custom/utils';
       import { get } from '@ember/object';
-      something.sort((a, b) => {
-          for (const key of ['abc', 'def']) {
-            const compareValue = compare(get(a, key), get(b, key));
-            if (compareValue) {
-              return compareValue;
+      [...something].sort((a, b) => {
+            for (const key of ['abc', 'def']) {
+              const compareValue = compare(get(a, key), get(b, key));
+              if (compareValue) {
+                return compareValue;
+              }
             }
-          }
-          return 0;
-        })`,
+            return 0;
+          })`,
       errors: [{ messageId: 'main', type: 'CallExpression' }],
     },
     {
@@ -954,15 +974,15 @@ import { compare as comp } from '@custom/utils';
 import { compare } from '@ember/utils';
 import { compare as comp } from '@custom/utils';
       import { get as g } from '@custom/object';
-      something.sort((a, b) => {
-          for (const key of ['abc', 'def']) {
-            const compareValue = compare(get(a, key), get(b, key));
-            if (compareValue) {
-              return compareValue;
+      [...something].sort((a, b) => {
+            for (const key of ['abc', 'def']) {
+              const compareValue = compare(get(a, key), get(b, key));
+              if (compareValue) {
+                return compareValue;
+              }
             }
-          }
-          return 0;
-        })`,
+            return 0;
+          })`,
       errors: [{ messageId: 'main', type: 'CallExpression' }],
     },
     {
@@ -973,15 +993,15 @@ import { compare as comp } from '@custom/utils';
       output: `import { compare } from '@ember/utils';
 import { compare as comp } from '@custom/utils';
       import { get as g } from '@ember/object';
-      something.sort((a, b) => {
-          for (const key of ['abc', 'def']) {
-            const compareValue = compare(g(a, key), g(b, key));
-            if (compareValue) {
-              return compareValue;
+      [...something].sort((a, b) => {
+            for (const key of ['abc', 'def']) {
+              const compareValue = compare(g(a, key), g(b, key));
+              if (compareValue) {
+                return compareValue;
+              }
             }
-          }
-          return 0;
-        })`,
+            return 0;
+          })`,
       errors: [{ messageId: 'main', type: 'CallExpression' }],
     },
     {