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

uniqueiniquity · 2021-06-03T20:04:40Z

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.

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

typescript-eslint · 2021-06-03T20:04:45Z

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 · 2021-06-03T22:01:39Z

Codecov Report

Merging #3481 (b1f6213) into master (fc5f171) will increase coverage by 0.00%.
The diff coverage is 95.45%.

@@           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

Flag	Coverage Δ
unittest	`92.67% <95.45%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
...ges/typescript-estree/src/create-program/shared.ts	`88.37% <90.00%> (+0.49%)`	⬆️
...-estree/src/create-program/createProjectProgram.ts	`94.11% <100.00%> (+1.43%)`	⬆️
...pt-estree/src/create-program/useProvidedProgram.ts	`100.00% <100.00%> (ø)`

bradzacher

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.

bradzacher · 2021-06-03T22:58:41Z

packages/typescript-estree/package.json

@@ -64,6 +64,9 @@
    "jest-specific-snapshot": "*",
    "make-dir": "*",
    "tmp": "^0.2.1",
+    "typescript": "^4.3.2"


keep this as *
We use the root package.json to control the TS version installed in the repo.

Suggested change

"typescript": "^4.3.2"

"typescript": "*"

bradzacher · 2021-06-03T23:01:17Z

packages/typescript-estree/package.json

@@ -64,6 +64,9 @@
    "jest-specific-snapshot": "*",
    "make-dir": "*",
    "tmp": "^0.2.1",
+    "typescript": "^4.3.2"
+  },
+  "peerDependencies": {


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

bradzacher · 2021-06-03T23:04:03Z

packages/types/package.json

@@ -48,5 +48,8 @@
        "_ts3.4/*"
      ]
    }
+  },
+  "devDependencies": {
+    "typescript": ">=2.6.0"


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

Suggested change

"typescript": ">=2.6.0"

"typescript": "*"

bradzacher · 2021-06-03T23:04:23Z

packages/types/src/parser-options.ts

@@ -1,4 +1,5 @@
 import { Lib } from './lib';
+import { Program } from 'typescript';


lets ensure that this isn't ever pulled in as a runtime dep on this package.

Suggested change

import { Program } from 'typescript';

import type { Program } from 'typescript';

bradzacher · 2021-06-03T23:07:23Z

packages/typescript-estree/src/create-program/useProvidedProgram.ts

+  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}`,


The filepath should be made relative.
This ensures that error messages are kept is stable and portable.

typescript-eslint/packages/typescript-estree/src/create-program/createProjectProgram.ts

Lines 52 to 55 in fc5f171

`The file does not match your project config: ${path.relative(

extra.tsconfigRootDir || process.cwd(),

extra.filePath,

)}.`,

bradzacher · 2021-06-03T23:09:17Z

packages/typescript-estree/src/parser.ts

+    );
+  }
+
+  if (extra.program === null) {


nit - just for safety so we can handle the undefined case as well, in case something somewhere is wacky.

Suggested change

if (extra.program === null) {

if (extra.program == null) {

I made this if (!extra.program)

bradzacher · 2021-06-03T23:10:10Z

packages/typescript-estree/src/parser.ts

-    options.project,
-    projectFolderIgnoreList,
-  );
+  if (typeof options.program === 'object') {


nit - we don't want to log if the user explicitly passes null here.

Suggested change

if (typeof options.program === 'object') {

if (typeof options.program === 'object' && options.program != null) {

I made this if (options.program && typeof options.program === 'object')

uniqueiniquity · 2021-06-03T23:38:25Z

@bradzacher I'm about to try it on a large project - will report back alongside addressing your suggestions.

uniqueiniquity · 2021-06-05T00:24:19Z

Closing in favor of #3484 - I screwed up my branch casing and couldn't fix it 😫
I've addressed all comments currently on this PR and added perf results on the new PR - spoiler alert, they're good.

Ben Lichtman added 5 commits June 3, 2021 10:47

feat(typescript-estree): allow ts program to be provided in options

7c4f35d

chore(types): add program option to ParserOptions

4c6aca9

test(typescript-estree): add tests for program option

f07d72e

test(parser): add test for program option

e4cbd2c

docs: document new program option

648caaf

uniqueiniquity requested a review from bradzacher June 3, 2021 20:04

Ben Lichtman added 2 commits June 3, 2021 14:03

fix(typescript-estree): fix condition in option application

defed5d

test(typescript-estree): stop using machine-specific snapshot

b1f6213

bradzacher added the enhancement New feature or request label Jun 3, 2021

bradzacher requested changes Jun 3, 2021

View reviewed changes

bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Jun 3, 2021

uniqueiniquity mentioned this pull request Jun 5, 2021

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

Merged

uniqueiniquity closed this Jun 5, 2021

github-actions bot locked as resolved and limited conversation to collaborators Jul 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

uniqueiniquity commented Jun 3, 2021 •

edited by bradzacher

Loading

typescript-eslint bot commented Jun 3, 2021

codecov bot commented Jun 3, 2021 •

edited

Loading

bradzacher left a comment

bradzacher Jun 3, 2021

bradzacher Jun 3, 2021

bradzacher Jun 3, 2021

bradzacher Jun 3, 2021

bradzacher Jun 3, 2021

bradzacher Jun 3, 2021

uniqueiniquity Jun 5, 2021

bradzacher Jun 3, 2021

uniqueiniquity Jun 5, 2021

uniqueiniquity commented Jun 3, 2021

uniqueiniquity commented Jun 5, 2021

		@@ -1,4 +1,5 @@
		import { Lib } from './lib';
		import { Program } from 'typescript';

	import { Program } from 'typescript';
	import type { Program } from 'typescript';

	`The file does not match your project config: ${path.relative(
	extra.tsconfigRootDir \|\| process.cwd(),
	extra.filePath,
	)}.`,

	if (typeof options.program === 'object') {
	if (typeof options.program === 'object' && options.program != null) {

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

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

Conversation

uniqueiniquity commented Jun 3, 2021 • edited by bradzacher Loading

typescript-eslint bot commented Jun 3, 2021

codecov bot commented Jun 3, 2021 • edited Loading

Codecov Report

bradzacher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uniqueiniquity commented Jun 3, 2021

uniqueiniquity commented Jun 5, 2021

uniqueiniquity commented Jun 3, 2021 •

edited by bradzacher

Loading

codecov bot commented Jun 3, 2021 •

edited

Loading