-
Notifications
You must be signed in to change notification settings - Fork 46.4k
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]react-hooks/exhaustive-deps rule autofix modifies code function, violating eslint best practices #16313
Comments
Yep. We’re hoping eslint/rfcs#30 will let us resolve this in a nice way. |
I realized today that the linter does not warn about local functions anymore. Is it related to this issue? Is it possible that the warning is disabled right now? |
auto fix is really annoying.sometimes I didn't need the dependencies。 |
This is usually a mistake and a misunderstanding.
As I said before, eslint/rfcs#30 will allow us to make autofix manual-only, but it's not ready yet. |
@gaearon Hi Dan, here is something really similar in react-native world: function App({ navigation }) {
React.useEffect(() => {
navigation.setParams({ myParam: 'test' });
// it will add navigation here which will cause infinity loop
}, []);
return (
...
)
} Here is the link to the reproducing example. |
@terrysahaidak That can be fixed on the navigation library side since there's no way for React to tell whether a value is static or dynamic. |
There's movement on eslint/eslint#12384, which should let us solve this. Let's keep an eye on it. |
Completely agree that an eslint fix should NEVER change logic. This can very easily introduce infinite loops, which in my case, surfaced only after a git hook ran eslint fix as part of the commit process. I.E. working code was tested locally, and but unknowingly broke after committing (due to this rule adding missing conditions), which is super easy to miss. @gaearon That linked eslint PR has been accepted, but is it directly related to this problem? I'm guessing you mean that this react lint flag should be changed to add a "suggestion" rather than make the code change as part of an eslint fix? |
@peter-wan-intuit what was basically happened to me on react native - it kept adding navigation prop to useEffect where I been setting a param on it which may change navigation ref link (but not the setParam method link), so it caused infinity loop. |
Yes. That's what #17385 does. |
Just checking in on this - if #17385 gets held up for a while waiting on vscode-eslint, are there any options right now to disable autofixing for this rule besides turning the rule off? I checked around in eslint docs and I don't see a way to just turn off autofix for one rule other than disabling the rule completely. Appreciate any pointers. |
I don't think there's any way, no. |
I just got hit by this today after enabling the recommended settings for this plugin. This is bad news guys. I had to spend 20 mins debugging my code only to figure out that the linter had auto-fixed my [] to [props]. Please do not include an auto fix for this. You will break others like you did me. Just because it’s not a common use case doesn’t mean people aren’t using it. |
@nashspence Sorry to hear that. To be clear, we understand the issue — it's what this thread is about. Once microsoft/vscode-eslint#814 ships, we will merge and release the update that uses the Suggestions API instead. |
@gaearon Cool cool. No worries man. I just wanted to whine a bit. Great job on everything else and whatnot. Honestly, I’m all good with the ignore on the crucial line at this point. |
I hope the mistake to autofix will be resolved soon, since I have the rule turned off completely. It was a bad idea from the beginning and I did not understand why people were pushing it so hard even when it was breaking the "not change functionality of code" rule. |
I have just wasted hours of time because the exhaustive-deps rule thought that a certain dependency was not a dependency, and the fix removed it. I didn't know that this rule had a fix that would change the execution of the code. ESLint fixes should never do that! So when I see a fix happen on save when I'm writing code, I don't anticipate that it'll change the execution of my code. As a result, I was getting very strange behaviour which took me hours to debug, partly because I thought the problem was in a different file (where I was doing most of the work; I've now needlessly re-written it 3 times), and partly because I thought that I must be misunderstanding useMemo and useCallback. But really the issue was not mine, it was that I think it is incredibly poor practice not to remove the fix! I appreciate that you wanted to use a new ESLint feature to offer users the option to fix something that might change the code execution, but in the meantime you should have just switched it off. See eslint/eslint#10242 for a similar problem when The React team is usually really good about not breaking code when making changes. You have really well-documented and thought out upgrade paths, deprecations, documentation, etc., but in this case you've failed spectacularly on that front. |
I understand your frustration. But adding more to the thread when we're already aware of the problem, have a solution in testing, and asked to refrain from commenting unless you have something new to add, isn't helpful. I'm locking this thread because it's creating notification spam for people subscribed to it who would like to know about the solution rather than more "+1" reports. I should add that I agree with you in principle. However, there is an argument that Suggestions API wouldn't have made it to the RFC stage and past it in the first place if this practical example (which a large part of the motivation for that RFC) wasn't so painful. So yes, it's unfortunate, but also — we're going to have a solution to this, and this temporary pain was a part of it. |
I published If it doesn't work, please comment on #17385. |
To restore the IDE functionality, you'll need to have |
why code below throw an eslint@7 message:
it should work, anything wrong? eslint@5.16.0 throw nothing |
The autofix was disabled in For Suggestions API yes, you'll need to wait for CRA to catch up. They're doing a release soon. |
const { onSectionDragEnd } = props
const onSectionDragEnd = useCallback(() => {
onSectionDragEnd && onSectionDragEnd();
}, [onSectionDragEnd]); Did you have a chance to read the message? It tells you to do exactly that:
|
@gaearon certainly i did, i just wonder why, it should work and there's no logical mistake - although it could be fixed by a
|
Hi |
Not if you’re using version 2.4 of the plugin or more recent. |
Well, according to |
I'm also seeing autofixing for this rule. My dependencies:
My (simplified) config file: {
"extends": ["plugin:react/recommended"],
"plugins": ["react", "react-hooks"],
"rules": {
"react-hooks/exhaustive-deps": "warn"
}
}
|
I can't help without a reproducing project. |
@gaearon I created a project from-scratch with eslint, React, and the eslint-plugin-react-hooks in VSCode and was unable to reproduce the autofixing issue. The autofixing must be caused by another tool in my specific project, sorry for the noise. |
@skitterm did you find out what tool is autofixing your code? I have similar issue, eslint is autofixing the dependency in the useEffect. |
@sedghi I did not 😞 |
@skitterm |
@skitterm |
I have the exact opposite issue. Somehow I broke the autofixing and it is driving me crazy 😂 |
This is giving me headaches with useEffect making endless re-renders as auto-fix is updating unwanted dependencies on save making it hard to debug especially if you have more than one Use effect in one file. Can this auto-fix be disabled by default and just show warnings? This will give developers flexibility to decide on the actionable. |
Do you want to request a feature or report a bug?
Bug
What is the current behavior?
Consider:
auto fix will automatically change this to:
Here we have a simple data fetch component that should refetch data when parameters change, but not when the fetch function changes. Whether this is right or not, it is legitimate code, that the lint fix will cause serious problems in the code if it is used like this:
What is the expected behavior?
Eslint best practices say that fix rules should not change functionality of code, so that you can safely run fix and expect no functional changes to be made. This rule directly breaks that. I as a repo maintainer see the auto fix as a greater risk than the problems the lint rule prevents. If autofix was turned off, the rule would be entirely a positive.
Unfortunately, ESLint also has rejected the idea of disabling autofix for certain rules. So not following best practices is not ideal.
Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
Not a React Bug but a lint rule.
The text was updated successfully, but these errors were encountered: