Skip to content

fix: Address eslint react-hook violations in CharacterCount #3321

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.

Sign up for GitHub

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

Merged
brandonlenz merged 3 commits into from
Nov 13, 2025
Merged

fix: Address eslint react-hook violations in CharacterCount #3321

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d16d044
fix: Address eslint react-hook violatoins in CharacterCount
mdmower-csnw Nov 11, 2025
9d33668
(Revisions) fix: Address eslint react-hook violatoins in CharacterCount
mdmower-csnw Nov 12, 2025
9dbe9bd
Merge branch 'main' into mdm-charcount
brandonlenz Nov 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 12 additions & 10 deletions src/components/forms/CharacterCount/CharacterCount.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ export const CharacterCount = ({
getMessage = defaultMessage,
...remainingProps
}: TextInputCharacterCountProps | TextareaCharacterCountProps): JSX.Element => {
const initialCount = getCharacterCount(value || defaultValue)
const [initialCount] = useState(getCharacterCount(value || defaultValue))
Copy link
Contributor

@brandonlenz brandonlenz Nov 12, 2025

Choose a reason for hiding this comment

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

I think this is fine, so not blocking, but out of curiosity: Why not use a ref here?

Something like

Suggested change
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

Copy link
Contributor Author

@mdmower-csnw mdmower-csnw Nov 12, 2025

Choose a reason for hiding this comment

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

@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 current property) 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.

const [length, setLength] = useState(initialCount)
const [message, setMessage] = useState(getMessage(initialCount, maxLength))
const [isValid, setIsValid] = useState(initialCount < maxLength)
Expand All @@ -76,17 +76,21 @@ export const CharacterCount = ({
'usa-character-count__status--invalid': !isValid,
})

useEffect(() => {
const message = getMessage(length, maxLength)
setMessage(message)
const [prevLength, setPrevLength] = useState(length)
if (length !== prevLength) {
Copy link
Contributor Author

@mdmower-csnw mdmower-csnw Nov 11, 2025

Choose a reason for hiding this comment

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

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
>

setPrevLength(length)
setMessage(getMessage(length, maxLength))
setIsValid(length <= maxLength)
}

useEffect(() => {
// Updates the character count status for screen readers after a 1000ms delay
const timer = setTimeout(() => {
// Setting the text directly for VoiceOver compatibility.
if (srMessageRef.current) srMessageRef.current.textContent = message
}, 1000)
return () => clearTimeout(timer)
}, [length])
}, [message])

const handleBlur = (
e:
Expand All @@ -97,7 +101,7 @@ export const CharacterCount = ({
): void => {
const validationMessage = !isValid ? 'The content is too long.' : ''
e.target.setCustomValidity(validationMessage)
if (callback) callback(e)
callback?.(e)
}

const handleChange = (
Expand All @@ -107,12 +111,10 @@ export const CharacterCount = ({
// eslint-disable-next-line @typescript-eslint/no-explicit-any
callback?: (e: any) => void
): void => {
const {
target: { value = '' },
} = e
const { value } = e.target
setLength(getCharacterCount(value))

if (callback) callback(e)
callback?.(e)
}

let InputComponent: JSX.Element
Expand Down