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

feat: allow user to provide TS program instance in parser options #3484

Merged
merged 19 commits into from Jun 8, 2021

Conversation

uniqueiniquity
Copy link
Contributor

typescript-estree has complex handling for constructing and managing TS program instances in order to provide type info for semantic rules. However, for one-off invocations on large codebases (e.g., in CI), this functionality isn't valuable, and the overhead is substantial.

Here, we introduce a new parser option program that allows users to programmatically provide their own TS program instance that will be used in rules for type info. This is based on TSLint's functionality.

On a real-world codebase with 30,000+ files, this reduced the linting time from ~60 minutes to ~10 minutes. 🥳

For user convenience, we export a utility function createProgram from @typescript-estree/parser that creates a TS program instance from a TypeScript configuration file. This is something that TSLint provided as well.

In this initial implementation, we only allow for a single program to be provided. However, this could be augmented in the future to support multiple programs.

Fixes #1442

Supersedes #3481 due to branch casing issues 😫

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @uniqueiniquity!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

@codecov
Copy link

codecov bot commented Jun 5, 2021

Codecov Report

Merging #3484 (8892427) into master (dac8845) will decrease coverage by 0.02%.
The diff coverage is 86.66%.

❗ Current head 8892427 differs from pull request most recent head ae03746. Consider uploading reports for the commit ae03746 to get more accurate results

@@            Coverage Diff             @@
##           master    #3484      +/-   ##
==========================================
- Coverage   92.65%   92.63%   -0.03%     
==========================================
  Files         324      325       +1     
  Lines       11191    11226      +35     
  Branches     3158     3163       +5     
==========================================
+ Hits        10369    10399      +30     
- Misses        365      367       +2     
- Partials      457      460       +3     
Flag Coverage Δ
unittest 92.63% <86.66%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...pt-estree/src/create-program/useProvidedProgram.ts 83.33% <83.33%> (ø)
...ges/typescript-estree/src/create-program/shared.ts 88.88% <92.30%> (+1.01%) ⬆️
...-estree/src/create-program/createProjectProgram.ts 94.11% <100.00%> (+1.43%) ⬆️

@bradzacher
Copy link
Member

On a real-world codebase with 30,000+ files, this reduced the linting time from ~60 minutes to ~10 minutes

Wow. Really?
So that's probably the biggest cause of slowness in our parser then.
That pretty much seals the deal on me wanting to add a environment variable to make this available to anyone without the need for custom scripts.

I never even knew that the watch program was so darn slow!

@bradzacher bradzacher added the enhancement New feature or request label Jun 5, 2021
@uniqueiniquity
Copy link
Contributor Author

uniqueiniquity commented Jun 5, 2021

@bradzacher Really really. I need to get another clean run to truly verify, since the 7 minute run I had earlier had some issues with some of the TSLint rules being used via eslint-plugin-tslint (caused by resolving two different versions of typescript). But yeah, I think there's a ~100ms cost per-file involved with retrieving the program instance from the watch program, which really builds up over a project of this scale. I'm not really sure what causes that cost, though.

@uniqueiniquity
Copy link
Contributor Author

@bradzacher - After fixing the eslint-plugin-tslint issue, a run took 15 minutes, 6.5 of which were spent creating the program instance. So not quite as the earlier run, but still compellingly better.

Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mostly LGTM - a few comments.
otherwise happy to get this out there

@bradzacher bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Jun 7, 2021
@bradzacher bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Jun 8, 2021
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
I just merged the dependabot PRs.
I'll update the branch and we can merge when the checks pass.

I did the dependabot PRs first so there was commits on the stack so this canary will work properly (long story).

@uniqueiniquity
Copy link
Contributor Author

Thank you so much!!

@bradzacher bradzacher merged commit e855b18 into typescript-eslint:master Jun 8, 2021
@uniqueiniquity uniqueiniquity deleted the provideTSProgram branch June 8, 2021 17:27
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create an API to "seed" the parser with an existing program
2 participants