[Feature request] Exhaustive dependencies checks for ReasonReact hooks

[dodomorandi]

Greetings everyone! I asked in ReasonML forum if anyone was aware of a way to check for exhaustive deps checks when using ReasonReact hooks. I really think that this issue is a great source of bugs in React, but in JS world there are tons of things like this, and obviously the ecosystem tools are lifesavers.

In ReasonML most of the source of bugs are handled at compile-time, but unfortunately this is not the case with exhaustive deps. I really believe that there should be a way to express React hooks using an abstraction layer that could detect these kind of issue at compile-time (maybe using ppx?), but I still don't know the language enough to write this sort of implementation (again, if it is possible).

I think that implementing an exhaustive deps check in reanalyze would help a lot -- it took me hours to find one missing dependency in one hook for a toy project. Do you think that it would be something doable?

[cristianoc]

@dodomorandi would you show one or two examples to illustrate what you mean by exhaustive dep check?

[dodomorandi]

Sure, and sorry that I assumed you were familiar with the problem!

Take the following simple example:

[@react.component]
let make = (text: string) => {
  let (actualText, setActualText) = React.useState(() => text);
  let onClick =
    React.useCallback(_ => setActualText(old => old ++ " " ++ text));

  <button onClick> {ReasonReact.string(actualText)} </button>;
};

This example contains an issue: when the text property of the component is changed from a parent component, onClick is still memoized with the old text variable. The following is what should have been used instead:

let onClick =
  React.useCallback2(
    _ => setActualText(old => old ++ " " ++ text),
    (text, setActualText),
  );

AFAIK it should be fine without passing setActualText in the tuple, but you get the idea.

This issue is obviously related to how React works, and when working in JS, eslint with the react hooks plugin is a must have. As you can imagine, it is extremely easy to introduce this sort of bugs, especially when refactoring code. And I would really like to avoid this family of problems with a strong type-based abstraction, but I don't even know if it is possible. Maybe, at least at the current state of the ecosystem, reanalyze could be the best way to handle the problem.

EDIT I forgot to show you the related issue in the React repo

[cristianoc]

@dodomorandi not sure what is the proposed rule that this violates. I see how that was not intentional in the example, but I'm not sure what is the general rule that the example violates: a rule that fires on mistakes and does not fire on intentional use.

[dodomorandi]

I think that the rule should be something like the following: given a set of hook functions with the first argument as a callable object and the second argument representing a tuple of variables, whenever the callable object is an inline function, all the variables captured by the function must be in the tuple of variables.

This is the basic rule AFAIK, and I don't know how this could be hard to implement. For instance, is it possible to analyze the AST in order to check if a variable is defined in scope or it is captured from the parent scope?

I don't know if it could be useful to check the code of the eslint plugin, maybe it could be possible to see how they defined the rules.

Feel free to continue asking, I am glad to help as best as I can.

[hoichi]

@cristianoc The corresponding eslint rule is simple: if something from the component/custom hook scope is used in a hook callback, that hook should receive it in its dependencies list. Sure, Kent C. Dodds, for one, recommends setting that rule to a warning, yet he argues that you should almost always adhere to that rule:

If it really will never change, then there's no harm in including it anyway. Also, if you think it will never change and it does, including it will help you avoid bugs.

There are a lot of other situations which are more nasty and harder to identify/explain (like, if you skip adding a function to the dependencies list you could be calling a stale closure). Just trust me, every time I thought "oh, I don't need to follow the rule this time" I later regretted disabling it because I was wrong.

He mostly talks about the useEffect hook though, not about memoization. But since in modern React functions, thanks to closures, are part of the data flow, and memorizing them and failing to update on dependency change could lead to those same bugs, so, like @dodomorandi says, the rule should probably affect all the hooks with dependency arrays/tuples: If a value from the component/hook scope is used by the hook, it should be in its dependencies.

[cristianoc]

Thanks for providing all the pointers.
I have to confess that I still don’t understand it enough, in the following sense. I’m wondering what’s really causing the issue here. Whether at the API level, where hooks pretend to be functions, but really are objects disguised a little bit to look like function (creation, internal state, staleness, life cycles, etc). Or perhaps deeper at the language level, and this is just one common pattern, encouraged by react, giving rise to the issue (but the issue predates the pattern).

[dodomorandi]

As far as I can tell, hooks are real functions, it is the way they are implemented that brings to this particular React coding pattern. I never dug into the React codebase, but I can imagine something like this:

// Reset to 0 for each render
let useEffectCounter = 0;
let useEffectHooksData = [];

export function useEffect(fn, observables) {
  const hookData = useEffectHooksData[useEffectCounter];
  if (hookData === undefined) {
    useEffectHooksData[useEffectCounter] = deepCopy(observables);
    fn();
    addRenderOnChange(hookData);
  } else {
    let changed = hookData.length !== observables.length;
    if (!changed) {
      for (let index = 0; index < hookData.length; ++index) {
        if (!deepIsEqual(hookData[index], observables[index])) {
          changed = true;
          break;
        }
      }
    }

    if (changed) {
      removeRenderOnChange(hookData);
      useEffectHooksData[useEffectCounter] = deepCopy(observables);
      fn();
      addRenderOnChange(hookData);
    }
  }

  ++useEffectCounter;
}

You can see that the memoization behavior could be implemented with something like this. Conceptually is something simple, but at the same time you always want to re-run fn whenever a captured variable is changed. Unfortunately, as you can imagine, this is a pretty bad combination of an error prone design in React (but it is not totally its fault) and the inability of choosing which variables are captured in JS.

I hope I helped a little bit in order to clarify the current state. I would really like to create a type-safe abstraction to avoid all this mess, but I still doubt it is possible... 😞

[hoichi]

Disclaimer: I’m no guru. But as for whether it's an old pattern vs something React-specific, in my understanding, it’s a bit of both.

On one hand, the JS closures already allow us to have a form of explicit state (and the stale closure problems), so even if JS didn’t have objects, methods, and context binding, functions disguised as objects are very much part of the language. Moreover, one of the React design goals is to reuse JS and JS semantics as much as possible. For instance, they never had any DSL for branching/looping in their templates, it was always just plain JS like ternaries or someArray.map.

On the other hand, what React encourages is a state that is local to components. For comparison, Elm, which is both a purely functional language and a Model-View-Update framework, there’s a single tree representing all the app state, and every time you update something in a sub-sub-sub-reducer, you end up immutably creating a new copy of that tree. React neither requires nor advises keeping UI state (like whether you have a dropdown open) in a global store, this belongs in the local (component) state. But that means that React runtime is keeping tabs on what state belongs to what component. So, this is already not ‘just JS’, and maybe the old this.setState(_ => ({ foo: 1})) conveys the notion of statefulness more naturally than the new let (foo, setFoo) = useState(0); setFoo(_ => 1);.

Now to the hooks with dependencies. You could probably say it’s not even a React-specific thing: it's specific to React hooks. That said, for the most part, the hooks that need dependencies use the pre-existing memoization pattern, if with a twist: even useCallback, that is supposed to memoize functions and not just values, can be used to encapsulate values from the component’s scope, and indeed, the example from the docs doesn’t seem to memoize a pure function:

const memoizedCallback = useCallback(
  () => {
    doSomething(a, b);
  },
  [a, b],
);

(This, I believe, is because passing pure functions—or at least the same pure function—to child components is rather useless: if a function doesn’t use any data from the parent’s scope, why create it in that scope at all?)

In a way, that’s very functional, or, at least, immutable: new (callback) reference === new data, etc. But by using data from the component scope, it opens the door for the old stale closure problem. Hence the need for dependency diffing. Because most of the hooks exist to go against the flow of the component function: the function is run anew on every component rerender, but the hooks are there specifically to remember and return the values from some previous render. With useState/useReducer/useRef, we have explicit ways to update those values imperatively, but useCallback and useMemo have a more declarative way of deciding whether to update, namely, comparing the dependencies.

Some notes:

1. useEffect doesn’t memorize, its goal is to sync state/props to the outer world, e.g., send network requests or update some global DOM. Yet it does use the same diffing mechanism to allow some control over what exactly to sync.
2. By ’diffing’ I mean diffing the dependency array, shallowly. If one of the dependencies is an object, React won’t diff the objects, it will compare the references. Immutable semantics is part of React’s DNA 😄
3. Custom hooks, composed of the standard hooks, potentially could have dependencies too, and ideally, a linter/analyzer should check those as well. Not sure though if it’s possible to tell whether a parameter of a random custom hook is a list of dependencies, or even whether a random function is a hook.

Hope this overlong comment makes the matter somewhat clearer, not the other way 😊

[tsnobip]

might be interesting to follow what is being done by https://github.com/reason-in-barcelona/react-rules-of-hooks-ppx