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

chore: begin adding strict flow types to lint rule #34

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bradzacher
Copy link

@bradzacher bradzacher commented Jul 19, 2022

I mentioned this in #33, but I figured I'd get the ball rolling and show what it can look like.
These types allow us to strictly type the lint rules so that they

  • only use the latest ESLint APIs
  • declare a strict and complete rule object
  • correctly declare tests
  • safely access AST node properties

So far I've only migrated one lint rule fully (noWeakTypes) just as an example.
This PR should be "CI complete", so we could land this with the intention of fully typing the entire codebase if that's something we were interested in.

This PR also switches to hermes-eslint (as per #33).

cc @mrtnzlml


For context - adding strict types to rules is a HUGE win for contributors as they need to think less. I've found that it greatly reduces the barrier to entry (thanks to autocomplete and flow protecting against bugs early), and lessens the time spent reviewing changes (as many sanity checks are done by flow).

I mentioned this in flow-typed#33, but I figured I'd get the ball rolling and show what it can look like.
These types allow us to strictly type the lint rules so that they
- only use the latest ESLint APIs
- declare a strict and complete rule object
- correctly declare tests
- safely access AST node properties

This PR also switches to `hermes-eslint` (as per flow-typed#33).

cc @mrtnzlml
@Brianzchen
Copy link
Member

Thanks for putting this together. I'll hopefully take a look into this over the weekend, I'm just working on some flow-typed stuff and obviously work. But seems there's some test failures that need taking care of?

@bradzacher
Copy link
Author

There are some test failures because both define-flow-type and use-flow-type are no longer required (and won't even work!).

Other test failures are things I didn't look into at all - this is just a work in progress that I'm happy to push on with if you're interested in this being part of the repo!

@Brianzchen
Copy link
Member

Brianzchen commented Jul 25, 2022

The changes at a high level make sense to me and they seem good, I'll admit I'm not a master of eslint and I trust what you're going for here, but

only use the latest ESLint APIs

Does this mean compatibility with eslint@8 only? What if someone is using an older version?

fully typing the entire codebase

This is only of value for codebase strictness right? Users consuming this library wouldn't feel that impact?

I'd also be interested in having this contributed back to flow-typed such as the eslint typings.


So what you've done from what I can tell is upgrade the way the rule is written, I presume to newer standards and then changed the test runner to run on hermes-eslint instead of @babel/eslint-parser. I can't see any difference to a consumer besides the rule rewrite. If a consumer continues to use @babel would they come across any issues?

And if they can keep using @babel should we break the test runner to run against both @babel/eslint-parser and hermes-eslint to ensure compatibility?

I'm pretty happy to push hermes-eslint as the standard and what we can do is update the docs + recommended config to reflect that

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

Successfully merging this pull request may close these issues.

2 participants