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

[eslint config] [react] Add support for React Hooks #2022

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@wsmd
Copy link

wsmd commented Mar 13, 2019

Fixes #1944

Looks like everyone (via #1944) is in favor of adding support for React Hooks!

eslint-plugin-react-hooks is stable now and could be included in eslint-config-airbnb.

@wsmd
Copy link
Author

wsmd left a comment

Should I add anything to the React guide (react/README.md) about this?


// Verify the list of the dependencies for Hooks like useEffect and similar
// https://github.com/facebook/react/blob/f99fca3cb28e1fbd87e0b647ac43959add290be7/packages/eslint-plugin-react-hooks/README.md
'react-hooks/exhaustive-deps': 'error',

This comment has been minimized.

@wsmd

wsmd Mar 13, 2019

Author

This is likely to cause errors in existing codebases so it's definitely a breaking change.

Let me know if it should be switched to 'warn', or 'off' with a semver-major todo comment.

'react-hooks/rules-of-hooks': 'error',

// Verify the list of the dependencies for Hooks like useEffect and similar
// https://github.com/facebook/react/blob/f99fca3cb28e1fbd87e0b647ac43959add290be7/packages/eslint-plugin-react-hooks/README.md

This comment has been minimized.

@wsmd

wsmd Mar 13, 2019

Author

The official repo doesn't have a separate README file for each rule, so I linked to the main one. There's also this link: https://reactjs.org/docs/hooks-rules.html

This comment has been minimized.

@ljharb

ljharb Mar 13, 2019

Member

At the least, each rule should have a permalink - if not, can you file an upstream issue for them to add one?

This comment has been minimized.

@wsmd

wsmd Mar 14, 2019

Author

I can get a permalink to the actual rule implementation. Does that work?

There aren't any dedicated READMEs for each one.

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Mar 13, 2019

The addition of a new peer dep is always a breaking change, so there's no avoiding that.


// Enforce Rules of Hooks
// https://github.com/facebook/react/blob/f99fca3cb28e1fbd87e0b647ac43959add290be7/packages/eslint-plugin-react-hooks/README.md
'react-hooks/rules-of-hooks': 'error',

This comment has been minimized.

@ljharb

ljharb Mar 13, 2019

Member

let's add this in a separate entry point; i'm thinking hooks.js, so that you can extend ['airbnb', 'airbnb/hooks']. That way, folks that aren't using hooks or don't want to, don't have to.

This comment has been minimized.

@QuentinRoy

QuentinRoy Mar 14, 2019

Won't having two entry points add unnecessary complexity? If you do not use hooks, these rules won't affect you. But folks may start using hooks thinking they are covered while they're not.

This comment has been minimized.

@ljharb

ljharb Mar 14, 2019

Member

They very well might, since component detection is hard, and functions whose names start with “use” isn’t something hooks invented.

The react team chose a brittle convention for hooks, unfortunately, so while the lint rules are helpful, i don’t think they should be on by default.

This comment has been minimized.

@QuentinRoy

QuentinRoy Mar 14, 2019

Is it to avoid introducing an additional peer dependency? Or just that airbnb's position is that hooks are bad so you would rather ignore them by default?

This comment has been minimized.

@ljharb

ljharb Mar 14, 2019

Member

No, we'd still require the additional peer dependency regardless - there's only one package.json per package.

Airbnb definitely won't be recommending against using hooks; but that doesn't mean there's no caveats with them.

This comment has been minimized.

@QuentinRoy

QuentinRoy Mar 14, 2019

That's what I figured. I suppose you could still not add the peer and document that it is required in this particular condition. But it did look like a terrible idea.
To be honest, I still don't quite understand why it is best to keep these rules as a separated config in this case. But thanks for the answer.

This comment has been minimized.

@wsmd

wsmd Mar 14, 2019

Author

To be honest, I still don't quite understand why it is best to keep these rules as a separated config in this case.

I can see both sides of the argument, but I also see no harm in making them an opt-in. If that's not the case, the individual rules can still be disabled if people choose not to use hooks, but... that's just unnecessary noise and it requires people to maintain their config if more hook rules were to be added in the future.

If you do not use hooks, these rules won't affect you. But folks may start using hooks thinking they are covered while they're not.

I don't necessarily disagree, but worth mentioning that I found a couple of places where methods prefixed with use (that weren't hooks) in a codebase I'm working on. It's possibly an edge case, but it could happen! The rules only use the function name to decide if it is a hook.

For example, with eslint-plugin-react-hooks added, the following file will throw an error even though I'm not using React at all:

function useSomething() {}

export function doSomething() {
  if (window.totallyNotAReactComponent) {
    useSomething();
    ~~~~~~~~~~~~
  }
}
'react-hooks/rules-of-hooks': 'error',

// Verify the list of the dependencies for Hooks like useEffect and similar
// https://github.com/facebook/react/blob/f99fca3cb28e1fbd87e0b647ac43959add290be7/packages/eslint-plugin-react-hooks/README.md

This comment has been minimized.

@ljharb

ljharb Mar 13, 2019

Member

At the least, each rule should have a permalink - if not, can you file an upstream issue for them to add one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.