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

Switch to use hermes-eslint as the recommended parser #33

Closed
bradzacher opened this issue Jun 16, 2022 · 14 comments · Fixed by #41
Closed

Switch to use hermes-eslint as the recommended parser #33

bradzacher opened this issue Jun 16, 2022 · 14 comments · Fixed by #41

Comments

@bradzacher
Copy link

We just released v0.8.0 of hermes-eslint.
As of this release we have full support for ESLint 8+ - meaning we support the latest ESTree syntax for class properties, class private properties and optional chaining.

hermes-eslint also has a fully-featured scope analyser which understands flow types. This means that the rules ft-flow/define-flow-type and ft-flow/use-flow-type are no longer needed! Also fun side-note that the rules react/jsx-uses-react and react/jsx-uses-vars are not needed because the analyser also supports JSX!

Worth noting that this will also reveal a bunch of no-undef errors in codebases because types are now analysed by the lint rule and types like $ReadOnly are global and have no global declared by ESLint. So no-undef should ideally be turned off in the recommended config. Flow checks this anyway, so it's a superfluous rule.

@Brianzchen
Copy link
Member

Thanks, I've been sick these past few days but I'll look into this next weekend once I'm better

@mrtnzlml
Copy link
Contributor

mrtnzlml commented Jun 18, 2022

@bradzacher Would you happen to have more information about the hermes-eslint parser? The only thing I could find is https://www.npmjs.com/package/hermes-eslint and it's not very welcoming. It doesn't seem to be open-sourced, and it doesn't explain very much what is it and what is the motivation to use it (over traditional parsers commonly used nowadays). Thanks! :)

Edit: is it related to https://github.com/facebook/hermes? I am lost on the React Native part. 🤔

@bradzacher
Copy link
Author

bradzacher commented Jun 18, 2022

Sorry, we probably could spruce up the docs.. Or rather add any docs at all heh. I'll look at doing that next week!

Hermes is Meta's JavaScript VM. It's primarily used to power react native.
As it's a VM, it also has a parser.

This parser is built with first-class support of flow syntax and its designed with performance in mind.

hermes-eslint (like @babel/eslint-parser) is a package designed to be the parser for ESLint. It uses Hermes' parser (compiled to WASM) to parse the code and outputs the AST in the latest ESTree format (I.e. It is fully compatible with ESLint v8+).

Unlike babel, hermes-eslint also includes a fully featured scope analyser which has full support for flow features - including types!!
This means (as I mentioned in the OP) we don't need extra hacky compatibility rules to teach eslint core rules about flow types or JSX!

At Meta we have been using hermes-eslint to power our linting in our largest repo for about a year. For context that is well over 550k files. This week I also migrated another repo to use it, which is another few hundred thousand files. So it's well and truly battle tested and hardened to say the least!

In terms of using it - nothing special is required. Just install the package and set parser: 'hermes-eslint' in the eslint config!

@mrtnzlml
Copy link
Contributor

Thanks, @bradzacher! Want to give it a try! 👍 Is facebook/hermes the correct repo for reporting bugs/asking questions? I've tried it in my monorepo, and I encountered some bumps (some parsing errors, unclear how to deal with Flow enums); you can see it here: adeira/universe#4583

@bradzacher
Copy link
Author

bradzacher commented Jun 18, 2022

@mrtnzlml
The import.meta support is missing (TBH I didn't know flow supported it!!!)
I'll talk to the hermes team about adding support for it.

For the no-unused-vars error on your enums - that's actually a completely correct error that you'll now get! Because hermes-eslint has a scope analyser that understands flow - it can keep track of things like types and enums as well as where they're used.
So in your file the three enums StatusDefault2, StatusString2 and StatusNumber2 are all not used anywhere - hence they're being flagged by no-unused-vars!

@mrtnzlml
Copy link
Contributor

For the no-unused-vars error on your enums - that's actually a completely correct error that you'll now get! Because hermes-eslint has a scope analyser that understands flow - it can keep track of things like types and enums as well as where they're used.

Ah, right, I didn't realize this. Very cool! 😊 Thanks for your help!

@Brianzchen
Copy link
Member

Hi @bradzacher doing some testing here, works pretty well on my source code but when I parse my dist (which people normally don't) with hermes-eslint it crashes but with @babel/eslint-parser it does not

Screen Shot 2022-06-25 at 11 47 56 am

Also going through the limited docs

your project you must specify "hermes-eslint" as the "parser"

But the code snippet references "parser": "hermes-parser" this is correct? Not quite sure which it's meant to be

@bradzacher
Copy link
Author

If you can isolate the code that's crashing then I can get this fixed up!

It looks like it was a property.
If you run eslint with --debug I think it'll show more info to help pinpoint?


The docs are wrong sorry. I forgot to go and get them update this week sorry. I'll make a note to do it on Monday.

@Brianzchen
Copy link
Member

@bradzacher just fyi I haven't forgotten this change! I'm just waiting for doc updates before I start digging into this further

@bradzacher
Copy link
Author

weird - I did cleanup the docs, but for some reason they didn't publish with the release.
I'm working on another release today so i'll make sure the docs update is shipped with it

@bradzacher
Copy link
Author

Just published v0.9.0 with the updated docs.
also @mrtnzlml this release includes import.meta support

@bradzacher
Copy link
Author

Also as an aside, @Brianzchen, we could convert this repo to use flow if you wanted.
I've got flow types for ESLint based off of the types included in the hermes-estree package.

Internally at meta we have over 400 rules written with flow types! It's a much nicer experience authoring ESLint rules with types 😄!

bradzacher added a commit to bradzacher/eslint-plugin-ft-flow that referenced this issue Jul 19, 2022
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
@pascalduez
Copy link
Member

Hey there,

I trying out a migration from @babel/eslint-parser to hermes-eslint, and eslint-plugin-flowtype to eslint-plugin-ft-flow.

I had to disable the ft-flow/space-after-type-colon rule in addition to those mentioned above. It raises errors in place where it should not.

For instance on line breaks:

foo?:
  | Foo
  | Bar

@Brianzchen
Copy link
Member

@pascalduez working on the upgrade now. Looks like there are some rules that definitely need fixing and on top of that will need to run assertions tests on both babel and hermes...

https://github.com/flow-typed/eslint-plugin-ft-flow/actions/runs/5697988866/job/15445355957?pr=41#step:10:3761

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 a pull request may close this issue.

4 participants