Skip to content
This repository has been archived by the owner on Aug 18, 2021. It is now read-only.

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

Closed
lencioni opened this issue Jun 12, 2020 · 4 comments

Comments

@lencioni
Copy link
Contributor

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:

let ast;
try {
ast = parse(code, opts);
} catch (err) {
if (err instanceof SyntaxError) {
err.lineNumber = err.loc.line;
err.column = err.loc.column;
}
throw err;
}
babylonToEspree(ast, traverse, tt, code);
return ast;
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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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 😬.

@kaicataldo
Copy link
Member

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants