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] Constructions should be caught in default props/args #20248

Open
nstepien opened this issue Nov 13, 2020 · 3 comments

Comments

@nstepien
Copy link

React version: 17.0.1

Steps To Reproduce

  1. function useMyHook1(arr = []) {
      useEffect(() => {
        console.log(arr);
      }, [arr]);
    }
    
     function useMyHook2({ arr = [] }) {
      useEffect(() => {
        console.log(arr);
      }, [arr]);
    }
    
    function MyComponent({ arr = [] }) {
      useEffect(() => {
        console.log(arr);
      }, [arr]);
    
      return null;
    }

Link to code example: --

The current behavior

No warning appears for arr being potentially constructed on every render.

The expected behavior

Should warn that arr can be constructed on each render.
The lint rules already warn for the following, so this is just an extension of the existing behavior:

const arr = props.arr ?? []; // The 'arr' logical expression could make the dependencies of useEffect Hook (at line 54) change on every render. Move it inside the useEffect callback. Alternatively, wrap the initialization of 'arr' in its own useMemo() Hook.

Reference: #19590

@nstepien nstepien added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Nov 13, 2020
@eps1lon eps1lon added Component: ESLint Rules Type: Enhancement and removed Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug labels Nov 13, 2020
@mdaj06
Copy link
Contributor

mdaj06 commented Dec 15, 2020

Can I have a go at this @eps1lon

@eps1lon
Copy link
Collaborator

eps1lon commented Dec 15, 2020

I can't guarantee whether this is accepted. I'm just triaging issues. But considering similar efforts were included (#19590) there's a good chance it will.

So feel free to work on it 👍 If you're stuck at any point you can share you progress and ask for help.

@nstepien
Copy link
Author

@mdaj06 Have you had a chance to look into it?

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

No branches or pull requests

3 participants