-
Notifications
You must be signed in to change notification settings - Fork 22
chore: add react hooks rules eslint plugin #5209
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
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5209 +/- ##
==========================================
+ Coverage 60.64% 60.71% +0.07%
==========================================
Files 1145 1145
Lines 81747 81613 -134
Branches 7014 7006 -8
==========================================
- Hits 49575 49553 -22
+ Misses 31989 31845 -144
- Partials 183 215 +32
🚀 New features to boost your workflow:
|
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 like a good refactor and safer code!
@@ -87,7 +87,7 @@ const DropdownElement = ({ | |||
</Button> | |||
</CogsTooltip> | |||
)}> | |||
{command.children.map((child) => createMenuItem(child, setOpen))} | |||
{command.children.map((child) => CreateMenuItem(child, setOpen))} |
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.
This doesn't seem righ to me - CreateMenuItem
contains a hook call, and when we call the component like a function like this, the hook will be counted as part of the parent components' hook. That means that if the number or order of commands change for some reason, the number of hook calls in this component will change, and React may explode
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.
I see that this may be the case in other components in this PR as well
Type of change
Description 📝
Enable lint plugin for react hooks rules