Added new eslint rule for hooks: react-hooks/exhaustive-dep

[sangameshsomawar]

[Edited by Yedhin]
Fixes #705

[yedhink]

@sangameshsomawar I had removed this ESLint rule on purpose because it was giving false positives and most importantly too much noisy in our neeto repos. Also, it wasn't auto-fixable by ESLint in most cases.

If you can make a strong case for including this rule and possible fixes for the issues that this rule points out in neeto repos, then we can discuss and consider adding it to wheel.

PS: Moving forward, it would be great if you can please raise an issue and mention me in it if you want to add something to wheel. It's better to first discuss the feasibility of adding something new before making a PR. Also please read: https://www.bigbinary.com/how-we-work/mention-closes-fixes-resolves

Closing this PR for now. You can raise an issue in wheel, if you have a strong case to make and definitely we can re-open this PR.

[sangameshsomawar]

@yedhink

I would like to understand the false positives as I have never faced any.
We can meet over a call or you can simulate then share with me.

Regarding auto fix, Eslint doesn't auto-fix if something changes the behavior of code.
In our case, a list of exhaustive-deps really changes the behavior of code. So the developer has to decide it, eslint can't help. But only because it doesn't auto-fix we should not overlook it.

I highly agree that this rule looks tough in the beginning but it has its importance.

Thanks for pointing out the processes. I will take care of it next time 😄

[yedhink]

@sangameshsomawar I saw issue #705 now and read through the references. Didn't know you had raised the issue since it wasn't linked in this PR.

What I had meant by false positives is that Main.jsx etc are receiving props that are given from the server itself rather than computed on the client-side. Thus if there is a change in those server props, then a re-render from scratch would be either way be required from my understanding. Thus passing it to array deps or not wouldn't matter much from my understanding in this case.

But reading more about this rule and seeing that it's a good practice, I think we can enforce it and see what issues devs are facing based on this rule.

Reopening this PR.

[yedhink]

@sangameshsomawar _a

Once you add the rule, make sure to run it on all files within this repo.

Refer: https://www.bigbinary.com/learn-rubyonrails-book/linting-and-formatting-code#formatting-all-javascript-files

For the time being, I have updated the first comment in this PR to mention which issue it fixes. Moving forward please take care of it.

Please go through the comment and make the necessary changes.

[yedhink]

We already have this rule .esint-rules/react.js. Make this fix over there.

[sangameshsomawar]

Updated.

[yedhink]

@sangameshsomawar _a

Few things to be followed:

• In BigBinary whenever we are making a PR, we assign the PR to the tech lead of that particular project.
• Once the TL reviews the PR, and if they request any changes, then they will assign the PR back to you
• And once you make the changes and push it, then you can assign the PR back to the reviewer.
• No need to mention the issue number in the commit message. We follow the Squash and Merge strategy. Also, we explicitly link the issue in the first comment of a PR.
• No need to add comments like updated etc. It sends out unnecessary notifications via email as well as Github to the subscribed reviewers. In order to acknowledge a comment use emojis. Comments generally are added to get the attention of the reviewer and to clear queries or to point something out explicitly.

Currently:

• This PR has conflicts with master. Thus rebase and fix the conflicts and push the changes.
• Go through the comments in this PR: Added helpers to handle non-stringified values when parsing keys from localStorage #662 (comment)
• I had explicitly pointed out something to be done in Added new eslint rule for hooks: react-hooks/exhaustive-dep #706 (review). Please carefully go through that comment and update with necessary changes.

Once all things are done, you can assign this PR back to me.

[sangameshsomawar]

@yedhink

Sorry if I am troubling you a lot. I am new here in BB 😄
Thank you, For guiding me through the process.

One more issue I am facing when I executed npx eslint --fix "./app/javascript/src/**/*.{js,jsx,json}"

Error: Rules with suggestions must set the `meta.hasSuggestions` property to `true`.
Occurred while linting /Users/sangameshsomawar/Desktop/bbprojects/wheel/app/javascript/src/components/Main.jsx:32
Rule: "react-hooks/exhaustive-deps"

Root Cause Analysis:
2 weeks ago, Someone upgraded eslint to v8.0.1 : https://github.com/bigbinary/wheel/blob/master/package.json#L34

Eslint v8 has some breaking changes:
https://eslint.org/docs/8.0.0/user-guide/migrating-to-8.0.0#suggestions

The above issues have been already reported & fixed by react team.
facebook/react#22545

We need to wait for the new version of eslint-plugin-react-hooks.
We can use eslint-plugin-react-hooks@next but I don't think that will be safe.

[yedhink]

@sangameshsomawar

Sorry if I am troubling you a lot. I am new here in BB

It's my pleasure to help you out. Feel free to ask any questions you have.

We can use eslint-plugin-react-hooks@next but I don't think that will be safe.

This is the only option that I see. We can revert back to stable version once they publish the latest stable version.