[ESLint] Treat functions that don't capture anything as static

[gaearon]

The exact heuristics will take me some time to figure out. This is just the first minimal draft.

It lifts the restriction on specifying functions as deps where it's safe. In particular, a function like

  function tick() {
    setCount(c => c + 1);
  }

would be considered safe because it doesn't capture any render-phase values except knowingly static ones (we only do this if setCount comes from useState Hook).

So we could omit it:

  useEffect(() => {
    let id = setInterval(() => {
      tick();
    }, 1000);
    return () => clearInterval(id);
  }, []); // No warning

But if we change tick to be

  function tick() {
    setCount(count + 1);
  }

it would warn, asking you to add tick to dependencies.

We only do this check one level deep. So it helps for helper functions that only set state or dispatch, but you might end up in a situation where you can't add a prop to one of them without triggering warnings for all effects using it. Ideally we might want to push you to useReducer a bit sooner but I haven't figured out what a good flow is.

This PR makes the rule strictly more relaxed. It doesn't add any new behaviors, it just fires the warnings less often in cases where it's safe.

In follow-ups, we'll need to tweak the behavior and warnings to be more useful for functions. Such as we might want to detect the above special case and suggest the updater form if it would fix the warning. Similarly, we could detect always-new deps (like tick) and in that case suggest wrapping in useCallback.

[sizebot]

Details of bundled changes.

Comparing: f16442a...c30cf04

eslint-plugin-react-hooks

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
eslint-plugin-react-hooks.development.js	+5.3%	+4.5%	57.77 KB	60.84 KB	13.45 KB	14.06 KB	NODE_DEV
eslint-plugin-react-hooks.production.min.js	🔺+3.7%	🔺+3.8%	14.57 KB	15.11 KB	5.05 KB	5.24 KB	NODE_PROD
ESLintPluginReactHooks-dev.js	+5.4%	+4.7%	61.48 KB	64.79 KB	13.78 KB	14.42 KB	FB_WWW_DEV

Generated by 🚫 dangerJS

[gaearon]

Perhaps, a better alternative could be to make the autofix wrap safe-but-used-in-effects/callbacks functions into useCallback for you. That teaches you about the pattern early and works transitively as you add more functions.

The upside of that would be that if you add a prop to some deep function in the call stack, it propagates to immediate callers. So you don't have an avalanche of transitively calling effects to fix. But the downside is you don't see which effects depend on that prop transitively. However, that may be good. Because effects might get reported with coarser deps than necessary otherwise. Having useCallback at every level helps you confront which of those actually need those props.

Another risk is it could guide you further from useReducer path which solves a lot by itself. Or from effect/ref path. So we need to catch those somehow.

It's hard to say without trying.

[gaearon]

This function just moved into the closure. It's the same implementation with a slightly different argument.

[threepointone]

#nakazawayeah