Separate rules

#15

Author: NickvanDyke

README.md

@@ -48,23 +48,23 @@ export default [
 
 The plugin will have more information to act upon when you pass the correct dependencies to your effect — [`react-hooks/exhaustive-deps`](https://www.npmjs.com/package/eslint-plugin-react-hooks).
 
-## 🔎 Rule: `you-might-not-need-an-effect`
+## 🔎 Rules
 
-Determines when an effect is likely unnecessary, such as when it:
+**Legend:**
+- 🟡 = Enabled as a warning in the recommended config
+- 🔴 = Enabled as an error in the recommended config
+- ⚪ = Not enabled by default
 
-- Stores derived state
-- Chains state updates
-- Initializes state
-- Resets all state when props change
-- Couples parent and child state or behavior
+| Rule | Description | Documentation | Default |
+|------|-------------|---------------|---------|
+| `no-derived-state` | Disallow storing derived state in an effect. | [docs](https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) | 🟡 |
+| `no-chain-state-updates` | Disallow chaining state changes in an effect. | [docs](https://react.dev/learn/you-might-not-need-an-effect#chains-of-computations) | 🟡 |
+| `no-initialize-state` | Disallow initializing state in an effect. | — | 🟡 |
+| `no-reset-all-state-when-a-prop-changes` | Disallow resetting all state in an effect when a prop changes. | [docs](https://react.dev/learn/you-might-not-need-an-effect#resetting-all-state-when-a-prop-changes) | 🟡 |
+| `no-event-handler` | Disallow using state and an effect as an event handler. | [docs](https://react.dev/learn/you-might-not-need-an-effect#sharing-logic-between-event-handlers) | 🟡 |
+| `no-parent-child-coupling` | Disallow coupling parent behavior or state to a child component in an effect. | [docs](https://react.dev/learn/you-might-not-need-an-effect#notifying-parent-components-about-state-changes) | 🟡 |
+| `no-empty-effect` | Disallow empty effects. | — | 🟡 |
 
-When possible, also suggests the more idiomatic pattern.
-
-While the effect may be unnecessary, we cannot reliably determine that when it:
-
-- Uses external state
-- Calls external functions
-- Uses internal state to handle events
 
 ## ⚠️ Limitations
 
@@ -77,3 +77,4 @@ This plugin aims to minimize false positives and accepts that some false negativ
 - https://react.dev/learn/synchronizing-with-effects
 - https://react.dev/learn/separating-events-from-effects
 - https://react.dev/learn/lifecycle-of-reactive-effects
+

src/index.js

@@ -1,15 +1,42 @@
-import { name as ruleName, rule } from "./rule.js";
+import * as noEmptyEffect from "./no-empty-effect.js";
+import * as noResetAllStateWhenAPropChanges from "./no-reset-all-state-when-a-prop-changes.js";
+import * as noEventHandler from "./no-event-handler.js";
+import * as noParentChildCoupling from "./no-parent-child-coupling.js";
+import * as noInitializeState from "./no-initialize-state.js";
+import * as noChainStateUpdates from "./no-chain-state-updates.js";
+import * as noDerivedState from "./no-derived-state.js";
 import globals from "globals";
 
-const pluginName = "react-you-might-not-need-an-effect";
-
 const plugin = {
   meta: {
-    name: pluginName,
+    name: "react-you-might-not-need-an-effect",
   },
   configs: {},
   rules: {
-    [ruleName]: rule,
+    [noEmptyEffect.name]: noEmptyEffect.rule,
+    [noResetAllStateWhenAPropChanges.name]:
+      noResetAllStateWhenAPropChanges.rule,
+    [noEventHandler.name]: noEventHandler.rule,
+    [noParentChildCoupling.name]: noParentChildCoupling.rule,
+    [noInitializeState.name]: noInitializeState.rule,
+    [noChainStateUpdates.name]: noChainStateUpdates.rule,
+    [noDerivedState.name]: noDerivedState.rule,
+  },
+};
+
+const recommendedRules = Object.keys(plugin.rules).reduce((acc, ruleName) => {
+  acc[plugin.meta.name + "/" + ruleName] = "warn";
+  return acc;
+}, {});
+const languageOptions = {
+  globals: {
+    // NOTE: Required so we can resolve global references to their upstream global variables
+    ...globals.browser,
+  },
+  parserOptions: {
+    ecmaFeatures: {
+      jsx: true,
+    },
   },
 };
 
@@ -19,38 +46,15 @@ Object.assign(plugin.configs, {
     files: ["**/*.{js,jsx,mjs,cjs,ts,tsx,mts,cts}"],
     plugins: {
       // Object.assign above so we can reference `plugin` here
-      [pluginName]: plugin,
-    },
-    rules: {
-      [pluginName + "/" + ruleName]: "warn",
-    },
-    languageOptions: {
-      globals: {
-        // NOTE: Required so we can resolve global references to their upstream global variables
-        ...globals.browser,
-      },
-      parserOptions: {
-        ecmaFeatures: {
-          jsx: true,
-        },
-      },
+      [plugin.meta.name]: plugin,
     },
+    rules: recommendedRules,
+    languageOptions,
   },
-  // eslintrc format
   "legacy-recommended": {
-    plugins: [pluginName],
-    rules: {
-      [pluginName + "/" + ruleName]: "warn",
-    },
-    globals: {
-      // NOTE: Required so we can resolve global references to their upstream global variables
-      ...globals.browser,
-    },
-    parserOptions: {
-      ecmaFeatures: {
-        jsx: true,
-      },
-    },
+    plugins: [plugin.meta.name],
+    rules: recommendedRules,
+    ...languageOptions,
   },
 });
 

src/messages.js

@@ -0,0 +1,76 @@
+import { getCallExpr, getDownstreamRefs } from "./util/ast.js";
+import {
+  getDependenciesRefs,
+  getEffectFnRefs,
+  getUpstreamReactVariables,
+  isDirectCall,
+  isFnRef,
+  isHOCProp,
+  isProp,
+  isState,
+  isStateSetter,
+  isUseEffect,
+} from "./util/react.js";
+
+export const name = "no-chain-state-updates";
+export const messages = {
+  avoidChainingStateUpdates: "avoidChainingStateUpdates",
+};
+export const rule = {
+  meta: {
+    type: "suggestion",
+    docs: {
+      description: "Disallow chaining state changes in an effect.",
+      url: "https://react.dev/learn/you-might-not-need-an-effect#chains-of-computations",
+    },
+    schema: [],
+    messages: {
+      [messages.avoidChainingStateUpdates]:
+        "Avoid chaining state changes. When possible, update all relevant state simultaneously.",
+    },
+  },
+  create: (context) => ({
+    CallExpression: (node) => {
+      if (!isUseEffect(node)) return;
+      const effectFnRefs = getEffectFnRefs(context, node);
+      const depsRefs = getDependenciesRefs(context, node);
+      if (!effectFnRefs || !depsRefs) return;
+
+      const isAllDepsInternal = depsRefs
+        .flatMap((ref) => getUpstreamReactVariables(context, ref.identifier))
+        .notEmptyEvery(
+          (variable) =>
+            isState(variable) || (isProp(variable) && !isHOCProp(variable)),
+        );
+
+      effectFnRefs
+        .filter(isFnRef)
+        .filter((ref) => isDirectCall(ref.identifier))
+        .filter((ref) => isStateSetter(context, ref))
+        .forEach((ref) => {
+          const callExpr = getCallExpr(ref);
+          const argsRefs = callExpr.arguments.flatMap((arg) =>
+            getDownstreamRefs(context, arg),
+          );
+          const argsUpstreamVariables = argsRefs.flatMap((ref) =>
+            getUpstreamReactVariables(context, ref.identifier),
+          );
+          const isAllArgsInternal = argsUpstreamVariables.notEmptyEvery(
+            (variable) =>
+              isState(variable) || (isProp(variable) && !isHOCProp(variable)),
+          );
+          const isSomeArgsExternal = argsUpstreamVariables.some(
+            (variable) =>
+              (!isState(variable) && !isProp(variable)) || isHOCProp(variable),
+          );
+
+          if (!isAllArgsInternal && !isSomeArgsExternal && isAllDepsInternal) {
+            context.report({
+              node: callExpr,
+              messageId: messages.avoidChainingStateUpdates,
+            });
+          }
+        });
+    },
+  }),
+};

src/no-chain-state-updates.js

@@ -0,0 +1,89 @@
+import {
+  isUseEffect,
+  getEffectFnRefs,
+  getDependenciesRefs,
+  isFnRef,
+  isDirectCall,
+  isStateSetter,
+  getUseStateNode,
+  isProp,
+  isHOCProp,
+  getUpstreamReactVariables,
+  isState,
+} from "./util/react.js";
+import { getCallExpr, getDownstreamRefs } from "./util/ast.js";
+import { arraysEqual } from "./util/javascript.js";
+
+export const name = "no-derived-state";
+export const messages = {
+  avoidDerivedState: "avoidDerivedState",
+};
+export const rule = {
+  meta: {
+    type: "suggestion",
+    docs: {
+      description: "Disallow storing derived state in an effect.",
+      url: "https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state",
+    },
+    schema: [],
+    messages: {
+      [messages.avoidDerivedState]:
+        'Avoid storing derived state. Compute "{{state}}" directly during render, optionally with `useMemo` if it\'s expensive.',
+    },
+  },
+  create: (context) => ({
+    CallExpression: (node) => {
+      if (!isUseEffect(node)) return;
+      const effectFnRefs = getEffectFnRefs(context, node);
+      const depsRefs = getDependenciesRefs(context, node);
+      if (!effectFnRefs || !depsRefs) return;
+
+      effectFnRefs
+        .filter(isFnRef)
+        .filter((ref) => isDirectCall(ref.identifier))
+        .filter((ref) => isStateSetter(context, ref))
+        .forEach((ref) => {
+          const callExpr = getCallExpr(ref);
+          const useStateNode = getUseStateNode(context, ref);
+          const stateName = (
+            useStateNode.id.elements[0] ?? useStateNode.id.elements[1]
+          )?.name;
+          const argsRefs = callExpr.arguments.flatMap((arg) =>
+            getDownstreamRefs(context, arg),
+          );
+          const argsUpstreamVariables = argsRefs.flatMap((ref) =>
+            getUpstreamReactVariables(context, ref.identifier),
+          );
+          const isAllArgsInternal = argsUpstreamVariables.notEmptyEvery(
+            (variable) =>
+              isState(variable) || (isProp(variable) && !isHOCProp(variable)),
+          );
+          const isAllArgsInDeps = argsRefs
+            .filter((ref) =>
+              ref.resolved.defs.every((def) => def.type !== "Parameter"),
+            )
+            .notEmptyEvery((argRef) =>
+              depsRefs.some((depRef) =>
+                arraysEqual(
+                  getUpstreamReactVariables(context, argRef.identifier),
+                  getUpstreamReactVariables(context, depRef.identifier),
+                ),
+              ),
+            );
+          const isStateSetterCalledOnce =
+            ref.resolved.references.length - 1 === 1;
+
+          if (
+            isAllArgsInternal ||
+            (isAllArgsInDeps && isStateSetterCalledOnce)
+          ) {
+            context.report({
+              node: callExpr,
+              messageId: messages.avoidDerivedState,
+              data: { state: stateName },
+            });
+          }
+        });
+    },
+  }),
+};

src/no-derived-state.js

@@ -0,0 +1,33 @@
+import { isUseEffect, getEffectFnRefs } from "./util/react.js";
+
+export const name = "no-empty-effect";
+export const messages = {
+  avoidEmptyEffect: "avoidEmptyEffect",
+};
+export const rule = {
+  meta: {
+    type: "suggestion",
+    docs: {
+      description: "Disallow empty React effects.",
+    },
+    schema: [],
+    messages: {
+      [messages.avoidEmptyEffect]: "This effect is empty and could be removed.",
+    },
+  },
+  create: (context) => ({
+    CallExpression: (node) => {
+      if (!isUseEffect(node)) return;
+      const effectFnRefs = getEffectFnRefs(context, node);
+
+      if (effectFnRefs?.length === 0) {
+        // Hopefully it's obvious the effect can be removed.
+        // More a follow-up for once they fix/remove other issues.
+        context.report({
+          node,
+          messageId: messages.avoidEmptyEffect,
+        });
+      }
+    },
+  }),
+};