babel / babel-eslint

:tokyo_tower: A wrapper for Babel's parser used for ESLint (renamed to @babel/eslint-parser)
https://github.com/babel/babel/tree/main/eslint/babel-eslint-parser
MIT License
2.96k stars 208 forks source link

Feature idea: have a way for eslint-plugin-prettier to share an AST with eslint #836

Closed lencioni closed 4 years ago

lencioni commented 4 years ago

I'm not sure if this is the best place to open this issue, given the migration to the babel monorepo. Please let me know if you'd like me to post this somewhere else.

Adding eslint-plugin-prettier to your ESLint config currently adds a lot of overhead to ESLint runs. On larger codebases, this can be a considerable performance hit.

One of the main costs here comes from the fact that both ESLint and Prettier need to parse each file to an AST separately. I think we could see a performance improvement by providing a way for the AST to be shared between these two programs.

Thankfully, both ESLint and Prettier allow for a custom parser to be used.

babel-eslint uses babel to parse the code to an AST, and then runs some functions to mutate that AST to make it look right for ESLint. That code is here: https://github.com/babel/babel-eslint/blob/187e69aa8942039a5ae91cb9ac3a7998417e935e/lib/parse.js#L57-L72 Because of this mutation, it seems like we might not be able to pass it directly to Prettier without it erroring.

So, I think that if we could make it so the original parsed AST is cached for a short amount of time (maybe WeakMap or a simple LRU cache so we are kind to memory usage), and provide a babel-eslint/prettier entrypoint that can be used by Prettier that pulls from this cached original Babel AST, we could avoid the double parsing.

I think the main downside with this is that it would require us to copy the AST instead of mutating it directly, which would come with its own performance cost in both CPU and memory usage.

One alternative might be to see if we can teach Prettier to handle the babel-eslint mutated AST, and then simply add some caching to the parser here. I'm not very familiar with Prettier's internals, so I'm not sure how much effort that would be or whether it would be possible with the babel-eslint mutated AST, but if it is possible that seems like it would be a good option worth considering. It looks like typescript-estree is able to support both ESLint and Prettier, so maybe this isn't too wild of an idea? https://github.com/typescript-eslint/typescript-eslint/tree/master/packages/typescript-estree

cc @BPScott

nicolo-ribaudo commented 4 years ago

Would it be possible to parse with @babel/parser, then run Prettier on the generated AST, and then let babel-eslint mutate that AST?

kaicataldo commented 4 years ago

I'm definitely not against exploring this, but I don't think this can be a very high priority issue for the core team. My top priority right now is to make sure that the new @babel/eslint-parser and @babel/eslint-plugin packages are behaving correctly.

FWIW, typescript-eslint is used as the underlying parser for the Prettier, but I don't believe it parses once and passes the AST around to different tools.

kaicataldo commented 4 years ago

Ideally all of our tools would be able to parse once and pass the AST through to the next tool in the chain. I'd love to be able to get there as a community, but it does seem a long way off since we currently can't even decide on a single AST spec to use :grimacing:.

kaicataldo commented 4 years ago

Thank you for the PR. Now that @babel/eslint-parser has been released, we are making this repository read-only. If this is a change you would still like to advocate for, please reopen this in the babel/babel monorepo.