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

347 Add eslint plugin react hooks #359

Merged
merged 13 commits into from
May 22, 2024

Conversation

g-saracca
Copy link
Contributor

@g-saracca g-saracca commented Apr 1, 2024

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:

  • Run tests to check everything works as usual.
  • Locate a useEffect call after a conditional statement in a react component, it should show an eslint error.
  • Use a local state variable or a prop inside a useEffect and don't add it to the dependency array of it, it should show an eslint warning.

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:

@g-saracca g-saracca added Size: 3 A percentage of a sprint. 2.1 hours. pm.GREI-d-2.7.1 NIH, yr2, aim7, task1: R&D UI modules for creating datasets and supporting publishing workflows pm.GREI-d-2.7.2 NIH, yr2, aim7, task2: Implement UI modules for creating datasets and publishing workflows labels Apr 1, 2024
@g-saracca g-saracca changed the title 347 eslint plugin react hooks 347 Add eslint plugin react hooks Apr 1, 2024
Copy link
Contributor

@MellyGray MellyGray left a 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

@g-saracca
Copy link
Contributor Author

g-saracca commented Apr 2, 2024

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.

@g-saracca g-saracca added Size: 30 A percentage of a sprint. 21 hours. (formerly size:33) and removed Size: 3 A percentage of a sprint. 2.1 hours. labels Apr 2, 2024
@coveralls
Copy link

coveralls commented Apr 19, 2024

Coverage Status

coverage: 97.83% (+0.001%) from 97.829%
when pulling fafc7a6 on feature/347-eslint-plugin-react-hooks
into 2ace63e on develop.

@g-saracca
Copy link
Contributor Author

g-saracca commented Apr 29, 2024

As discussed with the team, we thought the best idea would be to resolve the remaining warnings in a separate issue.
At the moment we dropped from having 7 errors and 24 warnings to only 8 warnings and no errors. ✅

I have created a new issue to resolve the remaining issues/warnings.
#386

@g-saracca g-saracca assigned MellyGray and unassigned g-saracca Apr 29, 2024
@g-saracca g-saracca added Size: 3 A percentage of a sprint. 2.1 hours. and removed Size: 30 A percentage of a sprint. 21 hours. (formerly size:33) labels Apr 29, 2024
@g-saracca
Copy link
Contributor Author

Added waiting tag, might be good to wait to merge this issue until we get the e2e tests fixed.

@g-saracca g-saracca removed the Waiting label May 16, 2024
@g-saracca
Copy link
Contributor Author

Hi @MellyGray , I update the branch with the fix of the e2e tests, both unit and e2e tests are passing 🎉 .
You can retake the review of this PR again now, thanks!

@MellyGray MellyGray removed their assignment May 21, 2024
@GPortas GPortas self-assigned this May 22, 2024
@GPortas GPortas merged commit 0d1d97e into develop May 22, 2024
14 checks passed
jayanthkomarraju pushed a commit to jayanthkomarraju/dataverse-frontend that referenced this pull request May 31, 2024
…-hooks

347 Add eslint plugin react hooks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pm.GREI-d-2.7.1 NIH, yr2, aim7, task1: R&D UI modules for creating datasets and supporting publishing workflows pm.GREI-d-2.7.2 NIH, yr2, aim7, task2: Implement UI modules for creating datasets and publishing workflows Size: 3 A percentage of a sprint. 2.1 hours.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add eslint-plugin-react-hooks
4 participants