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

[eslint-plugin-react-hooks]: auto-fix may crash app when deps is array/object types #16006

Closed
kpaxqin opened this issue Jun 27, 2019 · 5 comments

Comments

@kpaxqin
Copy link
Contributor

kpaxqin commented Jun 27, 2019

Do you want to request a feature or report a bug?
bug
What is the current behavior?

before run lint, my code is

const App = () => {
  const [options, setOptions] = useState([]);
  const [includesA, setIncludesA] = useState(false);

  const optionCodes = options.map(({ value }) => value);

  useEffect(() => {
    if (optionCodes.includes('A')) {
      setIncludesA(true);
    }
  }, [optionCodes.join(',')]); // transform array to string for compare 

  return <div onClick={() => setOptions([{ value: 'A' }])}></div>;
};

after 'eslint --fix'

const App = () => {
  const [options, setOptions] = useState([]);
  const [includesA, setIncludesA] = useState(false);

  const optionCodes = options.map(({ value }) => value);

  useEffect(() => {
    if (optionCodes.includes('A')) {
      setIncludesA(true);
    }
  }, [optionCodes]); // optionCodes.join(',') was replaced ! 

  return <div onClick={() => setOptions([{ value: 'A' }])}></div>;
};

The original code works fine and after 'eslint --fix' it just crashed cuz optionCodes is an array created in render function, the effect runs every time and crash my app with error :

react-dom.development.js:55 Uncaught Invariant Violation: Maximum update depth exceeded.

I've also notice this rule fix will add other params used by effect function to deps automatically. Like

// original code
const {id} = props
useEffect(()=> {
  console.log(id)
},[])

// after fix
const {id} = props
useEffect(()=> {
  console.log(id)
}, [id]) // id has been add to deps

What is the expected behavior?

For 'eslint --fix', what we expect is 'try to fix lint error automatically and SAFELY', SAFELY means DO NOT change my code logic, run 'eslint --fix' should never change your design or crash your app .

It would be better to tell developers to fix the deps by lint message, not auto fix it in dangerous way.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

eslint-plugin-react-hooks@1.6.1

@kpaxqin kpaxqin changed the title [eslint-plugin-react-hooks]: rule fix may crash app when deps is array/object types [eslint-plugin-react-hooks]: auto-fix may crash app when deps is array/object types Jun 27, 2019
@gaearon
Copy link
Collaborator

gaearon commented Jun 27, 2019

Yeah. See #15204 (comment) for context. I guess we’ll have to disable autofix if running it without looking is so common. The original intention was to make it an IDE suggestion only but ESLint doesn’t offer a way to do that. If we disable it, we’ll lose manual IDE autocompletion too which sucks.

@gaearon
Copy link
Collaborator

gaearon commented Jun 27, 2019

I’ll reach out to see if ESLint would be open to add a feature like this.

@gaearon
Copy link
Collaborator

gaearon commented Aug 2, 2019

Regarding the autofix, I hope eslint/rfcs#30 will let us solve the problem without compromising on IDE suggestions.

@gaearon
Copy link
Collaborator

gaearon commented Nov 15, 2019

Let's consolidate and track this in #16313 instead.

@gaearon gaearon closed this as completed Nov 15, 2019
@gaearon
Copy link
Collaborator

gaearon commented Feb 17, 2020

This is resolved now.
#16313 (comment)

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

No branches or pull requests

2 participants