eslint-plugin-react
eslint-plugin-react copied to clipboard
`hook-use-state` rule doesn't allow omitting unused setter
Thanks for implementing the new hook-use-state rule! I'm excited to use it on my team's codebase to enforce consistently-named [state, setState] pairs.
Occasionally we have code like this:
const [ constantValue ] = useState(() => expensivelyComputeConstantValue());
We use useState for this because it's the most straightforward way to guarantee that the initializer is run only once. While there may be more idiomatic ways to do this depending on the use case, I don't see a practical downside to using useState this way, so I think would be helpful if the hook allowed this pattern. What do you think about changing hook-use-state so that a one-argument array is considered correct?
The proper thing to do here is useMemo, not useState.
As we understand it, useMemo currently runs the function only once, but doesn't guarantee that it'll always behave that way in the future. Hence we're using useState instead.
Of course it does? It's based on the provided dependency array.
I'm referring, of course, to the big warning on the useMemo docs page. But, thinking a little deeper about this, there are really two cases:
- The memoized function is deterministic. In that case, it's okay if
useMemois run "too often" because the returned value will be the same anyway. - The memoized function is not deterministic (which is the case here – we're using it to generate a random ID). In React 18, we could use
useId(), but not everyone is using React 18 and the IDs it generates aren't applicable for all purposes. Maybe the idiomatic thing to do here is auseRef/useEffectcombo, but that means we don't get the value until after the first render.- If it's okay that the value might change,
useMemoanyway - If the value really has to stay the same across all renders,
useStatestill seems like a semantically accurate option.
- If it's okay that the value might change,
I should also note that our project is not using SSR – generating these values client-side-only with useState seems fine but I don't know if that's still true in the SSR world.
So I guess in the non-deterministic case, the idiomatic thing to do would be something like this:
const randomId = useRef('');
if (!randomId.current) {
randomId.current = generateId();
}
Because, like the warning suggests, this will still work correctly if we removed the "memoization". The thing I don't like about this, which is why I tend to use useState, is that refs have the pesky .current property, which makes things just a little more annoying.
If the value depends on the props, then react should be free to recalculate it whenever it wants.
if it doesn't, then why is it computed inside the component? Memoize it at module level, with your own memoization helper.
Your first suggestion makes sense — looking at my snippet in context, it's trying to give each instance of the component a unique random ID that it keeps for as long as it's mounted. But in this particular case, if the ID happens to get recalculated, it's not going to break anything, and we shouldn't really be thinking about function components in terms of mount/unmount lifecycle anyway.
To your second suggestion, it doesn't depend on the props but it's computed inside the component because the value should be unique per instance. I guess it is quite rare (there's probably a use case but I can't think of one) that one would need a unique value per component instance that isn't allowed to change.
If it's truly the overwhelmingly common case, would it be in character for the React lint plugin to suggest switching from useState to useMemo if it detects that the state setter isn't being used?
Why is computing a unique ID expensive? That's definitely the kind of thing useMemo is meant for.
would it be in character for the React lint plugin to suggest switching from useState to useMemo if it detects that the state setter isn't being used?
That definitely shouldn't be an autofix, but it's a reasonable suggestion for the rule to make, sure!
I have to raise a pattern I use sometimes, for incomplete code that is to be completed later:
const [thing, _setThing] = useState(null);
It violates react/hook-use-state because of the variable naming, but I think an option to allow leading underscore on either of the variable names might be nice to have.
Seems like it'd be more obviously a TODO to add an eslint override comment instead?
I still feel the rule could accept a ignorePattern like eslint core rule no-unused-vars takes as it's essentially the same, a unused var. I configure that eslint rule like this:
no-unused-vars: [2, {args: all, argsIgnorePattern: ^_, varsIgnorePattern: ^_, caughtErrorsIgnorePattern: ^_, destructuredArrayIgnorePattern: ^_, ignoreRestSiblings: false}]
So for consistency sake with that config, I would suggest :
react/hook-use-state: [2, {ignorePattern: ^_}]
Having a rule take a regex string is a very dangerous path that results in a ton of CVEs, so I wouldn't want to replicate that.
If you are afraid of regex execution time, you can use https://github.com/sindresorhus/super-regex, but generally I think eslint configs are unlikely to receive untrusted inputs.
I agree, but they are highly likely to get CVEs about untrusted inputs, which cause a lot of churn and cost.
That package (along with all of that author's packages) aren't viable options, since they're ESM-only.