Skip to content
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.

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

Feature request: enforce naming convention for useState() #2417

Closed
mbrowne opened this issue Sep 19, 2019 · 22 comments · Fixed by #2921
Closed

Feature request: enforce naming convention for useState() #2417

mbrowne opened this issue Sep 19, 2019 · 22 comments · Fixed by #2921

Comments

@mbrowne
Copy link

mbrowne commented Sep 19, 2019

It would be great to have a rule to enforce the standard naming convention for useState():

const [x, setX] = useState(...)

For example, the rule would consider this an error:

const [x, setFoo] = useState(...)

This would ensure that all useState variables followed the same convention, i.e. ${varName} and set${upperFirstVarName}.

@ljharb
Copy link
Member

ljharb commented Sep 25, 2019

This seems like a better rule for eslint-plugin-react-hooks.

@mbrowne
Copy link
Author

mbrowne commented Sep 25, 2019

@ljharb I read somewhere that eslint-plugin-react-hooks deliberately avoids including stylistic rules, and intends to focus only on behavioral things, i.e. "rules of hooks". But maybe they would be open to adding this stylistic rule if it were disabled by default...I could open an issue there and see what they say.

@karlhorky
Copy link
Contributor

@ljharb Would the eslint-plugin-react project be opposed to adding this feature?

Seems like the issue in facebook/react to add it to eslint-plugin-react-hooks got no response and was autoclosed by the bot...

@ljharb
Copy link
Member

ljharb commented Sep 22, 2020

I'd love to see it go into eslint-plugin-react-hooks if possible.

@karlhorky
Copy link
Contributor

Ok, @mbrowne opened a new PR at facebook/react, let's see if it gets any traction: facebook/react#19883

@karlhorky
Copy link
Contributor

karlhorky commented Nov 5, 2020

So gaearon confirmed in facebook/react#19883 (comment):

We won't be focusing on stylistic enforcement like this in the official plugin. This is definitely welcome in the ecosystem, but the official plugin focuses on correctness rather than style or naming.

@ljharb would a pull request be welcome in eslint-plugin-react?

@ljharb
Copy link
Member

ljharb commented Nov 5, 2020

Is there a way the “hook detection” code could be shared between the two? I don’t want this plugin to have to reinvent it, and then risk deviating from it in the future.

@karlhorky
Copy link
Contributor

Maybe depending on eslint-plugin-react-hooks and then somehow using the isHook and isHookName functions?

https://github.com/facebook/react/blob/30b47103d4354d9187dc0f1fb804855a5208ca9f/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js#L12-L40

Unfortunately they are not exported, but depending on the willingness of @gaearon to expose them and the value of sharing this code, I guess the file could be patched using patch-package or similar...

@ljharb
Copy link
Member

ljharb commented Nov 5, 2020

Hopefully they can either be exported, or published as a separate package that both consume. @gaearon?

@karlhorky
Copy link
Contributor

karlhorky commented Nov 17, 2020

Actually, looking into this a bit more, I guess the code that would be shared would be low (just the single-line regex). And - we probably could just match only the literal string 'useState', since this rule will only apply to this hook.

So maybe we don't need to export the code.

I am a beginner at writing ESLint plugins, but I've put together something as a proof of concept that we probably wouldn't need to share code on AST explorer. The only thing specific to React Hooks here is the one string "useState".

Thoughts @ljharb ?

function MyComponent() {
  const [a, set_A] = useState();
  const [x, setFoo] = useState('abc');
  const [c, d] = [1,2]; 
}
export default {
  meta: {
    messages: {
      useStateNaming:
        "Identifier '{{name}}' does not follow useState naming convention.",
    },
  },
  create(context) {
    return {
      Identifier(node) {
        if (
          node.parent &&
          node.parent.type === "ArrayPattern" &&
          node.parent.parent &&
          node.parent.parent.type === "VariableDeclarator" &&
          node.parent.parent.init &&
          node.parent.parent.init.callee &&
          node.parent.parent.init.callee.name === "useState" &&
          node.parent.parent.parent &&
          node.parent.parent.parent.type === "VariableDeclaration"
        ) {
          context.report({
            node,
            messageId: "useStateNaming",
            data: { name: node.name },
          });
        }
      },
    };
  },
};
// Identifier 'a' does not follow useState naming convention. (at 2:10)
     const [a, set_A] = useState();
// ---------^

// Identifier 'set_A' does not follow useState naming convention. (at 2:13)
     const [a, set_A] = useState();
// ------------^

// Identifier 'x' does not follow useState naming convention. (at 3:10)
     const [x, setFoo] = useState('abc');
// ---------^

// Identifier 'setFoo' does not follow useState naming convention. (at 3:13)
     const [x, setFoo] = useState('abc');
// ------------^

Source: https://astexplorer.net/#/gist/dc0972f911d088baea1f6bf34870eec6/51aa024746ed3fb03c15b04793a86afb7c96d955

@karlhorky
Copy link
Contributor

karlhorky commented Nov 17, 2020

One downside that I can think of is that this will also check code outside of React components.

But I guess a recursive check through all parents to check whether their Identifier names start with capital letters is also not too much code.

@ljharb
Copy link
Member

ljharb commented Nov 22, 2020

We have component detection logic, so you should be able to only warn inside functional components.

@ljharb
Copy link
Member

ljharb commented Nov 22, 2020

Also, note that someone could rename useState to anything else, so we have to follow any renames or assignments.

@karlhorky
Copy link
Contributor

Also, note that someone could rename useState to anything else, so we have to follow any renames or assignments.

Oh... I think the official plugin does not support this.

Gut feel says this seems like a nightmare to implement (I guess that's why the official plugin doesn't do it either) - or maybe you have a clear idea of how to traverse the AST to do this easily?

@karlhorky
Copy link
Contributor

We have component detection logic

Ah that sounds great. Would you be open to me starting on a PR for this with my rudimentary AST / ESLint API skills? 😅 I may need some guidance...!

@ljharb
Copy link
Member

ljharb commented Nov 22, 2020

Opening a draft PR sounds great. eslint provides some utilities for tracking variable renames; we’d have to research what’s available.

@duncanbeevers
Copy link
Contributor

We have component detection logic, so you should be able to only warn inside functional components.

Hooks are used outside of functional components; for example, when writing a custom hook.

@karlhorky
Copy link
Contributor

Hooks are used outside of functional components; for example, when writing a custom hook.

Ah, good catch!

And thanks for the PR at #2873, looking pretty good!

@karlhorky
Copy link
Contributor

New PR is at #2921, for anyone interested.

@duncanbeevers do you think that you'll get a chance to look at this again sometime soon?

@karlhorky
Copy link
Contributor

Thanks for the extensive work @duncanbeevers ! And the detailed review work @ljharb !

Will this be a part of 7.29.0?

@ljharb
Copy link
Member

ljharb commented Dec 31, 2021

Eventually yes.

@karlhorky
Copy link
Contributor

Nice, thanks for the release of 7.29.0, works well! 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

4 participants