8000 Enhancement: Try using tsserverlibrary's ProjectService to handle type-aware mode · Issue #6575 · typescript-eslint/typescript-eslint · GitHub
[go: up one dir, main page]
Skip to content

Enhancement: Try using tsserverlibrary's ProjectService to handle type-aware mode #6575

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.

Sign up for GitHub

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

Jump to bottom
Closed
4 tasks done
jakebailey opened this issue Mar 6, 2023 · 9 comments · Fixed by #6754
Closed
4 tasks done

Enhancement: Try using tsserverlibrary's ProjectService to handle type-aware mode #6575

jakebailey opened this issue Mar 6, 2023 · 9 comments · Fixed by #6754
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement New feature or request package: typescript-estree Issues related to @typescript-eslint/typescript-estree performance Issues regarding performance

Comments

@jakebailey
Copy link
Collaborator

Before You File a Proposal Please Confirm You Have Done The Following...

Relevant Package

typescript-estree

My proposal is suitable for this project

  • I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).

Description

This is a follow-up to #6172 and the TypeScript / ESLint meeting we had last week.

Currently, type-aware mode uses ts.Program to do its work. There are some problems with this approach:

  • This mode is not references aware, leading to the need to either put every file into one program, or manually specify all programs (https://typescript-eslint.io/linting/typed-linting/monorepos).
  • With multiple programs in play, lots of work can be duplicated as projects can reference each other.
  • This project has to manage much of the lifecycle of Programs, and do a lot of patching to the program to make things work (e.g. not emit, make sure not to exit, etc). This seems to lead to known memory leaks and other problems.
  • Probably more I'm forgetting. I don't think I need list more 😄

#6172 takes the approach of using the LanguageService to handle this stuff. This is a good step, and one taken by projects like ts-morph. However, a single LanguageService itself will only handle one project at a time, so is only a partial fix.

My idea is to go further, and use tsserverlibrary's ProjectService instead. This is "the thing" that tsserver uses to load projects and respond to language server requests for the editor. The idea is that you can create a LanguageService, then act like a user does, opening files, making requests, closing files, and so on. This means:

  • Project references are handled "for free".
  • Users do not need to specify any tsconfig, just like how you don't specify anything when opening VS Code.
  • The lifecycle of programs and related language serivces is fully managed, hopefully fixing leaks.
  • Work shouldn't be duplicated, or at least not duplicated any more than anyone already experiences using TypeScript in their editors.

The general approach is as follows:

  • Import tsserverlibrary instead of typescript.
  • Create a ProjectService. Like everything else, it takes in a host, but nothing surprising.
  • To look at a file, call openClientFile to mark the file as open (potentially providing contents), and then call getScriptInfo on its path.
  • Drill down from ScriptInfo like getScriptInfo(filename).getDefaultProject().getLanguageService(/*ensureSynchronized*/ true) and so on until you get to Program / TypeChecker whatever else is needed.
  • Eventually, close the file.

There are some problems:

  • tsserverlibrary is an entirely different module than typescript. Will this break downstream rules which import typescript directly? Is that a thing people do? Do we have to move the API? (uh oh)
  • In CLI mode, this all seems fine, as the service does all of the work. But in the editor, is this too heavy given tsserver is already running? maybe it's not a problem becuase it's no worse than now.
  • If people are running one of the "parallel eslint" wrappers, will this cause resource usage problems? Or, again, is this no worse than the current state of things?

Fail

N/A

Pass

N/A

Additional Info

No response
@jakebailey jakebailey added enhancement New feature or request triage Waiting for team members to take a look labels Mar 6, 2023
@bradzacher bradzacher added package: typescript-estree Issues related to @typescript-eslint/typescript-estree performance Issues regarding performance accepting prs Go ahead, send a pull request that resolves this issue and removed triage Waiting for team members to take a look labels Mar 7, 2023
@JoshuaKGoldberg JoshuaKGoldberg mentioned this issue Mar 11, 2023
Closed
3 tasks
@JoshuaKGoldberg JoshuaKGoldberg mentioned this issue Mar 24, 2023
Merged
3 tasks
@JoshuaKGoldberg
Copy link
Member
JoshuaKGoldberg commented Mar 24, 2023 •
edited
Loading

@bradzacher and I prototyped this in #6754 and left some questions there. It seems to work , albeit with shockingly worse performance. 😄 quite nicely!

@DanielRosenwasser DanielRosenwasser mentioned this issue May 12, 2023
Closed
@jakebailey jakebailey mentioned this issue May 26, 2023
Closed
@me4502
Copy link
me4502 commented Jul 12, 2023

But in the editor, is this too heavy given tsserver is already running? maybe it's not a problem becuase it's no worse than now.

Is it possible for typescript-eslint to use the editor's tsserver instance if it exists? Or provide a way for an IDE extension such as vscode-eslint to provide access to the IDEs running tsserver instance?

@jakebailey
Copy link
Collaborator Author

Is it possible for typescript-eslint to use the editor's tsserver instance if it exists? Or provide a way for an IDE extension such as vscode-eslint to provide access to the IDEs running tsserver instance?

Even if there were some tsserver API to access everything, I'd think it'd be prohibitively slow to work with because eslint would be in a totally different process. You definitely don't want to be proxying everything across the wire.

The only way to get direct access would be to write a tsserver plugin (which may work), but it's not clear to me how that would tie into eslint unless eslint itself ran entirely within said plugin.

@bradzacher
Copy link
Member

There is a 3rr party that has created an LSP plugin for ESLint but yeah they had to reimplement much of the eslint extension within the plugin and convert all the eslint errors to diagnostics... It's not clean or easy.

@JoshuaKGoldberg
Copy link
Member

cc @Quramy for the https://github.com/Quramy/typescript-eslint-language-service perspective. Would love any thoughts you have! I've had a long-standing todo to reach out about this a week or two after we merge #6754.

@erictheswift
Copy link

Early user report:

When I enabled type-aware mode (configured parserOptions & added 2 dependent rules: consistent-type-exports, require-array-sort-compare) on 2600 typescript modules (220k LOC) codebase, I faced:

  • high memory pressure (OOMs actively began on a one peer machine with less memory resources / higher memory pressure, required --max-old-space-size override to workaround)
  • tremendously low performance (sometimes 20s timeouts when linting file changes via IntelliJ WebStorm embedded ESLint code quality tool).

Adding EXPERIMENTAL_useProjectService=true parserOption from @typescript-eslint/parser@6.1.0 improved IDE performance a lot (somewhat better performance, no OOMs).

One bug made me revert this flag as well as parserOptions & rules that is likely related here:
When created new typescript file (and likely when moved existing one) in WebStorm, eslint failed on this file until eslint tool disable+enable (eslint server restart internally?):

ESLint: Error: Error while loading rule '@typescript-eslint/consistent-type-exports':
You have used a rule which requires parserServices to be generated. You must therefore provide a value for the "parserOptions project" property for @typescript-eslint/parser.
Occurred while linting %NEW_FILE%.tsx
Error: Error while loading rule '@typescript-eslint/consistent-type-exports': You have used a rule which requires parserServices to be generated. You must therefore provide a value for the "parserOptions project" property for @typescript-eslint/parser.
Occurred while linting %NEW_FILE%.tsx
at Object.getParserServices (_/node_modules/@typescript-eslint/utils/dist/eslint-utils/getParserServices.js:24:15)
at create (_/node_modules/@typescript-eslint/eslint-plugin/dist/rules/consistent-type-exports.js:63:31)
at Obiect.create (_/node_modules/@typescript-eslint/utils/dist/eslint-utils/RuleCreator.js:38:20)
at createRuleListeners_/node_modules/eslint/lib/linter/linter.js:870:21)
at _/node_modules/eslint/lib/linter/linter.js:1040:110 at Array.forEach (<anonymous>)
at runRules(_/node_modules/eslint/lib/linter/linter.js:977:34)
at Linter._verify WithoutProcessors (_/node_modules/eslint/lib/linter/linter js:1330:31) at Linter._verifyWithConfigArray(_/node_modules/eslint/lib/linter/linter.js:1706:21)
at Linter.verify (_/node_modules/eslint/lib/linter/linter.js:1412:65)

Can be related to #6814?
Did not make any internal investigation.
WebStorm 2023.1.1 Build #WS-231.8770.64, built on April 27, 2023

        "eslint": "8.45.0",
        "@typescript-eslint/eslint-plugin": "6.1.0",
        "@typescript-eslint/parser": "6.1.0",

@erictheswift erictheswift mentioned this issue Jul 20, 2023
Closed
4 tasks
@JoshuaKGoldberg
Copy link
Member

Awesome, thanks for trying this out @erictheswift! Is this something you'd be able to (either or both of):

  • Let one or more of us take a look at? (we can sign any NDA you need if it's a private repo)
  • File a standalone issue with more info (the templates will ask you for a bunch) to help us investigate & track the report?

Really excited to see evidence of real life perf & memory improvements! 🚀

@jakebailey
Copy link
Collaborator Author

Another bit that I unfortunately only realized after the feature was merged; things might get "weird" when the "inferred" project is involved (this is exported as InferredProject, and can be checked for via Project.projectKind).

When tsserver doesn't have a tsconfig, it still needs to work on files that are open; these could be random TS/JS files that aren't normally built. That usually tends to cover things like dotfiles, TS's Herebyfile, etc. Now that ts-eslint can use the ProjectService, those files are likely to be opened in an inferred project.

However, the default settings for an inferred project are typically set by the client, e.g. VS Code. The upshot is that the settings that ts-eslint may have here could be different than what is seen in the editor. And indeed, that is likely happening already, because VS Code sets much more reasonable defaults, e.g. turning on strictNullChecks and strictFunctionTypes, at least (and, they can be overridden by the user).

Not quite sure what to do about that; potentially some new config that lets you set some compilerOptions in the eslint config, which ts-eslint then gives to setCompilerOptionsForInferredProjects?

Sidenote; this issue was closed when the PR was merged, but should we keep this issue open to gain feedback about this feature while it's being hacked on? Or is there some better mechanism to have these kinds of discussions?

@JoshuaKGoldberg
Copy link
Member

Or is there some better mechanism to have these kinds of discussions?

@bradzacher were chatting about this a bit - I think I'd like to let it sit for a version, then add explicit docs with a pinned discussion for it.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement New feature or request package: typescript-estree Issues related to @typescript-eslint/typescript-estree performance Issues regarding performance
Projects
None yet
Milestone
No milestone
5 participants
@me4502 @erictheswift @JoshuaKGoldberg @jakebailey @bradzacher
0