-
Notifications
You must be signed in to change notification settings - Fork 16
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
347 Add eslint plugin react hooks #359
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for adding the plugin!
I just think that fixing all the ESLint errors or warnings related to the new plugin should be within the scope of this issue. Adding a plugin doesn't mean much if we leave the code with errors. What truly adds value is addressing and resolving all these issues
Yes, it makes total sense, we could tackle those warnings as well. I'm going to resize this issue due to the large amount of warnings received about incorrect dependencies in useEffect which is something I need to be careful with because otherwise we can end up in infinite loops, multiple calls to the api or unnecessary re-renders. |
As discussed with the team, we thought the best idea would be to resolve the remaining warnings in a separate issue. I have created a new issue to resolve the remaining issues/warnings. |
Added waiting tag, might be good to wait to merge this issue until we get the e2e tests fixed. |
Hi @MellyGray , I update the branch with the fix of the e2e tests, both unit and e2e tests are passing 🎉 . |
…-hooks 347 Add eslint plugin react hooks
What this PR does / why we need it:
Adds Eslint plugin for React Hooks.
We need it to enforce Rules of Hooks.
Which issue(s) this PR closes:
Special notes for your reviewer:
The plugin raised only a few errors when initially configured, only related to calling hooks conditionally, all were resolved.
There are several minor warnings that we could fix as we come across them.
I only hide via a comment of eslint two errors regarding the use of a hook in the Story of the DatasetAlert as I think it might take more time than expected and it is just a story, however we could see how to configure the alerts story to avoid the eslint error in the future.
Suggestions on how to test this:
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
No
Is there a release notes update needed for this change?:
No
Additional documentation: