feat: flag most rules when *any* - not *all* - upstreams are internal

typically still correct I think, and it significantly simplifies
handling upstream edge cases.

requires some testing in the wild.

#53
#38

Author: NickvanDyke

README.md

@@ -237,20 +237,6 @@ function Component() {
 }
 ```
 
-### `no-manage-parent`
-
-Disallow effects that only use props:
-
-```js
-function Child({ isOpen, onClose }) {
-  useEffect(() => {
-    if (!isOpen) {
-      onClose();
-    }
-  }, [isOpen, onClose]);
-}
-```
-
 ### `no-empty-effect`
 
 Disallow empty effects:
@@ -263,7 +249,7 @@ function Component() {
 
 ## 💬 Feedback
 
-The ways to (mis)use an effect in real-world code are practically endless! This plugin is not exhaustive, and minimizes false positives at the expense of occasional false negatives. If you encounter unexpected behavior or see opportunities for improvement, please open an issue. Your feedback helps improve the plugin for everyone!
+The ways to (mis)use an effect in real-world code are practically endless! This plugin is not exhaustive. If you encounter unexpected behavior or see opportunities for improvement, please open an issue. Your feedback helps improve the plugin for everyone!
 
 ## 📖 Learn More
 

src/index.js

@@ -7,7 +7,6 @@ import noInitializeState from "./rules/no-initialize-state.js";
 import noChainStateUpdates from "./rules/no-chain-state-updates.js";
 import noDerivedState from "./rules/no-derived-state.js";
 import noPassDataToParent from "./rules/no-pass-data-to-parent.js";
-import noManageParent from "./rules/no-manage-parent.js";
 import noPassRefToParent from "./rules/no-pass-ref-to-parent.js";
 import globals from "globals";
 
@@ -26,7 +25,6 @@ const plugin = {
     "no-event-handler": noEventHandler,
     "no-pass-live-state-to-parent": noPassLiveStateToParent,
     "no-pass-data-to-parent": noPassDataToParent,
-    "no-manage-parent": noManageParent,
     "no-pass-ref-to-parent": noPassRefToParent,
     "no-initialize-state": noInitializeState,
     "no-chain-state-updates": noChainStateUpdates,
@@ -69,8 +67,3 @@ Object.assign(plugin.configs, {
 });
 
 export default plugin;
-
-// Wraps `Array.every()` to return false for empty arrays.
-Array.prototype.notEmptyEvery = function (predicate) {
-  return this.length > 0 && this.every(predicate);
-};

src/rules/no-adjust-state-on-prop-change.js

@@ -1,8 +1,9 @@
-import { getCallExpr, getUpstreamRefs } from "../util/ast.js";
 import {
+  getCallExpr,
+  getDownstreamRefs,
+  getUpstreamRefs,
   getEffectDepsRefs,
   getEffectFnRefs,
-  isArgsAllLiterals,
   isImmediateCall,
   isProp,
   isStateSetter,
@@ -32,18 +33,28 @@ export default {
       const depsRefs = getEffectDepsRefs(context, node);
       if (!effectFnRefs || !depsRefs) return;
 
-      const isAllDepsProps = depsRefs
+      const isSomeDepsProps = depsRefs
         .flatMap((ref) => getUpstreamRefs(context, ref))
-        .notEmptyEvery((ref) => isProp(ref));
+        .some((ref) => isProp(ref));
 
       effectFnRefs
         .filter((ref) => isStateSetter(context, ref))
         .filter((ref) => isImmediateCall(ref.identifier))
         .forEach((ref) => {
           const callExpr = getCallExpr(ref);
 
-          // TODO: Flag non-literals too? e.g. I think this is the correct warning for https://github.com/getsentry/sentry/pull/100177/files#diff-cf3aceaba5cdab4553d92644581e23d54914923199d31807fe090e0d49b786caR97
-          if (isAllDepsProps && isArgsAllLiterals(context, callExpr)) {
+          const argsUpstreamRefs = callExpr.arguments
+            .flatMap((arg) => getDownstreamRefs(context, arg))
+            .flatMap((ref) => getUpstreamRefs(context, ref));
+          // Avoid overlap with no-derived-state
+          const isSomeArgsNotProps =
+            // TODO: literals check may be less reliable with *all* upstream refs...
+            // What if that was `getUpstreamNodes()` instead, returning AST nodes?
+            // Could get complicated though. Ideally we may restructure the rules to not need this at all?
+            argsUpstreamRefs.length === 0 || // All literals
+            argsUpstreamRefs.some((ref) => !isProp(ref));
+
+          if (isSomeDepsProps && isSomeArgsNotProps) {
             context.report({
               node: callExpr,
               messageId: "avoidAdjustingStateWhenAPropChanges",

src/rules/no-chain-state-updates.js

@@ -1,9 +1,10 @@
-import { getCallExpr, getUpstreamRefs } from "../util/ast.js";
 import {
+  getCallExpr,
+  getDownstreamRefs,
+  getUpstreamRefs,
   getEffectDepsRefs,
   getEffectFnRefs,
   hasCleanup,
-  isArgsAllLiterals,
   isImmediateCall,
   isUseState,
   isStateSetter,
@@ -33,19 +34,25 @@ export default {
       const depsRefs = getEffectDepsRefs(context, node);
       if (!effectFnRefs || !depsRefs) return;
 
-      // TODO: Should filter out setters before checking?
-      // exhaustive-deps doesn't enforce one way or the other.
-      const isAllDepsState = depsRefs
+      const isSomeDepsState = depsRefs
         .flatMap((ref) => getUpstreamRefs(context, ref))
-        .notEmptyEvery((ref) => isUseState(ref));
+        .some((ref) => isUseState(ref));
 
       effectFnRefs
         .filter((ref) => isStateSetter(context, ref))
         .filter((ref) => isImmediateCall(ref.identifier))
         .forEach((ref) => {
           const callExpr = getCallExpr(ref);
 
-          if (isAllDepsState && isArgsAllLiterals(context, callExpr)) {
+          const argsUpstreamRefs = callExpr.arguments
+            .flatMap((arg) => getDownstreamRefs(context, arg))
+            .flatMap((ref) => getUpstreamRefs(context, ref));
+          // Avoid overlap with no-derived-state
+          const isSomeArgsState = argsUpstreamRefs.some((ref) =>
+            isUseState(ref),
+          );
+
+          if (isSomeDepsState && !isSomeArgsState) {
             context.report({
               node: callExpr,
               messageId: "avoidChainingStateUpdates",

src/rules/no-derived-state.js

@@ -54,18 +54,20 @@ export default {
           const depsUpstreamRefs = depsRefs.flatMap((ref) =>
             getUpstreamRefs(context, ref),
           );
-          const isAllArgsInternal = argsUpstreamRefs.notEmptyEvery(
+          const isSomeArgsInternal = argsUpstreamRefs.some(
             (ref) => isUseState(ref) || isProp(ref),
           );
 
-          const isAllArgsInDeps = argsUpstreamRefs.notEmptyEvery((argRef) =>
-            depsUpstreamRefs.some(
-              (depRef) => argRef.resolved == depRef.resolved,
-            ),
-          );
+          const isAllArgsInDeps =
+            argsUpstreamRefs.length &&
+            argsUpstreamRefs.every((argRef) =>
+              depsUpstreamRefs.some(
+                (depRef) => argRef.resolved == depRef.resolved,
+              ),
+            );
           const isValueAlwaysInSync = isAllArgsInDeps && countCalls(ref) === 1;
 
-          if (isAllArgsInternal) {
+          if (isSomeArgsInternal) {
             context.report({
               node: callExpr,
               messageId: "avoidDerivedState",

src/rules/no-event-handler.js

@@ -40,7 +40,7 @@ export default {
           getDownstreamRefs(context, ifNode.test)
             .flatMap((ref) => getUpstreamRefs(context, ref))
             // TODO: Should flag props too, but maybe with a different message?
-            .notEmptyEvery((ref) => isUseState(ref)),
+            .some((ref) => isUseState(ref)),
         )
         .forEach((ifNode) => {
           context.report({

src/rules/no-manage-parent.js

@@ -9,6 +9,7 @@ import {
   isUseEffect,
   getUpstreamRefs,
   isImmediateCall,
+  isRefCall,
 } from "../util/ast.js";
 import { getCallExpr, getDownstreamRefs } from "../util/ast.js";
 
@@ -37,20 +38,22 @@ export default {
 
       effectFnRefs
         .filter((ref) => isPropCallback(context, ref))
+        .filter((ref) => !isRefCall(context, ref))
         .filter((ref) => isImmediateCall(ref.identifier))
         .forEach((ref) => {
           const callExpr = getCallExpr(ref);
 
-          const isAllData =
-            callExpr.arguments.length &
-            callExpr.arguments
-              .flatMap((arg) => getDownstreamRefs(context, arg))
-              .flatMap((ref) => getUpstreamRefs(context, ref))
-              .notEmptyEvery(
-                (ref) => !isUseState(ref) && !isProp(ref) && !isUseRef(ref),
-              );
+          const argsUpstreamRefs = callExpr.arguments
+            .flatMap((arg) => getDownstreamRefs(context, arg))
+            .flatMap((ref) => getUpstreamRefs(context, ref));
+          // TODO: Includes literals. I think that makes sense, but could have a better message.
+          const isSomeArgsData =
+            callExpr.arguments.length & argsUpstreamRefs.length &&
+            argsUpstreamRefs.some(
+              (ref) => !isUseState(ref) && !isProp(ref) && !isUseRef(ref),
+            );
 
-          if (isAllData) {
+          if (isSomeArgsData) {
             context.report({
               node: callExpr,
               messageId: "avoidPassingDataToParent",

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

@@ -27,6 +27,8 @@ export default {
         "Avoid passing refs to parents in an effect. Use `forwardRef` instead.",
       avoidPropCallbackInRefCallback:
         "Avoid calling props inside callbacks registered on refs in an effect. Use `forwardRef` to register the callback in the parent instead.",
+      avoidReceivingRefFromParent:
+        "Avoid receiving refs from parents to use in an effect. Use `forwardRef` instead.",
     },
   },
   create: (context) => ({
@@ -59,18 +61,31 @@ export default {
         .forEach((ref) => {
           const callExpr = getCallExpr(ref);
 
-          const passesCallbackDataToParent = callExpr.arguments
+          const passesDataToParent = callExpr.arguments
             .flatMap((arg) => getDownstreamRefs(context, arg))
             .flatMap((ref) => getUpstreamRefs(context, ref))
             .some((ref) => isPropCallback(context, ref));
 
-          if (passesCallbackDataToParent) {
+          if (passesDataToParent) {
             context.report({
-              node: getCallExpr(ref),
+              node: callExpr,
               messageId: "avoidPropCallbackInRefCallback",
             });
           }
         });
+
+      effectFnRefs
+        .filter(
+          (ref) => isPropCallback(context, ref) && isRefCall(context, ref),
+        )
+        .forEach((ref) => {
+          const callExpr = getCallExpr(ref);
+
+          context.report({
+            node: callExpr,
+            messageId: "avoidReceivingRefFromParent",
+          });
+        });
     },
   }),
 };