From 54dbfb9d028623d6dfb209fa3d3faf2507ecff96 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Tue, 15 Sep 2026 07:50:24 -0400 Subject: [PATCH 1/4] Add restart-key and effect-protocol rules to `perf-hooks-effects`, and drop a stale-closure example --- .../repos/metamask-extension.md | 122 ++++++++++++++++-- 1 file changed, 110 insertions(+), 12 deletions(-) diff --git a/domains/performance/skills/perf-hooks-effects/repos/metamask-extension.md b/domains/performance/skills/perf-hooks-effects/repos/metamask-extension.md index 349d9527..e97fc908 100644 --- a/domains/performance/skills/perf-hooks-effects/repos/metamask-extension.md +++ b/domains/performance/skills/perf-hooks-effects/repos/metamask-extension.md @@ -344,7 +344,7 @@ const useHistoricalPrices = () => { **DO:** - Split effects when conditional logic excludes some dependencies -- Ensure all dependencies in array are actually used +- Ensure every dependency is either read by the effect or is a named restart key (see Rule: Name Restart Keys) **DON'T:** @@ -374,19 +374,47 @@ const useHistoricalPrices = ({ isEvm, chainId, address }: Props) => { fetchPrices(chainId, address); } }, [isEvm, chainId, address]); // All deps are used +}; +``` - // OR Option 2: Separate effects - useEffect(() => { - if (isEvm) return; - fetchPrices(chainId, address); - }, [isEvm]); // Only depends on condition +### Rule: Name Restart Keys - useEffect(() => { - if (!isEvm) { - fetchPrices(chainId, address); - } - }, [chainId, address]); // Only when not EVM -}; +**DO:** + +- When an effect synchronizes with an external system (a connection, a subscription, a background registration) and a dependency's only job is to tear that down and start it again when it changes, keep it in the array and say so in a comment beside it +- Prefer passing the value into the call the effect makes, so the effect reads it and the dependency explains itself + +**DON'T:** + +- Leave a dependency the effect body never reads without a comment. A reviewer cannot tell a restart key from a dead dependency, and the next edit deletes it +- Delete a restart key to satisfy "include only what the effect reads". The effect then keeps a session bound to the old value + +**Example - WRONG:** + +```typescript +useEffect(() => { + const session = startSession(address); + return () => session.stop(); +}, [address, networkId]); // networkId is never read: restart key, or leftover? +``` + +**Example - CORRECT:** + +```typescript +useEffect(() => { + const session = startSession(address); + return () => session.stop(); +}, [ + address, + // Not read above: a network change must end this session and start a new one. + networkId, +]); + +// Or make the dependency real by passing it in: +useEffect(() => { + const session = startSession({ address, networkId }); + return () => session.stop(); +}, [address, networkId]); ``` ### Rule: Use useRef for Persistent Values @@ -856,6 +884,76 @@ const PriceTicker = ({ tokenAddress }: PriceTickerProps) => { }; ``` +### Rule: Keep Multi-Step Async Protocols Out of the Effect + +**DO:** + +- When an effect acquires an external resource through more than one async step, with a timeout or more than one way to end, move that protocol into a plain object owned outside React: `start()` returns a handle, and `handle.stop()` releases the resource +- Keep the effect to acquiring and releasing: start in the body, stop in the cleanup +- Unit-test the object's end paths directly (ready, timeout, failure, stop while a step is in flight), without rendering + +**DON'T:** + +- Grow a state machine of closure flags (`cancelled`, `ended`, `ready`) inside the effect. Every end path needs its own cleanup, every fix has to reason about every flag at once, and the only way to test it is through `renderHook` + +**Why:** every line of such an effect can pass the rules above (stable dependencies, timers cleared, a cancelled flag on the async chain) while the bugs sit between its end paths: a timeout that fires mid-acquire, a release sent before registration, a release that also runs on the success path. Those bugs belong to the protocol, not to any one line, so a line-level review does not catch them. + +**Example - WRONG:** + +```typescript +useEffect(() => { + let cancelled = false; + let ended = false; + let ready = false; + const finish = (reason: string) => { + if (!ended) { + ended = true; + report(reason); + } + }; + const timeout = setTimeout(() => { + cancelled = true; + finish('timeout'); + release(id); + }, 30_000); + register(id) + .then(() => !cancelled && init(address)) + .then(() => !cancelled && start(id)) + .then(() => { + if (!cancelled) { + ready = true; + finish('ready'); + } + }) + .catch(() => { + finish('failed'); + if (!cancelled) release(id); + }) + .finally(() => clearTimeout(timeout)); + return () => { + cancelled = true; + clearTimeout(timeout); + finish('released'); + release(id, ready); + }; +}, [address]); +``` + +**Example - CORRECT:** + +```typescript +// preload-session.ts: a plain object, unit-tested without React +export function startPreloadSession(address: string): { stop: () => void } { + // register, init, start, the timeout, and every end path live here +} + +// The hook only acquires and releases +useEffect(() => { + const session = startPreloadSession(address); + return () => session.stop(); +}, [address]); +``` + ### Rule: Avoid Large Object Retention in Closures **DO:** From 6f107c0a2df5e01b7672bf9b5b7a7941355b5e76 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Wed, 16 Sep 2026 10:38:05 -0400 Subject: [PATCH 2/4] Require `useEffect` to read every dependency instead of commenting restart keys --- .../repos/metamask-extension.md | 60 ++++++++++++------- 1 file changed, 37 insertions(+), 23 deletions(-) diff --git a/domains/performance/skills/perf-hooks-effects/repos/metamask-extension.md b/domains/performance/skills/perf-hooks-effects/repos/metamask-extension.md index e97fc908..88ce95b6 100644 --- a/domains/performance/skills/perf-hooks-effects/repos/metamask-extension.md +++ b/domains/performance/skills/perf-hooks-effects/repos/metamask-extension.md @@ -344,7 +344,7 @@ const useHistoricalPrices = () => { **DO:** - Split effects when conditional logic excludes some dependencies -- Ensure every dependency is either read by the effect or is a named restart key (see Rule: Name Restart Keys) +- Ensure the effect reads every dependency (see Rule: Don't List Dependencies the Effect Doesn't Read) **DON'T:** @@ -377,44 +377,58 @@ const useHistoricalPrices = ({ isEvm, chainId, address }: Props) => { }; ``` -### Rule: Name Restart Keys +### Rule: Don't List Dependencies the Effect Doesn't Read + +The dependency array describes the effect's code. A value that must restart the effect when it changes has to be a value the effect uses. **DO:** -- When an effect synchronizes with an external system (a connection, a subscription, a background registration) and a dependency's only job is to tear that down and start it again when it changes, keep it in the array and say so in a comment beside it -- Prefer passing the value into the call the effect makes, so the effect reads it and the dependency explains itself +- Treat a dependency the effect never reads as a finding, even when the effect is meant to restart when it changes +- If the effect's work depends on the value, pass the value into that work: the call that starts the connection, subscription or background registration +- If the value marks a scope whose state must all reset when it changes, render that scope as a component and give it the value as its `key` +- If the value changes because of a specific action, do the work where that action is handled instead of in an effect that watches for the change **DON'T:** -- Leave a dependency the effect body never reads without a comment. A reviewer cannot tell a restart key from a dead dependency, and the next edit deletes it -- Delete a restart key to satisfy "include only what the effect reads". The effect then keeps a session bound to the old value +- Delete the dependency without one of the changes above. The effect then keeps a session bound to the old value +- Keep the unread dependency and explain it in a comment. React Compiler's effect-dependency validation (`validateExhaustiveEffectDependencies`, lint rule `react-hooks/exhaustive-effect-dependencies`, off by default) reports it as unnecessary, and with that validation on, the compiler leaves the whole component or hook uncompiled, or fails the build under a strict `panicThreshold` +- Add `'use no memo'` to protect the dependency array. It opts the whole function out of React Compiler, and React documents it as a temporary debugging tool +- Add a parameter the called code ignores just so the effect reads the value. That moves the unread value into the call + +**Reference:** [Removing Effect Dependencies](https://react.dev/learn/removing-effect-dependencies), [Resetting all state when a prop changes](https://react.dev/learn/you-might-not-need-an-effect#resetting-all-state-when-a-prop-changes), [`"use no memo"`](https://react.dev/reference/react-compiler/directives/use-no-memo) **Example - WRONG:** ```typescript -useEffect(() => { - const session = startSession(address); - return () => session.stop(); -}, [address, networkId]); // networkId is never read: restart key, or leftover? +const useSession = ({ address, networkId }: Props) => { + useEffect(() => { + const session = startSession(address); + return () => session.stop(); + }, [address, networkId]); // networkId is never read +}; ``` **Example - CORRECT:** ```typescript -useEffect(() => { - const session = startSession(address); - return () => session.stop(); -}, [ - address, - // Not read above: a network change must end this session and start a new one. - networkId, -]); +// The session is for one network, so the effect passes the network in +const useSession = ({ address, networkId }: Props) => { + useEffect(() => { + const session = startSession({ address, networkId }); + return () => session.stop(); + }, [address, networkId]); +}; -// Or make the dependency real by passing it in: -useEffect(() => { - const session = startSession({ address, networkId }); - return () => session.stop(); -}, [address, networkId]); +// Caches must be cleared when the account or network changes: +// a keyed child unmounts on the change, and its cleanup clears them +const CacheScope = () => { + useEffect(() => () => clearCaches(), []); + return null; +}; + +const Root = ({ address, networkId }: Props) => ( + +); ``` ### Rule: Use useRef for Persistent Values From 5e4191bcadc0bbac0826d5173f5f60823d9cfc20 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Wed, 16 Sep 2026 11:23:49 -0400 Subject: [PATCH 3/4] Minimize `useEffect` dependencies by reading primitives, not by dropping a value the effect uses --- .../repos/metamask-extension.md | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/domains/performance/skills/perf-hooks-effects/repos/metamask-extension.md b/domains/performance/skills/perf-hooks-effects/repos/metamask-extension.md index 88ce95b6..828937f1 100644 --- a/domains/performance/skills/perf-hooks-effects/repos/metamask-extension.md +++ b/domains/performance/skills/perf-hooks-effects/repos/metamask-extension.md @@ -48,37 +48,39 @@ const TokenDisplay = ({ token }: TokenDisplayProps) => { **DO:** -- Reduce dependencies by moving values to default parameters when possible -- Only include dependencies that actually trigger the effect +- Depend on the primitive the effect reads (an ID, an address, a number) rather than an object rebuilt on each render +- Keep values the effect does not use out of the effect **DON'T:** - Include unnecessary dependencies that cause effects to run too often +- Drop a value the effect reads to make it run less often. A default parameter does not stop it being a dependency (see Rule: Include All Dependencies in useEffect) **Example - WRONG:** ```typescript -const TokenBalance = ({ address, network, refreshInterval }: Props) => { +const TokenBalance = ({ account, network, refreshInterval }: Props) => { const [balance, setBalance] = useState('0'); useEffect(() => { const fetch = async () => { - const result = await fetchBalance(address, network); + const result = await fetchBalance(account.address, network); setBalance(result); }; fetch(); const interval = setInterval(fetch, refreshInterval); return () => clearInterval(interval); - }, [address, network, refreshInterval]); // Effect runs too often + }, [account, network, refreshInterval]); // A parent passing account={{ address }} rebuilds it on every render, so the effect refetches and resets the interval each time }; ``` **Example - CORRECT:** ```typescript -const TokenBalance = ({ address, network, refreshInterval = 10000 }: Props) => { +const TokenBalance = ({ account, network, refreshInterval }: Props) => { const [balance, setBalance] = useState('0'); + const { address } = account; useEffect(() => { const fetch = async () => { @@ -89,7 +91,7 @@ const TokenBalance = ({ address, network, refreshInterval = 10000 }: Props) => { fetch(); const interval = setInterval(fetch, refreshInterval); return () => clearInterval(interval); - }, [address, network]); // refreshInterval moved to default param + }, [address, network, refreshInterval]); // The address string is equal across renders }; ``` From 47ec68405b95abe03683534d155770c728e88916 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 17 Sep 2026 19:16:28 -0400 Subject: [PATCH 4/4] Make the unread-dependency rule reachable from the React Compiler side It was stated only as an effects rule, and the cost it names is a compiler cost: an entry the effect never reads makes the compiler skip the whole function, so the hook ships unmemoized. Someone asking what loses them compiler coverage opens the compiler overlay, where the rule was absent. Zero hits for it across all four react-compiler surfaces before this. --- .../repos/metamask-extension.md | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/domains/performance/skills/perf-react-compiler/repos/metamask-extension.md b/domains/performance/skills/perf-react-compiler/repos/metamask-extension.md index 4ef9d41b..363f5549 100644 --- a/domains/performance/skills/perf-react-compiler/repos/metamask-extension.md +++ b/domains/performance/skills/perf-react-compiler/repos/metamask-extension.md @@ -694,6 +694,46 @@ const TokenList = ({ tokens, filter }: TokenListProps) => { }; ``` +### ❌ Anti-Pattern: Dependencies the Effect Never Reads + +**Problem:** An entry the effect does not read is not a harmless extra. The compiler's +effect-dependency validation treats it as an error and **skips the whole function**, so the +hook ships with no memoization at all. A comment next to the array does not help: the check +reads the array, not the prose. + +**Solution:** Make the value one the effect actually reads, by passing it into the work the +effect starts. Where it marks a scope that must reset as a whole, put a `key` on a component +instead. Never satisfy it with a comment, with `'use no memo'`, or with a parameter the callee +ignores. + +**Example - WRONG:** + +```typescript +useEffect(() => { + const session = manager.startPreload({ address, accountChanged }); + return () => session.stop(); +}, [ + address, + // Provider or network changes release the old preload and register a new one. + provider, + isTestnet, +]); +``` + +**Example - CORRECT:** + +```typescript +useEffect(() => { + // `provider` and `isTestnet` are now read, so the array describes the code. + const session = manager.startPreload({ address, accountChanged, provider, isTestnet }); + return () => session.stop(); +}, [address, provider, isTestnet]); +``` + +**Why this lives here as well as in `perf-hooks-effects`:** the rule is stated there as an +effects rule, and the cost is a React Compiler cost. Someone asking what loses them compiler +coverage opens this file, so the rule has to be reachable from this side too. + ### ❌ Anti-Pattern: Using Index as Key for Dynamic Lists **Problem:** Breaks React's reconciliation when lists can be reordered, filtered, or have items added/removed.