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

fix enable rules of hooks #329

Merged
merged 4 commits into from
Jul 5, 2023

Conversation

jesperhodge
Copy link
Member

@jesperhodge jesperhodge commented May 9, 2023

This is a PR enabling eslint "rules-of-hooks". It is a proposal from my side - needs agreement from the team.

There are a couple of reasons why I would like to do this.

  1. the rules of hooks prevent some very hard-to-fix bugs by ensuring hooks are always called in the same order and ensuring use of hooks is visible at the component level to the developer. By disabling the eslint rule we make all the potential bugs invisible.
  2. to prevent these bugs, it is necessary to distinguish custom hooks from normal JS functions by prepending them with "use". We don't do that, but we should.
  3. We are wilfully breaking the rules of hooks the way we have setup our "hooks.js" files. However, the rules of hooks are not optional. React.js docs specifically state that they are mandatory, not optional, including the naming of custom hooks.
  4. We already discovered cases in which mistakes were made due to our having disabled the linter rule.
  5. I have not seen any necessity whatsoever in this codebase for breaking these rules. It seems the only reason we break them is because this is the way code was written in the past, but obviously it creates some problems.
  6. Disabling this rule and putting hooks into functions that are not named as custom hooks also forces us to turn off the "exhaustive deps" rule, which can be equally helpful in finding bugs. That rule cannot work unless the other rules are followed, so that is a negative consequence of disabling the rules of hooks linting.

I added eslint-disable statements wherever the rules are broken, and if this is merged, I would expect new code not to break the rules any longer.

I strongly suggest that the much better way to extract and reuse hooks and logic from components is the way the community does it and the React docs suggest. The new React docs are really fantastic in this regard and you can use the patterns found there very well to have an excellent application. https://react.dev/learn/reusing-logic-with-custom-hooks

@codecov
Copy link

codecov bot commented May 9, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.24 🎉

Comparison is base (c09faa3) 90.38% compared to head (1a44b40) 90.62%.

❗ Current head 1a44b40 differs from pull request most recent head d147bb2. Consider uploading reports for the commit d147bb2 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #329      +/-   ##
==========================================
+ Coverage   90.38%   90.62%   +0.24%     
==========================================
  Files         221      216       -5     
  Lines        3899     3735     -164     
  Branches      757      720      -37     
==========================================
- Hits         3524     3385     -139     
+ Misses        356      329      -27     
- Partials       19       21       +2     
Impacted Files Coverage Δ
...rs/EditorContainer/components/TitleHeader/hooks.js 92.85% <ø> (ø)
...s/EditorContainer/components/TitleHeader/index.jsx 100.00% <ø> (ø)
src/editors/containers/EditorContainer/hooks.js 100.00% <ø> (ø)
...r/components/EditProblemView/AnswerWidget/hooks.js 100.00% <ø> (ø)
...components/EditProblemView/SettingsWidget/hooks.js 95.18% <ø> (+0.24%) ⬆️
...Widget/settingsComponents/GeneralFeedback/hooks.js 58.33% <ø> (ø)
...gsWidget/settingsComponents/GroupFeedback/hooks.js 79.24% <ø> (ø)
...gsWidget/settingsComponents/Randomization/hooks.js 70.00% <ø> (ø)
.../ProblemEditor/components/EditProblemView/hooks.js 100.00% <ø> (ø)
.../ProblemEditor/components/SelectTypeModal/hooks.js 90.90% <ø> (ø)
... and 19 more

... and 42 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@rayzhou-bit rayzhou-bit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me!

Copy link
Contributor

@kenclary kenclary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right on

@connorhaugh
Copy link
Contributor

I haven't reviewed the code changes, but the spirit is good with me.

@jesperhodge jesperhodge merged commit f942ef9 into openedx:main Jul 5, 2023
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

Successfully merging this pull request may close these issues.

6 participants