eslint-plugin-react icon indicating copy to clipboard operation
eslint-plugin-react copied to clipboard

`hook-use-state` rule doesn't allow omitting unused setter

Open drbr opened this issue 3 years ago • 14 comments

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?

drbr avatar Apr 12 '22 05:04 drbr

The proper thing to do here is useMemo, not useState.

ljharb avatar Apr 12 '22 05:04 ljharb

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.

drbr avatar Apr 12 '22 05:04 drbr

Of course it does? It's based on the provided dependency array.

ljharb avatar Apr 12 '22 05:04 ljharb

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:

  1. The memoized function is deterministic. In that case, it's okay if useMemo is run "too often" because the returned value will be the same anyway.
  2. 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 a useRef/useEffect combo, but that means we don't get the value until after the first render.
    • If it's okay that the value might change, useMemo anyway
    • If the value really has to stay the same across all renders, useState still seems like a semantically accurate option.

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.

drbr avatar Apr 12 '22 05:04 drbr

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.

drbr avatar Apr 12 '22 06:04 drbr

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.

ljharb avatar Apr 12 '22 06:04 ljharb

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?

drbr avatar Apr 12 '22 06:04 drbr

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!

ljharb avatar Apr 12 '22 06:04 ljharb

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.

silverwind avatar Sep 06 '22 15:09 silverwind

Seems like it'd be more obviously a TODO to add an eslint override comment instead?

ljharb avatar Sep 06 '22 17:09 ljharb

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: ^_}]

silverwind avatar Sep 06 '22 17:09 silverwind

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.

ljharb avatar Sep 06 '22 17:09 ljharb

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.

silverwind avatar Sep 06 '22 17:09 silverwind

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.

ljharb avatar Sep 06 '22 17:09 ljharb