react icon indicating copy to clipboard operation
react copied to clipboard

Feature Request (react-hooks/exhaustive-deps): `ignoreObjectReferences` flag

Open Newbie012 opened this issue 2 years ago • 2 comments

React version: 18.2.0

Steps To Reproduce

  1. Create a component that uses useDisclosure() from a 3rd party library to manage a modal's visibility status.
  2. Inside the component, add a useEffect hook that depends on a onOpen method of the disclosure object returned from useDisclosure().
  3. ESLint raises a warning suggesting to replace disclousure.onOpen with disclosure in the dependency array of the useEffect() hook.
import React from 'react';
import { useDisclosure } from 'library-a';
import { useQuery } from 'library-b';

function MyComponent() {
    const disclosure = useDisclosure();
    const query = useQuery({ queryKey: ["..."], queryFn: () => Promise.resolve(1) });

    React.useEffect(() => {
        if (!query.isSuccess) return;
        disclosure.onOpen();
    }, [disclosure.onOpen, query.isSuccess]);
     // ^^^^^^^^^^^^^^^^^ ESLint will suggest changing 'disclosure.onOpen' to 'disclosure'
 
    ...
}

The current behavior

ESLint's react-hooks/exhaustive-deps rule suggests changing the dependency disclosure.onOpen to disclosure. This happens because the ESLint plugin checks for dependencies at the reference level and does not go deeper into object properties.

As a workaround, one alternative is to destructure the disclosure object to acquire onOpen:

const { onOpen } = useDisclosure();

And then use onOpen directly in the useEffect dependencies. But this approach has limitations:

  1. It can raise uncertainty when there are multiple hooks like useDisclosure because it's not instantly clear from which object the destructured property originated.
  2. In TypeScript, additional verbosity is introduced because you will need to specify the type of the destructured property, effectively writing the properties twice.

Another approach would be to write disclosure.onOpen.call(null), but it's really awkward.

The expected behavior

It would be beneficial if ESLint could recognize that the dependency on dislcosure.onOpen does not necessitate a dependency on disclosure in instances where object properties are stable across renders or are reliably memoized.

I propose a feature where a new setting, such as ignoreObjectReferences, could be introduced in the "react-hooks/exhaustive-deps" rule configuration. When ignoreObjectReferences is true, ESLint should treat the properties and methods of an object as individual dependencies, ignoring the reference of the object itself. This would instead allow us to just include dislosure.onOpen in our dependency arrays, thus circumventing unnecessary rerenders caused by the change of the object reference.

I'll be happy to raise a PR for this.

Newbie012 avatar Jul 27 '23 16:07 Newbie012

I agree that this needs to be revisited.

clicktodev avatar Mar 11 '24 09:03 clicktodev

Related #16265

clicktodev avatar Mar 11 '24 09:03 clicktodev

This is because when you call disclosure.onOpen() you're passing the this value of disclosure to onOpen, which can be stale. Since linters can't do full program analysis to confirm if disclosure reads this, we can't detect whether it's safe. De-structuring will not pass the this context to onOpen and that guarantees you won't read a stale closure.

rickhanlonii avatar Mar 24 '24 15:03 rickhanlonii