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

independently-controllable rules #7

Closed
jrr opened this issue Feb 10, 2019 · 6 comments
Closed

independently-controllable rules #7

jrr opened this issue Feb 10, 2019 · 6 comments

Comments

@jrr
Copy link

jrr commented Feb 10, 2019

I'd like to disable A hook cannot be used inside of another function, but keep all the others (e.g. A hook cannot appear inside an if statement), but they seem to be all fall under the rule react-hooks-nesting.

Can rules be independently controlled?

@Gelio
Copy link
Owner

Gelio commented Feb 10, 2019

@jrr This is a solid suggestion 👍 Initially, I went with the approach eslint-plugin-react-hooks introduced to have everything under one rule.

It can be changed, though. I will consider splitting the rule into multiple ones with lesser individual scopes

Out of curiosity, what is your use case for enabling hooks in regular functions? 😄

@jrr
Copy link
Author

jrr commented Feb 10, 2019

Initially I'd like to set all the rules I'm already in compliance with to "error", and then put the rest on "warning" until we get around to fixing them.

For this particular rule, though, there may be some places we'll never comply, and think it's Probably Okay. For example, when we need to call a hook from tests or storybook, which make heavy use of lambdas.

@Gelio
Copy link
Owner

Gelio commented Feb 13, 2019

I gave some thought into this issue. Having multiple rules means that the parser and rule logic would have to be invoked multiple times due to the limitations of TSLint. One walker cannot report violations of multiple rules.

This can still be achieved, it's just that the performance may be worse than when having a single rule with controllable options.

How about adding options to a single react-hooks-nesting rule? Then, you would be able to allow using hooks in any function. The configuration could be as follows:

"react-hooks-nesting": {
  "severity": "error",
  "options": {
    "allow-using-hooks-in-all-functions": true
  }
}

That would make the rule walker ignore violations connected with checking the function's name.

What do you think? Would that be sufficient?

@jrr
Copy link
Author

jrr commented Feb 13, 2019

Thanks for looking into this!

Having multiple rules means that the parser and rule logic would have to be invoked multiple times due to the limitations of TSLint. One walker cannot report violations of multiple rules.

Yikes, that's disappointing. Do you happen to know if ESLint (with the ts parser) has this limitation?

Your proposal would unblock me on this issue, but doesn't help with the general goal of granularly tightening rules, gradually over time.

I wouldn't bother unless anyone else is clamoring for it; I'll probably just keep severity set to warning, suppress in source code, and/or try my luck with ESLint.

@Gelio
Copy link
Owner

Gelio commented Feb 13, 2019

I think I might have worded that incorrectly. I meant that the walker and rule logic would have to be invoked multiple times. The parser runs once and parses the file into an AST that is then provided to every rules' walkers.

Still, the walker (the class that explores the AST) has to once for every rule and for every CallExpression (essentially for every function call in a file) has to check whether it is a hook call and check its ancestors to determine if it is inside a switch etc.

This is not as bad as running the parser multiple times but still adds some overhead compared to a single rule with one walker that reports violations conditionally, based on the options.

I wouldn't bother unless anyone else is clamoring for it; I'll probably just keep severity set to warning, suppress in source code, and/or try my luck with ESLint.

Sure, I will consider splitting it into multiple rules in the future, but currently, I do not have the time to implement it.

I heard create-react-app wants to use ESLint with the TS parser when using Typescript, so trying ESLint may be a solid idea 👍 For more information on this, take a look at facebook/create-react-app#5641

@jrr
Copy link
Author

jrr commented Aug 31, 2019

Closing because the world has moved on to eslint.

@jrr jrr closed this as completed Aug 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants