[eslint-plugin-react-hooks] Report constant constructions

[captbaritone]

Summary

This PR expands upon the existing logic which will report functions defined inside the render function as invalid hook dependencies.

We can now detect additional types of expressions which will result in values that will be referentially unique on each render, such as [] or {}.

function MyComponent() {
  const foo = ['a', 'b', c']; // <== This array is reconstructed each render
  const normalizedFoo = useMemo(() => foo.map(expensiveMapper), [foo]);
  return <OtherComponent foo={normalizedFoo} />
}

We also detect cases where an expression may conditionally result in a referentially unique on each render, such as someArr ?? [] or someObj || {}.

function MyComponent({ nullableValue }) {
  const foo = nullableValue ?? ['a', 'b', c']; // <== This array is _potentially_ reconstructed each render
  const normalizedFoo = useMemo(() => foo.map(expensiveMapper), [foo]);
  return <OtherComponent foo={normalizedFoo} />
}

Test Plan

yarn jest

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 31f75cd:

Sandbox	Source
React	Configuration

[captbaritone]

For even better developer experience, we could try to blame the actual sub-expression that is a construction.

For example:

const foo = bar ?? [];
                   ^^ This is actually to blame

However, it would add some additional complexity.

[captbaritone]

FYI @gaearon who suggested we add this here rather than as a separate rule.

cc @dylanOshima who paired with me on the original, standalone, version of this rule.

[sizebot]

Details of bundled changes.

Comparing: c8d9b88...31f75cd

eslint-plugin-react-hooks

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
eslint-plugin-react-hooks.development.js	+3.2%	+3.5%	84.38 KB	87.1 KB	19.36 KB	20.03 KB	NODE_DEV
eslint-plugin-react-hooks.production.min.js	🔺+4.8%	🔺+4.7%	23.31 KB	24.43 KB	7.89 KB	8.26 KB	NODE_PROD

Size changes (stable)

Generated by 🚫 dangerJS against 31f75cd

[sizebot]

Details of bundled changes.

Comparing: c8d9b88...31f75cd

eslint-plugin-react-hooks

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
eslint-plugin-react-hooks.development.js	+3.2%	+3.5%	84.4 KB	87.11 KB	19.37 KB	20.04 KB	NODE_DEV
eslint-plugin-react-hooks.production.min.js	🔺+4.8%	🔺+4.7%	23.33 KB	24.44 KB	7.9 KB	8.27 KB	NODE_PROD

Size changes (experimental)

Generated by 🚫 dangerJS against 31f75cd

[captbaritone]

The ExhaustiveDeps test was segfaulting on my work machine unless I omitted most of the tests, so I was only running my new tests (using only: true). They seem to run fine on my personal computer, so I'll fix up failing tests now.

[captbaritone]

I got this working on VSCode with some actual production code and looked at a few examples. Some thoughts:

Suggestions are helpful but clunky

If I have a conditional array construction in my render function the flow looks like this:

1. Start state:

• const foo = bar ?? [];

2. See the error and apply the automatic suggestion to wrap in useMemo:

• const foo = useMemo(() => {return bar ?? [];});

3. Realize that useMemo is not imported. Manually import that (auto import screwed up due to required/import migration)

• import {useMemo} from 'React';

4. Lint complains about missing dependency array, but does not suggest a fix. Manually add an empty one:

• const foo = useMemo(() => {return bar ?? [];}, []);

5. Lint informs me I'm missing a dependency bar and offers an automatic fix, which I take:

• const foo = useMemo(() => {return bar ?? [];}, [bar]);

Overall this is clunky, but at least I get an error at each point and the error tells me what to do.

Should our suggested fix include an empty dependency array? That changes the semantics, but gets the user closer to a fix, assuming they listen to other lint advice.

Wrapping only the declaration may cause bugs if the value gets mutated

Consider this case:

function Greeting({name}) {
  const myArr = ['Hello'];
  if (name) {
    myArr.push(name);
  }
  const greeting = useMemo(() => myArr.join(' '), [myArr]);
  return greeting;
}

Currently we offer an automatic suggestion to wrap the declaration in useMemo, but that would cause a bug:

function Greeting({name}) {
  const myArr = useMemo(() => ['Hello'];
  if (name) { // <== ERROR: `name` could change and `greeting` would not update.
    myArr.push(name);
  }
  const greeting = useMemo(() => myArr.join(' '), [myArr]);
  return greeting;
}

Maybe we need to check for the case where the value gets mutated and not offer an automated fix in that case

[gaearon]

Should our suggested fix include an empty dependency array?

Can we suggest a filled-in array in the first place?

[gaearon]

Currently we offer an automatic suggestion to wrap the declaration in useMemo, but that would cause a bug:

Yeah — this is really, really bad.

How do you feel about starting without an autofix at all? We can maybe add it later if it's too burdensome.

[captbaritone]

How do you feel about starting without an autofix at all? We can maybe add it later if it's too burdensome.

Sounds good. Should we leave the autofix for functions on the assumption that it's not very common for folks to mutate functions?

[captbaritone]

Can we suggest a filled-in array in the first place?

I'm sure we could, I haven't looked hard enough to know exactly how much work it would be. I saw this comment (maybe you wrote it?) and just assumed it would be more work than was worth it for now.

// TODO: ideally we'd gather deps here but it would require
// restructuring the rule code. This will cause a new lint
// error to appear immediately for useCallback. Note we're
// not adding [] because would that changes semantics.

[gaearon]

Should we leave the autofix for functions on the assumption that it's not very common for folks to mutate functions?

Yeah I think that's reasonable to keep.

[gaearon]

Let's remove the risky useMemo wrapping for new cases

[captbaritone]

@gaearon Actually, I'm trying to think how hard it would be to detect cases where the value might have been mutated. Would it be sufficient to identify cases where the object has one of its properties read/written/called? If it's that simple, and if the ESLint scope API can give us that info easily (I'm still new to that API and it's poorly documented) then maybe it would be possible to allow the fix in the common case where the value is not mutated.

[gaearon]

One way to tell is to go through references and see what its usage is like. It might work. But keep in mind it might be tricky to determine. Like if an array is used with arr.map() for rendering, we wouldn't know with a simple heuristic if this mutates or not.

[captbaritone]

Updated:

• Only offer this autofix for functions -- which are unlikely to get mutated.
• Update the message to clarify that the entire construction of the value should get wrapped not just the declaration.

[captbaritone]

I'll punt on trying to detect potential ussages for now.

[gaearon]

Gonna review this tomorrow morning.

[bradzacher]

Other options instead of construction:

• instantiation
• initialisation (add a z if you want to americanise it 😉)
• assignment

[bradzacher]

nit - could pull this up to L857 where you declare wrapperHook

[captbaritone]

Since this is only used within the actual fix, it seemed reasonable to have it here.

[bradzacher]

I believe that you could also add support for JSX here, because under the hood they are going to create a new object ref each render (right? I don't know the exact semantics of JSX transforms)

const frag = <></>; // JSXFragment
const comp = <div />; // JSXElement
useMemo(() => {}, [frag, comp]);

[bradzacher]

could also add handling for AssignmentExpression:

let x = null;
let y = x = () => {};
useMemo(() => {}, [y]);

(i.e. ignore the node and check the right)

[bradzacher]

could also add handling for NewExpression?

const x = new Map();
useMemo(() => {}, [x]);

[bradzacher]

could also add handling for Literal when value instanceof RegExp?

const x = /a/;
useMemo(() => {}, [x]);

[bradzacher]

could also handle TS type assertions (TSAsExpression):

const x = {} as any;
useMemo(() => {}, [x]);

(i.e. ignore the node and check the expression.)

[bradzacher]

could also handle flow type casts (TypeCastExpression):

const x = ({}: any);
useMemo(() => {}, [x]);

(i.e. ignore the node and check the expression.)

[captbaritone]

This list is gold. Thanks! I had intentionally omitted NewExpression thinking that String Number and Boolean would be compared by value, but I forgot that they will be boxed so presumably they will be referentially unique and therefore worth flagging.

[bradzacher]

Yeah I didn't think about that, but testing in chrome shows they are referentially unique

Note that they're also not even loosely equal:

(note: if you swap one side for a literal, it coerces the object to the same type and then they become loosely equal)

[gaearon]

This is such an edge case btw. I don't think handling boxed primitives is super important.

[captbaritone]

@gaearon I would agree that it's very unlikely to happen, we get it for free when handling other NewExpressions like new Map([]).

[gaearon]

You'll want to add some tests for cases where the same variable is used by multiple useMemo's.

[captbaritone]

@gaearon I've added a test for a constant construction used by multiple dependency arrays here: 31f75cd

Note that it creates two different errors at the same location. One referencing the first dependency array, and one referencing the other.

[gaearon]

For patterns where an object doesn't use any values from the pure scopes, would it be possible to change the message to suggest to move it outside the component scope? This is a lot less noisy and seems fairly common in practice.

[captbaritone]

For patterns where an object doesn't use any values from the pure scopes, would it be possible to change the message to suggest to move it outside the component scope? This is a lot less noisy and seems fairly common in practice.

Good suggestion, but I think it would require significantly more analysis. Currently we don't do any work to try to understand the values used to construct the object. If we did, not only could we adjust the message as you suggest, but we could also construct the dependency array for the autofix of adding a wrapping useMemo. Given the amount of work involved, I think it makes sense to leave this for another task.

[nstepien]

Will it also handle cases like default props? e.g.:

function Foo({ arr = [] }) {
  useMemo(() => {
    console.log(arr);
  }, [arr]);
}

[captbaritone]

@nstepien Good question. It doesn't currently. It looks like it wouldn't be too hard to add though. Here's a commit that adds it: 29d560b

I'm happy to open that as a PR, but I suspect we would want to come up with a somewhat different error message in that case, and I'm not sure I have the time right now to work on that (maybe you do?).

Thoughts @gaearon? Do you agree that we would want to different error message for the default param case?

[gaearon]

That would need a different message, yes.

[nstepien]

@captbaritone Checking for default props is still missing, do you think you could work on it? I can log an issue otherwise.
It should also check for default params in custom hooks, i.e.:

function useArr(arr = []) {
  useEffect(() => {
    console.log(arr);
  }, [arr]);
}

// or

function useArr({ arr = []} ) {
  useEffect(() => {
    console.log(arr);
  }, [arr]);
}