feat: allow user to provide TS program instance in parser options #3481
Conversation
|
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 Report
@@ Coverage Diff @@
## master #3481 +/- ##
=======================================
Coverage 92.66% 92.67%
=======================================
Files 324 325 +1
Lines 11188 11201 +13
Branches 3157 3158 +1
=======================================
+ Hits 10367 10380 +13
Misses 365 365
Partials 456 456
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
i didn't review the tests - just glanced through the code.
looks good! a few comments.
have you tested this in a real-world lint setup?
I'd be interested to know how much this improves the perf for them.
Also would like to triple check that ESLint doesn't mangle the program instance or anything.
| @@ -64,6 +64,9 @@ | |||
| "jest-specific-snapshot": "*", | |||
| "make-dir": "*", | |||
| "tmp": "^0.2.1", | |||
| "typescript": "^4.3.2" | |||
There was a problem hiding this comment.
keep this as *
We use the root package.json to control the TS version installed in the repo.
| "typescript": "^4.3.2" | |
| "typescript": "*" |
| @@ -64,6 +64,9 @@ | |||
| "jest-specific-snapshot": "*", | |||
| "make-dir": "*", | |||
| "tmp": "^0.2.1", | |||
| "typescript": "^4.3.2" | |||
| }, | |||
| "peerDependencies": { | |||
There was a problem hiding this comment.
We have an implicit peer dependency via the peerDependenciesMeta property.
The reason we don't explicitly declare the peer dependency is because on older versions of yarn and npm, they do not understand peerDependenciesMeta.
So if we add an explicit peer dep - then they will log warnings at install time.
In the past this annoyed many users 😅 that had our packages installed (but not used) transitively.
TL;DR - remove the explicit peer dep
| @@ -48,5 +48,8 @@ | |||
| "_ts3.4/*" | |||
| ] | |||
| } | |||
| }, | |||
| "devDependencies": { | |||
| "typescript": ">=2.6.0" | |||
There was a problem hiding this comment.
We use the root package.json to control the TS version installed in the repo.
You can use the peerDependenciesMeta to add an optional peer dep on typescript, which we'll need to do to ensure package managers can understand the new requirement
| "typescript": ">=2.6.0" | |
| "typescript": "*" |
| @@ -1,4 +1,5 @@ | |||
| import { Lib } from './lib'; | |||
| import { Program } from 'typescript'; | |||
There was a problem hiding this comment.
lets ensure that this isn't ever pulled in as a runtime dep on this package.
| import { Program } from 'typescript'; | |
| import type { Program } from 'typescript'; |
| if (!astAndProgram) { | ||
| const errorLines = [ | ||
| '"parserOptions.program" has been provided for @typescript-eslint/parser.', | ||
| `The file was not found in the provided program instance: ${extra.filePath}`, |
There was a problem hiding this comment.
The filepath should be made relative.
This ensures that error messages are kept is stable and portable.
| ); | ||
| } | ||
|
|
||
| if (extra.program === null) { |
There was a problem hiding this comment.
nit - just for safety so we can handle the undefined case as well, in case something somewhere is wacky.
| if (extra.program === null) { | |
| if (extra.program == null) { |
There was a problem hiding this comment.
I made this if (!extra.program)
| options.project, | ||
| projectFolderIgnoreList, | ||
| ); | ||
| if (typeof options.program === 'object') { |
There was a problem hiding this comment.
nit - we don't want to log if the user explicitly passes null here.
| if (typeof options.program === 'object') { | |
| if (typeof options.program === 'object' && options.program != null) { |
There was a problem hiding this comment.
I made this if (options.program && typeof options.program === 'object')
bradzacher
added
the
awaiting response
|
@bradzacher I'm about to try it on a large project - will report back alongside addressing your suggestions. |
|
Closing in favor of #3484 - I screwed up my branch casing and couldn't fix it 😫 |
typescript-estreehas 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
programthat 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.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