Only alphabetize if it already was

facebook · Feb 20, 2019 · 12aeaca · 12aeaca
1 parent 0c722bb
commit 12aeaca
Show file tree

Hide file tree

Showing 2 changed files with 62 additions and 4 deletions.
diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js
@@ -1019,7 +1019,30 @@ const tests = {
           }, [c, a, g]);
         }
       `,
-      // Alphabetize during the autofix.
+      // Don't alphabetize if it wasn't alphabetized in the first place.
+      output: `
+        function MyComponent(props) {
+          let a, b, c, d, e, f, g;
+          useEffect(() => {
+            console.log(b, e, d, c, a, g, f);
+          }, [c, a, g, b, e, d, f]);
+        }
+      `,
+      errors: [
+        "React Hook useEffect has missing dependencies: 'b', 'd', 'e', and 'f'. " +
+          'Either include them or remove the dependency array.',
+      ],
+    },
+    {
+      code: `
+        function MyComponent(props) {
+          let a, b, c, d, e, f, g;
+          useEffect(() => {
+            console.log(b, e, d, c, a, g, f);
+          }, [a, c, g]);
+        }
+      `,
+      // Alphabetize if it was alphabetized.
       output: `
         function MyComponent(props) {
           let a, b, c, d, e, f, g;
@@ -1033,6 +1056,29 @@ const tests = {
           'Either include them or remove the dependency array.',
       ],
     },
+    {
+      code: `
+        function MyComponent(props) {
+          let a, b, c, d, e, f, g;
+          useEffect(() => {
+            console.log(b, e, d, c, a, g, f);
+          }, []);
+        }
+      `,
+      // Alphabetize if it was empty.
+      output: `
+        function MyComponent(props) {
+          let a, b, c, d, e, f, g;
+          useEffect(() => {
+            console.log(b, e, d, c, a, g, f);
+          }, [a, b, c, d, e, f, g]);
+        }
+      `,
+      errors: [
+        "React Hook useEffect has missing dependencies: 'a', 'b', 'c', 'd', 'e', 'f', and 'g'. " +
+          'Either include them or remove the dependency array.',
+      ],
+    },
     {
       code: `
         function MyComponent(props) {
@@ -1324,7 +1370,7 @@ const tests = {
             console.log(ref2.current.textContent);
             alert(props.someOtherRefs.current.innerHTML);
             fetch(props.color);
-          }, [props.color, props.someOtherRefs, ref1, ref2]);
+          }, [props.someOtherRefs, props.color, ref1, ref2]);
         }
       `,
       errors: [

diff --git a/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js b/packages/eslint-plugin-react-hooks/src/ReactiveDeps.js
@@ -341,8 +341,20 @@ export default {
           // Already did that. Do nothing.
         }
       });
-      // Alphabetize for the autofix.
-      suggestedDependencies.sort();
+
+      function areDeclaredDepsAlphabetized() {
+        if (declaredDependencies.length === 0) {
+          return true;
+        }
+        const declaredDepKeys = declaredDependencies.map(dep => dep.key);
+        const sortedDeclaredDepKeys = declaredDepKeys.slice().sort();
+        return declaredDepKeys.join(',') === sortedDeclaredDepKeys.join(',');
+      }
+
+      if (areDeclaredDepsAlphabetized()) {
+        // Alphabetize the autofix, but only if deps were already alphabetized.
+        suggestedDependencies.sort();
+      }
 
       const problemCount =
         duplicateDependencies.size +