fix: Address eslint react-hook violations in CharacterCount#3321
fix: Address eslint react-hook violations in CharacterCount#3321brandonlenz merged 3 commits intotrussworks:mainfrom
Conversation
| const message = getMessage(length, maxLength) | ||
| setMessage(message) | ||
| const [prevLength, setPrevLength] = useState(length) | ||
| if (length !== prevLength) { |
There was a problem hiding this comment.
This matches the current implementation, only updating the message and validation status when the length changes. A more complete revision would respect an isolated change to the getMessage prop. It's a bit of an edge case, but not completely unrealistic:
<CharacterCount
getMessage={new Date() >= modern_requirements_date ? modernMessageMaker : legacyMessageMaker}
...props
>| ...remainingProps | ||
| }: TextInputCharacterCountProps | TextareaCharacterCountProps): JSX.Element => { | ||
| const initialCount = getCharacterCount(value || defaultValue) | ||
| const [initialCount] = useState(getCharacterCount(value || defaultValue)) |
There was a problem hiding this comment.
I think this is fine, so not blocking, but out of curiosity: Why not use a ref here?
Something like
| const [initialCount] = useState(getCharacterCount(value || defaultValue)) | |
| const initialCount = useRef(null) | |
| if (intialCount.current === null) { | |
| intialCount.current = getCharacterCount(value || defaultValue) | |
| } |
Admittedly, it looks way grosser, but is supposedly more semantically correct. Found some relevant discussion/examples here
There was a problem hiding this comment.
@brandonlenz - Argh, neither of our approaches is correct here. I'm glad you left a comment.
My approach failed at the one improvement it was supposed to achieve: only evaluate getCharacterCount(value || defaultValue) once for the purpose of setting initialCount during the initial render. Use that value for the next few useState() lines and then forget about it. Because I passed getCharacterCount(value || defaultValue) as a value to useState() rather than through an initializer function, it still gets evaluated every render. My change only achieved adding tiny overhead to each render cycle.
Your approach does better at only evaluating getCharacterCount(value || defaultValue) once like intended (kudos for spotting that and not assigning the value as the useRef() argument). Unfortunately, it violates another principle: ref values are not supposed to be accessed during renders. Enabling react-hooks recommended eslint ruleset notes this:
Error: Cannot access refs during render
React refs are values that are not needed for rendering. Refs should only be accessed outside of render, such as in event handlers or effects. Accessing a ref value (the
currentproperty) during render can cause your component not to update as expected (https://react.dev/reference/react/useRef)
In 9d33668, I switch to using initializer functions for this setState and also for the message setState.
This is a rule we can enable, if you would prefer! |
Use initializer functions for setState calls.
Enabling the ruleset reveals dozens of violations. Some are challenging to solve. I was planning to peck away at them over a long period of time. If CI doesn't depend on an error free lint report, sure enabling the ruleset would be nice. Otherwise, I'm happy to keep temporarily enabling the rules as I find motivation. |
brandonlenz
left a comment
There was a problem hiding this comment.
Thanks for taking the time to reevaluate that logic! LGTM now
Summary
useEffectwith state updates during render, as described in Adjusting some state when a prop changes.initialCountonce and don't keep reevaluating it in each render.valuecannot be undefined for text area and text input.How To Test