[WIP] Add ESLint rule for useEffect/useCallback/useMemo Hook dependencies

[jamiebuilds]

Moving from #14048 and gaearon#3

---

#14048:

This is just initial work from @calebmer. We didn't end up dogfooding this yet but @jamiebuilds expressed interest in picking it up. For some reason it doesn't pass tests — maybe there's some bug in the internal diff or maybe some dependency upgrade broke it. The semantics are also not set in stone and will probably need changes. Original description by @calebmer:

This diff adds a lint to enforce reactive dependencies are provided in the second argument of useEffect/useCallback. This lint is currently really strict. There are a lot of valid patterns it doesn't support. Unlike many other lints you should really be disabling this one when you want to do something more interesting then what the strict rule supports.

Here's what the lint does. The lint checks that identifiers and member expressions used in a "reactive hook callback" (lint internal name only) are declared in the reactive hook's dependency list.

function MyComponent(props) {
  useEffect(() => {
    console.log(props.foo);
  }, [props.foo]);
}

The lint requires that the dependency list be an array literal (no useEffect(() => {}, dependencies)) and that the dependency list not have spreads (no useEffect(() => {}, [...dependencies])). Both of these are valid forms in some interesting cases, though, but they make static analysis harder.

One thought is that perhaps we could support spreading an array as satisfying a dependency. For example allow:

function MyComponent(props) {
  useEffect(() => {
    for (const item of props.array) {
      console.log(item);
    }
  }, [...props.array]);
}

But I have not currently implemented this.

Some other reasonable cases (IMO) this lint does not allow include:

function MyComponent(props) {
  useEffect(() => {
    console.log(props.foo);
  }, [computeCacheKey(props.foo)]);
}

and:

function MyComponent(props) {
  useEffect(() => {
    console.log(props.foo);
  }, [props.foo.id]);
}

Both these forms allow you to be a bit more precise about the memoization breaking in useEffect and other reactive hooks.

This lint also warns about extra declarations in the dependency list:

function MyComponent(props) {
  useEffect(() => {
    console.log(props.foo);
  }, [props.foo, props.bar]);
}

And warns about duplicated declared dependencies:

function MyComponent(props) {
  useEffect(() => {
    console.log(props.foo);
  }, [props.foo, props.foo]);
}

Let me know if you think these constraints are reasonable. They might be good for 70-80% of cases and then after that there is always // eslint-disable-line.

---

gaearon#3:

I'm interested in how parts of this might have been working in the past because it seemed to rely on things that ESLint does not do.

In particular, ESLint doesn't have a NodePath type like Babel so they assign node.parent as the parent node. But for some reason they don't do this everywhere. So you end up in scenarios like we had here where you don't have access to the parent node.

I was not able to find a way to get the parent node from ESLint's context.getScope().references. So I was forced to do the worst possible thing and search through the entire AST for the node we're looking for.

I've optimized this search as much as possible. It's similar to a binary search except we're working with a generic tree. The search doesn't know anything about the AST though, it just looks for anything with a { type: string, ... } and assumes that has the same basic interface as other nodes do in ESLint.

[sizebot]

Details of bundled changes.

Comparing: 8a12009...8797fa6

eslint-plugin-react-hooks

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
eslint-plugin-react-hooks.development.js	+73.9%	+65.3%	25 KB	43.48 KB	5.75 KB	9.51 KB	NODE_DEV
eslint-plugin-react-hooks.production.min.js	🔺+99.9%	🔺+86.1%	4.84 KB	9.67 KB	1.78 KB	3.32 KB	NODE_PROD
ESLintPluginReactHooks-dev.js	+74.5%	+67.6%	26.83 KB	46.81 KB	5.89 KB	9.88 KB	FB_WWW_DEV

Generated by 🚫 dangerJS

[gaearon]

What do you think about rule semantics? Would you change anything?

[sophiebits]

I'd like to add a warning if you have foo.current in a dependency list and ref={foo} in the same component. It very likely indicates code that won't do what you want and we've seen this error a few times. @jamiebuilds Any interest in adding this? Not sure if it should be this rule or a separate one.

[jamiebuilds]

It very likely indicates code that won't do what you want and we've seen this error a few times.

Can you explain what breaks?

[sophiebits]

Well – the first time you run the code it will be null, then the second time (and every time thereafter) it will be the DOM node. So you end up with this situation of the effect running in the render after the ref changes. Like

<div ref={ref} />
useEffect(() => {
  ref.current.textContent = 'hello';
}, [ref.current]);

looks like it will set that property when the div mounts, but it doesn't. Similarly, if the ref actually moves to a new node then you still end up executing the effect in the first render after that happens. I'm not aware of any valid use case for writing this.

(In contrast, you might want to lazy initialize a ref in the render phase and refer to it during commit, and I think that is OK.)

[calebmer]

Worth mentioning that I initially designed this lint rule to be pretty opinionated. That is, in the common 80% case this rule should be useful, but in 20% of exceptional cases this lint will over-fire. For instance the computeCacheKey(props.foo) case. Or conditional cases like the following @gaearon introduced to me:

useEffect(
  () => {
    if (props.updateTitle) {
      document.title = props.title;
    }
  },
  [props.updateTitle, props.updateTitle ? props.title : undefined]
);

For this effect the best possible dependency list includes the conditional. In its current form the lint rule will reject this example. Supporting this case is tricky since if you accept the above should you also reject this:

useEffect(
  () => {
    if (props.updateTitle) {
      document.title = props.title;
    }
  },
  [props.updateTitle, props.updateTitle ? undefined : props.title]
);

This example is wrong, but if you want the lint rule to reject it now you’re in the business of tracking conditionals.

I like this rule as-is on the 80/20 principle, but given its limited semantics should it really be enabled by default in the official React Hooks eslint plugin? This rule could be community managed say in eslint-plugin-react.

[sophiebits]

Yeah, I'm a bit worried about false positives here.

[jamiebuilds]

Yeah, I'm a bit worried about false positives here.

At the same time, if we make it too loose it's only going to catch "obvious" things and will let through the more subtle bugs

[gaearon]

We could also add a level to the rule. So people can choose between more strict and more relaxed.

[jamiebuilds]

It's sometimes hard to codify these things as "levels", but we can probably group together pieces of logic as options to toggle. I'll try to do that

[acdlite]

Anecdotally, I've noticed that underspecifying dependencies is alarmingly common. In many (most?) cases you're better off omitting the dependencies list entirely. Since mistakes are common, and you usually have the option remove the list, I don't mind so much if this particular rule isn't perfect.

However, if false positives are the main blocker, I propose that the rule bails out on more complex cases. If the dependencies list is anything other than a list of variables (e.g. contains conditional expressions or function calls) it should assume the user knows what they're doing.

[acdlite]

Alternatively, it could forbid complex expressions. They're probably not a good idea anyway, unless they're automatically generated by a compiler.

I would even consider going so far to forbid property access in the list and in the effect (except for methods I guess?) though I don't know if that's feasible.

[Jessidhia]

That would make it a problem to have an effect that only runs when, say, a particular prop changes. Property access is also required to access refs.

[jamiebuilds]

Might be better to bail out only when there is no list currenty there. Otherwise it should probably do its best to add more dependencies. Otherwise it may bail out when it could be most useful

[acdlite]

@Kovensky

That would make it a problem to have an effect that only runs when, say, a particular prop changes.

The idea is you should be picking off specific props anyway. [foo, bar] instead of [props.foo, props.bar].

Property access is also required to access refs.

Correct, because they are mutable. I was thinking you could make an exception for refs and forbid all other mutable access. But maybe this is too pedantic and/or complicated to implement.

[acdlite]

@jamiebuilds

Might be better to bail out only when there is no list currenty there.

Oh definitely I assumed that's what it already does

[gaearon]

I pushed some failing tests. (cc @jamiebuilds @calebmer)

I will work on them tomorrow unless somebody beats me to a fix.

[gaearon]

I'm going to take a different approach in #14636.
@jamiebuilds Thanks for helping!