Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: eslint-plugin-react-hooks: Incorrect dependency requirement when using typeof on nested data structures #27335

Open
jwueller opened this issue Sep 5, 2023 · 7 comments

Comments

@jwueller
Copy link

jwueller commented Sep 5, 2023

This issue seems related to #18828, but it's not identical. Using typeof on a primitive value seems to work as expected, but it fails for nested property access.

Version: [email protected]

Steps To Reproduce

import { useEffect, useState } from "react";

const Dummy = () => {
    const [foo, setFoo] = useState<{ bar: number }>({ bar: 42 });

    useEffect(() => {
        const square = (x: typeof foo.bar) => x * x;
        setFoo((previous) => ({ ...previous, bar: square(previous.bar) }));
    }, []);
};

The current behavior

This effect clearly doesn't have external (value) dependencies, but I get this:

ESLint: React Hook useEffect has a missing dependency: 'foo'. Either include it or remove the dependency array. (react-hooks/exhaustive-deps)

The expected behavior

Using typeof never leads to a dependency requirement, just like in this example doesn't:

import { useEffect, useState } from "react";

const Dummy = () => {
    const [bar, setBar] = useState<number>(42);

    useEffect(() => {
        const square = (x: typeof bar) => x * x;
        setBar(square);
    }, []);
};
@jwueller jwueller added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Sep 5, 2023
@sophiebits sophiebits added Type: Bug Component: ESLint Rules and removed Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug labels Oct 19, 2023
@sophiebits
Copy link
Collaborator

Perhaps the fix in #19316 wasn't quite right and needs to skip if TSTypeAnnotation is anywhere in the ancestors.

@Shubhdeep12
Copy link

Yes, correct @sophiebits the type checking should not be only just for the single parent of dependencynode
can I give this a shot? Or you are working on this already?

@sophiebits
Copy link
Collaborator

Go for it!

@Shubhdeep12
Copy link

Great! Thanks

@Shubhdeep12
Copy link

Hi @sophiebits actually need some help.
I have the first draft of my solution ready but I am a little bit confused on how to test it.

like how to use my local react repo as a dependency in my local project to test out the changes.

it would be really helpful if you could guide me on this.

@Shubhdeep12
Copy link

Figured out. :)

@Shubhdeep12
Copy link

Raised the PR @sophiebits and its ready for review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants