Validate useEffect without deps too (#15183)

facebook · Mar 21, 2019 · 78968bb · 78968bb
1 parent 4b8e164
commit 78968bb
Show file tree

Hide file tree

Showing 2 changed files with 59 additions and 1 deletion.
diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
@@ -2969,6 +2969,36 @@ const tests = {
           `and use that variable in the cleanup function.`,
       ],
     },
+    {
+      code: `
+        function MyComponent() {
+          const myRef = useRef();
+          useEffect(() => {
+            const handleMove = () => {};
+            myRef.current.addEventListener('mousemove', handleMove);
+            return () => myRef.current.removeEventListener('mousemove', handleMove);
+          });
+          return <div ref={myRef} />;
+        }
+      `,
+      output: `
+        function MyComponent() {
+          const myRef = useRef();
+          useEffect(() => {
+            const handleMove = () => {};
+            myRef.current.addEventListener('mousemove', handleMove);
+            return () => myRef.current.removeEventListener('mousemove', handleMove);
+          });
+          return <div ref={myRef} />;
+        }
+      `,
+      errors: [
+        `The ref value 'myRef.current' will likely have changed by the time ` +
+          `this effect cleanup function runs. If this ref points to a node ` +
+          `rendered by React, copy 'myRef.current' to a variable inside the effect, ` +
+          `and use that variable in the cleanup function.`,
+      ],
+    },
     {
       code: `
         function useMyThing(myRef) {
@@ -4457,6 +4487,31 @@ const tests = {
           'Learn more about data fetching with Hooks: https://fb.me/react-hooks-data-fetching',
       ],
     },
+    {
+      code: `
+        function Thing() {
+          useEffect(async () => {});
+        }
+      `,
+      output: `
+        function Thing() {
+          useEffect(async () => {});
+        }
+      `,
+      errors: [
+        `Effect callbacks are synchronous to prevent race conditions. ` +
+          `Put the async function inside:\n\n` +
+          'useEffect(() => {\n' +
+          '  async function fetchData() {\n' +
+          '    // You can await here\n' +
+          '    const response = await MyAPI.getData(someId);\n' +
+          '    // ...\n' +
+          '  }\n' +
+          '  fetchData();\n' +
+          `}, [someId]); // Or [] if effect doesn't need props or state\n\n` +
+          'Learn more about data fetching with Hooks: https://fb.me/react-hooks-data-fetching',
+      ],
+    },
     {
       code: `
         function Example() {

diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
@@ -88,7 +88,7 @@ export default {
       // So no need to check for dependency inclusion.
       const depsIndex = callbackIndex + 1;
       const declaredDependenciesNode = node.parent.arguments[depsIndex];
-      if (!declaredDependenciesNode) {
+      if (!declaredDependenciesNode && !isEffect) {
         // These are only used for optimization.
         if (
           reactiveHookName === 'useMemo' ||
@@ -468,6 +468,9 @@ export default {
         },
       );
 
+      if (!declaredDependenciesNode) {
+        return;
+      }
       const declaredDependencies = [];
       const externalDependencies = new Set();
       if (declaredDependenciesNode.type !== 'ArrayExpression') {