fix: ignore effects with cleanup

not in every rule - just the ones that false positives have surfaced in.
may introduce rare false negatives, but this is preferrable.
#30
#36
#37

Author: NickvanDyke

src/no-chain-state-updates.js

@@ -2,6 +2,7 @@ import { getCallExpr } from "./util/ast.js";
 import {
   getEffectDepsRefs,
   getEffectFnRefs,
+  hasCleanup,
   isArgsAllLiterals,
   isImmediateCall,
   isState,
@@ -29,7 +30,10 @@ export default {
       const effectFnRefs = getEffectFnRefs(context, node);
       const depsRefs = getEffectDepsRefs(context, node);
       if (!effectFnRefs || !depsRefs) return;
+      if (hasCleanup(node)) return;
 
+      // TODO: Should filter out setters before checking?
+      // exhaustive-deps doesn't enforce one way or the other.
       const isAllDepsState = depsRefs.notEmptyEvery((ref) =>
         isState(context, ref),
       );

src/no-derived-state.js

@@ -7,6 +7,7 @@ import {
   isProp,
   getUpstreamReactVariables,
   isState,
+  hasCleanup,
 } from "./util/react.js";
 import { getCallExpr, getDownstreamRefs } from "./util/ast.js";
 
@@ -31,6 +32,7 @@ export default {
       const effectFnRefs = getEffectFnRefs(context, node);
       const depsRefs = getEffectDepsRefs(context, node);
       if (!effectFnRefs || !depsRefs) return;
+      if (hasCleanup(node)) return;
 
       effectFnRefs
         .filter((ref) => isStateSetter(context, ref))

src/no-event-handler.js

@@ -1,4 +1,8 @@
-import { getEffectFnRefs, getEffectDepsRefs } from "./util/react.js";
+import {
+  getEffectFnRefs,
+  getEffectDepsRefs,
+  hasCleanup,
+} from "./util/react.js";
 import { findDownstreamNodes, getDownstreamRefs } from "./util/ast.js";
 import { isState } from "./util/react.js";
 
@@ -23,6 +27,7 @@ export default {
       const effectFnRefs = getEffectFnRefs(context, node);
       const depsRefs = getEffectDepsRefs(context, node);
       if (!effectFnRefs || !depsRefs) return;
+      if (hasCleanup(node)) return;
 
       // TODO: Can we also flag this when the deps are internal, and the body calls internal stuff?
       // That'd overlap with other rules though... maybe just useRefs?

src/no-pass-data-to-parent.js

@@ -5,6 +5,7 @@ import {
   isState,
   isRef,
   isProp,
+  hasCleanup,
 } from "./util/react.js";
 import { getCallExpr, getDownstreamRefs } from "./util/ast.js";
 
@@ -29,10 +30,12 @@ export default {
       const effectFnRefs = getEffectFnRefs(context, node);
       const depsRefs = getEffectDepsRefs(context, node);
       if (!effectFnRefs || !depsRefs) return;
+      if (hasCleanup(node)) return;
 
       effectFnRefs
         .filter((ref) => isPropCallback(context, ref))
-        // We don't check `isDirectCall` because it shouldn't matter - passing data to the parent is passing data to the parent.
+        // We don't check `isDirectCall` because it shouldn't matter; passing data to the parent is passing data to the parent.
+        // And misuses are often indirect, e.g. retrieving and passing up external data in a Promise chain.
         .forEach((ref) => {
           const callExpr = getCallExpr(ref);
 
@@ -43,6 +46,8 @@ export default {
                 (ref) =>
                   !isState(context, ref) &&
                   !isProp(context, ref) &&
+                  // TODO: Should warn to use `forwardRef` instead?
+                  // https://github.com/NickvanDyke/eslint-plugin-react-you-might-not-need-an-effect/issues/22
                   !isRef(context, ref),
               )
           ) {

src/util/react.js

@@ -48,6 +48,22 @@ export const isUseState = (node) =>
     return !el || el.type === "Identifier";
   });
 
+// While it *could* be an anti-pattern or unnecessary, effects *are* meant to synchronize systems.
+// So we presume that a "subscription effect" is usually valid, or at least may be more readable.
+// TODO: We might be able to use this more granularly, e.g. ignore state setters inside a subscription effect,
+// instead of ignoring the whole effect...? But it'd have to be more complicated, like also ignore the same state setters called in the body.
+export const hasCleanup = (node) => {
+  const effectFn = node.arguments[0];
+  return (
+    (effectFn.type === "ArrowFunctionExpression" ||
+      effectFn.type === "FunctionExpression") &&
+    effectFn.body.type === "BlockStatement" &&
+    effectFn.body.body.some(
+      (stmt) => stmt.type === "ReturnStatement" && stmt.argument,
+    )
+  );
+};
+
 export const isPropDef = (def) => {
   const declaringNode =
     def.node.type === "ArrowFunctionExpression"

test/no-adjust-state-on-prop-change.test.js

@@ -24,7 +24,7 @@ new MyRuleTester().run("no-adjust-state-on-prop-change", rule, {
         function Counter() {
           const [count, setCount] = useState(0);
           const [otherState, setOtherState] = useState();
-          
+
           useEffect(() => {
             setOtherState('Hello World');
           }, [count]);
@@ -36,7 +36,7 @@ new MyRuleTester().run("no-adjust-state-on-prop-change", rule, {
       code: js`
         function Counter({ count }) {
           const [doubleCount, setDoubleCount] = useState(0);
-          
+
           useEffect(() => {
             setDoubleCount(count * 2);
           }, [count]);

test/no-chain-state-updates.test.js

@@ -41,6 +41,19 @@ new MyRuleTester().run("no-chain-state-updates", rule, {
         }
       `,
     },
+    {
+      name: "Synchronize internal state with literal",
+      code: js`
+        function Component() {
+          const [isMounted, setIsMounted] = useState(false);
+
+          useEffect(() => {
+            setIsMounted(true);
+            return () => setIsMounted(false);
+          }, [setIsMounted]);
+        }
+      `,
+    },
   ],
   invalid: [
     {
@@ -49,7 +62,7 @@ new MyRuleTester().run("no-chain-state-updates", rule, {
         function Counter() {
           const [count, setCount] = useState(0);
           const [otherState, setOtherState] = useState();
-          
+
           useEffect(() => {
             setOtherState('Hello World');
           }, [count]);

test/no-derived-state.test.js

@@ -103,6 +103,25 @@ new MyRuleTester().run("no-derived-state", rule, {
         }
       `,
     },
+    {
+      // https://github.com/NickvanDyke/eslint-plugin-react-you-might-not-need-an-effect/issues/35
+      // While it *could* be an anti-pattern or unnecessary, effects *are* meant to synchronize systems.
+      // So we guess that a "subscription effect" is usually valid, or may be more readable.
+      name: "Synchronize internal state",
+      code: js`
+        function Component() {
+          const [name, setName] = useState();
+          const [model] = useState(
+            () => new FormModel(props)
+          );
+
+          useEffect(() => {
+            model.setFieldDescriptor(name);
+            return () => model.removeField(name);
+          }, [model, name]);
+        }
+      `,
+    },
     {
       name: "Subscribe to external state",
       code: js`
@@ -390,53 +409,6 @@ new MyRuleTester().run("no-derived-state", rule, {
         }
       `,
     },
-    {
-      // https://github.com/NickvanDyke/eslint-plugin-react-you-might-not-need-an-effect/issues/35
-      // TODO: Finally time to not consider state MemberExpressions as state setters
-      todo: true,
-      name: "Calling method on state",
-      code: js`
-        function Component() {
-          const [name, setName] = useState();
-          const [model] = useState(
-            () => new FormModel(props)
-          );
-
-          // Register field within the model
-          useEffect(() => {
-            model.setFieldDescriptor(name);
-          }, [model, name]);
-        }
-      `,
-    },
-    {
-      // https://github.com/NickvanDyke/eslint-plugin-react-you-might-not-need-an-effect/issues/35
-      name: "Cleanup function calls method on state",
-      code: js`
-        function Component() {
-          const [name, setName] = useState();
-          const [model] = useState(
-            () => new FormModel(props)
-          );
-
-          useEffect(() => {
-            return () => model.removeField(name);
-          }, [model, name]);
-        }
-      `,
-    },
-    {
-      // TODO: Double-check whether this is valid; compared to calling a method on state
-      name: "Cleanup function sets state",
-      code: js`
-        function Component() {
-          const [isMounted, setIsMounted] = useState(true);
-          useEffect(() => {
-            return () => setIsMounted(false);
-          }, []);
-        }
-      `,
-    },
   ],
   invalid: [
     {
@@ -518,7 +490,7 @@ new MyRuleTester().run("no-derived-state", rule, {
       ],
     },
     {
-      name: "From derived prop",
+      name: "From intermediate prop",
       code: js`
         function Form({ firstName, lastName }) {
           const [fullName, setFullName] = useState('');
@@ -537,7 +509,7 @@ new MyRuleTester().run("no-derived-state", rule, {
       ],
     },
     {
-      name: "From props via member function",
+      name: "From props via method",
       code: js`
         function DoubleList({ list }) {
           const [doubleList, setDoubleList] = useState([]);
@@ -555,8 +527,7 @@ new MyRuleTester().run("no-derived-state", rule, {
       ],
     },
     {
-      name: "From internal state via member function",
-      todo: true,
+      name: "From internal state via method",
       code: js`
         function DoubleList() {
           const [list, setList] = useState([]);
@@ -572,6 +543,12 @@ new MyRuleTester().run("no-derived-state", rule, {
           messageId: "avoidDerivedState",
           data: { state: "doubleList" },
         },
+        // TODO: Kinda confusing to double-flag... ideally we'd ignore the `concat` call, given it's a setter arg.
+        // Or even a different error message/rule for `no-mutate-state`...? If we can find a good heuristic.
+        {
+          messageId: "avoidDerivedState",
+          data: { state: "list" },
+        },
       ],
     },
     {

test/no-pass-data-to-parent.test.js

@@ -135,6 +135,37 @@ new MyRuleTester().run("no-pass-data-to-parent", rule, {
         }
       `,
     },
+    {
+      // https://github.com/NickvanDyke/eslint-plugin-react-you-might-not-need-an-effect/issues/37
+      // Alternate solutions exist, but this is arguably the most readable.
+      // TODO: There is probably a more granular opportunity here...
+      name: "Pass cleanup function that depends on ref",
+      code: js`
+        import { dropTargetForElements } from "@atlaskit/pragmatic-drag-and-drop/element/adapter";
+
+        function DeleteDropTarget({ onDelete }) {
+          const ref = React.useRef(null);
+
+          React.useEffect(() => {
+            const element = ref.current;
+            if (!element) {
+              return;
+            }
+
+            const cleanup = dropTargetForElements({
+              element,
+              onDrop: ({ source }) => {
+                onDelete(source.data);
+              },
+            });
+
+            return cleanup;
+          }, [onDelete]);
+
+          return <div ref={ref}>Drop an item here to delete</div>;
+        };
+      `,
+    },
   ],
   invalid: [
     {
@@ -230,6 +261,9 @@ new MyRuleTester().run("no-pass-data-to-parent", rule, {
       ],
     },
     {
+      // TODO: This could be done (possibly conditionally) in the parent because it doesn't depend on anything in the child?
+      // May fit better as a new, more general rule.
+      todo: true,
       name: "Pass window event data to parent",
       code: js`
         const Child = ({ onResized }) => {

test/real-world.test.js

@@ -301,6 +301,37 @@ describe("recommended rules on real-world code", () => {
           }
         `,
       },
+      {
+        // https://github.com/NickvanDyke/eslint-plugin-react-you-might-not-need-an-effect/issues/30
+        name: "Ref callback",
+        code: js`
+          export const useOnScreen = () => {
+              const [element, setElement] = useState(null);
+              const [isIntersecting, setIntersecting] = useState(false);
+
+              const ref = useCallback((element) => {
+                  setElement(element);
+              }, []);
+
+              useEffect(() => {
+                  if (!element) {
+                      return;
+                  }
+
+                  const observer = new IntersectionObserver(([entry]) => {
+                      setIntersecting(entry?.isIntersecting ?? false);
+                  });
+
+                  observer.observe(element);
+                  return () => {
+                      observer.disconnect();
+                  };
+              }, [element]);
+
+              return { ref, isIntersecting };
+          };
+        `,
+      },
     ].forEach(({ name, code }) => {
       it(name, async () => {
         const results = await eslint.lintText(code);