[New] Symmetric useState hook variable names

[duncanbeevers]

Ensure two symmetrically-named variables are destructured from useState hook calls

Discussion

This is a re-opening of #2873
I've rebased and updated the README using npm run generate-list-of-rules to follow the new table-ish layout.

• Supports both useState and React.useState
• Tested with TypeScript parser and useState<> type annotation
• Robust against weird useState uses like destructuring too many items, or not destructuring at all
• Fixable when 2nd destructured arg does not match set<X> naming convention

Fixes #2417.

[ljharb]

i would have strongly preferred reopening #2873 instead of leaving it permanently orphaned on the repo forever :-/ please strive to avoid opening unnecessary PRs.

[duncanbeevers]

i would have strongly preferred reopening #2873

Same; sorry about the premature close+delete. After I restored the branch, I wasn't able to re-open the PR.
I'll tread more lightly in the future.

[ljharb]

What about React.useState calls?

[duncanbeevers]

What about React.useState calls?

Those are covered in the documentation and in the tests;

https://github.com/yannickcr/eslint-plugin-react/pull/2921/files#diff-21ec40762f2f5ec7df5499db37c5971f0dc3f590927c1f49fddd72b0e103e057R35-R37

    {
      code: 'const [color, setColor] = React.useState()'
    },

[duncanbeevers]

I've added some primitive support for ensuring that React / useState is imported (though not necessarily in-scope).
React.useState calls without an imported React are not flagged. This covers exotic imports of React as demonstrated in a couple of the specs.

import ReactAlternative from 'react'; // React detected, exotic local name
import { useState as useStateAlternative } from 'react'; // React.useState detected, exotic local name

I tried using the ScopeManager to determine at the CallExpression whether React or useState were in scope, but didn't succeed in getting any Scope information at all out of it.

I've never seen a ScopeManager usage in the wild so I may simply have been holding it wrong.

[ljharb]

Could we add some tests for useState etc that aren't the hook, and also some tests with a pragma that isn't "React"?

[karlhorky]

@duncanbeevers just circling back around to this one - do you think that you'll be able to proceed with this PR?

[duncanbeevers]

Yeah, I'll take another pass at this.

I'm seeing some unexpected failures in the TS parsing. It has been a while since I dusted this off and as I'm bringing it into alignment with the parsers.all / features workflow, I'm getting a TS parsing error.
Interestingly, similar TS-based tests parse.

Example

    {
      code: `import { useState } from 'react';
      const [color, setColor] = useState<string>()`, // THIS DOES NOT PARSE
      features: ['ts'],
    },
    {
      code: `import { useState } from 'react';
      const [color, setColor] = useState<string>('#ffffff')`, // THIS PARSES
      features: ['ts'],
    },

Error Message

      AssertionError [ERR_ASSERTION]: A fatal parsing error occurred: Parsing error: Unexpected token

  1 | import { useState } from 'react';
> 2 |       const [color, setFlavor] = useState<string>()
    |                                                   ^

Am I using the wrong features set here, or perhaps there's a bug upstream?

Nevertheless, I'll carry on with expanding test test cases I can get working.

[ljharb]

No, but the ts feature runs on both the old and new TS parsers - it’s fine to add a no-ts-old feature if the old one doesn’t support what’s valid syntax.

[codecov-commenter]

Codecov Report

Merging #2921 (35077ca) into master (9be55ed) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2921      +/-   ##
==========================================
+ Coverage   97.51%   97.53%   +0.01%     
==========================================
  Files         119      120       +1     
  Lines        8180     8245      +65     
  Branches     2934     2967      +33     
==========================================
+ Hits         7977     8042      +65     
  Misses        203      203              

Impacted Files	Coverage Δ
index.js	100.00% <ø> (ø)
lib/rules/hook-use-state.js	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9be55ed...35077ca. Read the comment docs.

[duncanbeevers]

Could we add some tests for useState etc that aren't the hook, and also some tests with a pragma that isn't "React"?

• Added valid specs where a non-React useState function is invoked.
• Added invalid specs where React is imported with an alternative identifier ReactAlternative
• Added invalid specs where useState is imported with an alternative identifier useStateAlternativeName

I've updated the specs to use the new parsers.all helper, annotating TS-specific tests with the ts feature.
I had to annotate a few cases with no-babel-old, all related to Babel(old)'s failure to recognize a typed, 0-argument useState<T>() expression

[ljharb]

This should all be done inside a Components.detect call, to ensure it only runs on things that are detected as SFCs.

[duncanbeevers]

Okay, I wrapped the rule definition in Components.detect

[ljharb]

useState can only be called inside an SFC that React renders (or inside another hook), so i don't think these tests are valid unless they're inside a component or inside a custom hook

[duncanbeevers]

Do you feel that these cases as-written should be flagged as invalid by this rule?
Or should I just add the dressing to these test cases?

[ljharb]

As written, the rule should ignore them, because they're not actually a valid hook call - it will throw at runtime already.

Also, eslint-plugin-react-hooks will, i believe, warn on it as-is.

[ljharb]

i thought we decided this should be ignored? or did we decide instead that its placement should be ignored.

[duncanbeevers]

eslint-plugin-react-hooks#rules-of-hooks is almost entirely dedicated to detecting

• where a hook is being called
• whether it's called in an appropriate context

I see a few options here.

• Do nothing; flag this case as invalid, and rely on eslint-plugin-react-hooks to flag the bad usage
• Do something simple, like ignore top-level invocations of the hook, which solves this test case, but is totally cheating
• Do something ambitious like port (most of) FBs RulesOfHooks over to this repo
• Do something ambitious like integrate FBs RulesOfHooks into util/Components
• Do something ambitious like extract FBs RulesOfHooks into a new module that can be shared by both repos
• Do something even more ambitious which I haven't thought of.

I'm in favor of Do nothing
The price of a mis-flag here is low; neither this rule nor eslint-plugin-react-hooks#rules-of-hooks attempt to fix this situation (suggestions-only), so it doesn't add any extra fixer passes.

[duncanbeevers]

I checked out copying over pieces from rules-of-hooks, but its architected a bit differently.

Whereas this rule relies solely on visiting CallExpression nodes, rules-of-hooks:

• accumulates info about CallExpressions
• and then does its heavy-lifting, including checking the isInsideComponentOrHook status when the rule's onCodePathEnd callback fires.

[duncanbeevers]

I wrapped most test-cases in a little more realistic code; a custom hook that acts as pass-through for the useState return value.
It highlights the problem that I'm able to rename one instance of the variable (within the destructuring assignment) but don't fix other instances of that identifier within the scope.

[ljharb]

Are there other hooks that might want naming guidelines? useReducer, for example? If so, i'd rather a single rule to cover both instead of one for each hook.

[ljharb]

Seems pretty good overall.

[duncanbeevers]

I see there are quite a few CI failures around various node versions, all related to the new shadowed-references code examples I introduced.

In my local testing I had to use the no-default feature in order to avoid the same error while running node 14.17.0, and I expected that same escape hatch would serve for all node versions.

What should my next steps be, if any, in order to address these CI failures?

For now, I'm going to link this in to my day work and see how the rule holds up in day-to-day use on a large codebase.

[ljharb]

so like, in this example:

 import React from 'react'
      const React = {
        useState: () => {
          return null
        }
      }

      export default function useColor() {
        const result = React.useState()
      }

that's not valid javascript, because of the repeated identifier in a top-level scope. To fix that, it'd need to be shadowed inside a nested scope - like this:

 import React from 'react'

      export default function useColor() {
        { 
        const React = {
          useState: () => {
            return null
          }
        }
        const result = React.useState()
      }
      }

but you basically can't shadow an import at the top level in the first place.

[duncanbeevers]

Thanks for the suggestion. I haven't had to deal with shadowing in the wild in a LONG time!
After the implementing the update, I was able to remove the no-default features and it looks like the tests are much happier. 😄

[duncanbeevers]

I found some usages in our codebase where we use only the first member of setState as an ad-hoc memoization.

const [id] = useState(getUniqueElementId())

As it stands, this rule will flag this usage and recommend the following correction

// Now `setId` is unused
// Maybe this is good when you're first writing the code
const [id, setId] = useState(getUniqueElementId())

However, in this case I think the correct change is to directly memoize the value.

// Now id hook-use-state is happy, but useMemo may not be imported
const id = useMemo(() => getUniqueElementId(), [])

Discussion

Should this rule:

• do the naive change, and add the setId member
• attempt the bigger refactor
• something else (ignore, warn-but-not-fix, etc…)

[ljharb]

I think it should offer a suggestion to change it to useMemo, but no autofix at all - if someone writes that, we can’t know their intention, so the safe thing to do is force them to manually intervene.

Since you know which AST node imported useState, can the suggestion change that also so it adds useMemo? (one of the no-unused rules can clean up the useState separately, if it becomes unused)

[duncanbeevers]

Since you know which AST node imported useState, can the suggestion change that also so it adds useMemo? (one of the no-unused rules can clean up the useState separately, if it becomes unused)

Yep; it shouldn't even be super difficult since with the new Components react-imports tracking, I can tell whether or not useMemo has already been imported.

[duncanbeevers]

Are there other hooks that might want naming guidelines? useReducer, for example? If so, i'd rather a single rule to cover both instead of one for each hook.

I certainly use some naming conventions, but in my experience there's nothing as universally-used as this value+setter idiom.

Here are some conventions I use regularly, just as a concrete example of stuff I would enforce with a lint rule. These are simply personal preferences, and I don't feel they represent common practice.

• const myRef = useRef() – -Ref suffix makes it easy to remember to use myRef.current when handling this value
• const onMyEvent = useCallback(() => … – on- prefix (or handle- prefix, in some projects) makes it easy to spot functions, and to sort callback props together in JSX Components.

[ljharb]

Fair - I'm more thinking that if there were ever to be anything in the future, it'd be nicer to add it as an option to a single hook naming rule than to have to make a whole new rule.

[duncanbeevers]

Config / Other Hooks

Yes, I tried to consider some real-world cases, and how this rule might accommodate those conventions, but I couldn't come up with anything that was both compelling, and that felt amenable to being expressed here alongside this rule.

One thing I really like about this rule as it stands is zero-configuration.

If naming (or other) conventions emerge around other hooks in the future, adding new rules for those hooks while keeping the config for this one clean seems like a reasonable approach. This code supporting this hook-use-state rule is already pretty complicated!

Use in the Wild

I hooked this rule up to a couple of work repositories; as expected I saw a few usages flagged. Somewhat unexpected was that it didn't crash on anything! (pats self on back)

The automatic variable renaming works, but falls short of a complete refactor. Here's a small example.

Screen.Recording.2021-12-12.at.9.43.42.AM.mov

This behavior makes me feel that all the fixes should be changed to suggestions, since I can't reliably rename later usages of the variable.

Maybe there's an easy way to do this properly, but if so I feel that rename-this-variable-throughout-the-scope should live in a helper function for other rules to use, and not be implemented as a one-off inside this rule.

Next Steps

Let me know which way you'd like to see this go. If I don't hear anything right away, I'll probably convert the fixes to suggestions, and leave the larger variable-rename-refactor work for another PR.

[ljharb]

I agree such a thing would need to be a shared helper, and i agree that they shouldn’t be autofixes if they’re going to leave things in a broken state. Your plan seems fine.

[duncanbeevers]

Okay, this rule is now suggestions-only, including the useMemo suggestion (only applied when useState is only destructured into a 1-element array, and is called with 1 argument).

Specs are updated, and the internal implementation is simplified since everything is suggestions now (no more fix/suggest split)

[duncanbeevers]

I think the next step for this might be to actually do the correct variable rename within its scope. It seems like a a chunk of work, but maybe there's something already out there in the eslint ecosystem to accomplish this.

I saw this comment about the general technique of expanding the fix range to cover the entire scope of the changed variable.

[ljharb]

i thought we decided this should be ignored? or did we decide instead that its placement should be ignored.

[ljharb]

I rebased this with some tweaks, and there's a failing test - namely, the one where it should ignore hook calls outside of components (unless we agreed not to treat those differently, in which case, please point me to that thread). Thanks!

[duncanbeevers]

i thought we decided this should be ignored? or did we decide instead that its placement should be ignored.

Implementing this (ignoring hook usages in non-valid contexts) is equivalent to a large portion of the work eslint-plugin-react-hooks#rules-of-hooks does.

I replied in this comment as to why I think I think the best option is not to implement such functionality for this rule, at least today.
#2921 (comment)