[eslint-plugin-react-hooks] warn on unexpanded object dependencies

[gnowland]

Summary

This PR expands upon the existing logic which reports functions and constructors defined inside the render function as invalid hook dependencies (#19590) to allowing a developer to receive warning about any object present in a dependencies array when an avoidObjects option is set to true in the exhaustive-deps rule.

This is necessary because current rules do not cover the case where an object that will be referentially unique on each render is passed into the component as a parameter and used as a dependency, only objects created within the component.

It has been my experience in code reviews that the referential inequality of objects causing side-effects to execute unnecessarily is a particularly common cause of unnecessary memory use, especially when combined with XHR requests (a common useEffect action). Confoundingly There is no mention of object comparison by referential equality or warning thereto in the React Handbook Hooks API Reference.

This PR aims to provide a flexible way for developers to opt-in to receiving a detailed message regarding potential unintended side effects arising from the use of an object in a dependency array, and tips to resolve the issue (by destructuring or using property accessors - not deep equality checks as Dan Abramov is on the record against them).

This is also a feature that has been requested elsewhere.

How did you test this change?

I created a derivative implementation, used it in a couple of existing React projects, and wrote tests and ran yarn test in this repo.

interface AnObj {
    prop: string;
}

export const useAddPlotData = (
  obj: AnObj,
) -> {
    useEffect(() => {
       console.log(obj)
    }, [obj]);
}

  10:5   warning  React Hook useEffect has an object in its dependency array: 'obj'. Non-primitive dependencies can result in triggering the callback unnecessarily due to referential equality comparison. Consider destructuring the object outside the useEffect call or using property accessors to refer to primitive values within the dependency array         react-hooks-unreliable-deps/reference-deps

[sizebot]

Comparing: a276638...08edbed

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	=	131.28 kB	131.28 kB	=	42.13 kB	42.13 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	136.54 kB	136.54 kB	=	43.68 kB	43.68 kB
facebook-www/ReactDOM-prod.classic.js	=	439.35 kB	439.35 kB	=	80.29 kB	80.29 kB
facebook-www/ReactDOM-prod.modern.js	=	424.64 kB	424.64 kB	=	78.13 kB	78.13 kB
facebook-www/ReactDOMForked-prod.classic.js	=	439.35 kB	439.35 kB	=	80.29 kB	80.29 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-experimental/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.production.min.js	+1.91%	25.67 kB	26.15 kB	+1.84%	8.79 kB	8.96 kB
oss-stable-semver/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.production.min.js	+1.91%	25.67 kB	26.15 kB	+1.84%	8.79 kB	8.96 kB
oss-stable/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.production.min.js	+1.91%	25.67 kB	26.15 kB	+1.84%	8.79 kB	8.96 kB
oss-experimental/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js	+1.09%	87.66 kB	88.61 kB	+0.99%	20.81 kB	21.01 kB
oss-stable-semver/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js	+1.09%	87.66 kB	88.61 kB	+0.99%	20.81 kB	21.01 kB
oss-stable/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js	+1.09%	87.66 kB	88.61 kB	+0.99%	20.81 kB	21.01 kB

Generated by 🚫 dangerJS against 08edbed

[gnowland]

Upon further testing I've found that a primitive derived from a destructuring assignment is tripping the declaredDependencyNode.type === 'Identifier' -- any AST experts that could lend their expertise as to how to accurately determine that the declaredDependencyNode.type is indeed an Object/reference type please weigh in. In the mean time I will perform further testing and attempt to resolve.

[pawojciechowski]

Thanks, @gnowland for creating this PR! Bumping this up, as I had an issue with a similar thing. When there is a spread operator used ESLint should report the error as this dependency will change on every render.

// It could be memorized object from useMemo() too
const values = { a: 1, b: 2, c: 3 }

const TestComponent = () => {
  const { a, ...rest } = values

  useEffect(() => {
    console.log(rest)
  }, [rest])

  return null
}

I've ended up adding an additional check to https://github.com/facebook/react/blob/main/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js#L1532

    if (node.name.parent.type === "RestElement") {
      return [ref, 'variable defined by a rest spread']
    }

but I would love to have the option of avoiding all the objects in hooks dependencies.

[ahoisl]

This should be a default rule. We just had a freeze in one of the main pages of our product because an object dependency in an effect triggered a (never ending) feedback loop... 😒

Thank you for creating the PR, hope it gets merged soon!

[icopp]

This is a good idea, but I found that as-is, it quickly runs into problems of mis-labeling things in a way that makes it useless in a large codebase. For example:

const [doTheThing, setDoTheThing] = useState(false);

const memoResult = useMemo(() => {
  // some result here that changes based on doTheThing
}, [doTheThing]) // this is an error

[github-actions]

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

[ahoisl]

Would still be great to have IMO...

[github-actions]

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

[github-actions]

Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you!

[ahoisl]

This is still an issue, but commenting was disallowed, so did not have an option to "bump"