-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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] hook-use-state
: Allow UPPERCASE setState setter prefixes
#3244
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3244 +/- ##
==========================================
- Coverage 97.69% 97.68% -0.02%
==========================================
Files 121 121
Lines 8550 8554 +4
Branches 3096 3100 +4
==========================================
+ Hits 8353 8356 +3
- Misses 197 198 +1
Continue to review full report at Codecov.
|
{ | ||
code: ` | ||
import { useState } from 'react' | ||
function useCustomColorValue() { |
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 added this case to ensure the camel-case splitter case captures the correct suffix.
import { useState } from 'react' | ||
function useRGB() { | ||
const [rgb, setRGB] = useState() | ||
return [rgb, setRGB] |
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.
Testing both:
- bare ALL-UPPERCASE value
- compound UPPERCASE-prefix + arbitrary suffix
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.
LGTM, thanks
lib/rules/hook-use-state.js
Outdated
@@ -72,7 +72,7 @@ module.exports = { | |||
const expectedSetterVariableNames = upperCaseCandidatePrefix ? [ | |||
`set${upperCaseCandidatePrefix.charAt(0).toUpperCase()}${upperCaseCandidatePrefix.slice(1)}${caseCandidateSuffix}`, | |||
`set${upperCaseCandidatePrefix.toUpperCase()}${caseCandidateSuffix}`, | |||
] : undefined; |
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 usually try to avoid this empty-object fallback pattern as it allocates unnecessary empty objects, and because it aligns really naturally with the conditional-access-operator pattern in more-modern js versions.
Happy to follow with the suggestions, but just thought I'd share my thoughts, particularly because they've been shaped somewhat by this repository's tool-for-all-ages mentality.
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.
yeah, that's true, but without optional chaining, it's a lot grosser. i prefer having a constant type for things; it's simpler to reason about, and "object allocation" is a non-problem.
hook-use-state
: Allow UPPERCASE setState setter prefixes
I think this is a bugfix, not an addition :-) |
304edf2
to
cab6943
Compare
As you wish to call it… I'll trust your judgement and re-message the commit before squashing. However, imo…
To me this seems like a refinement / enhancement / new feature, rather than addressing a bug that emerged from using the old behavior. |
It certainly adds a regression if someone was depending on the previous behavior; we just forgot about initialisms in the initial creation of the rule. I've already rebased it; i'll land it once tests pass. |
Closes #3243
This PR allows
useState
destructured variable names to use ALL-UPPER-CASE for the first segment of the setter variable name.Example
Discussion
The existing suggested variable name
setRgbColor
is still the sole suggestion;setRGBColor
is considered valid, but is never suggested.This should be a non-breaking change as all previously-valid code is still considered valid.