microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.16k stars 12.38k forks source link

[isolatedDeclarations] An option like disableSourceOfProjectReferenceRedirect, but configurable on the referenced project's end #58692

Open MichaelMitchell-at opened 3 months ago

MichaelMitchell-at commented 3 months ago

🔍 Search Terms

isolated declarations disableSourceOfProjectReferenceRedirect editor vscode tsserver

✅ Viability Checklist

⭐ Suggestion

disableSourceOfProjectReferenceRedirect is an option that improves performance by making it so that files can be typechecked using source files in referenced projects for type information rather than declaration files. I'm interested in a new option which enables this behavior only for projects that have isolated declarations enabled.

cc @sheetalkamat @andrewbranch - I'd be interested in working on this if this seems valuable.

📃 Motivating Example

With isolated declarations, it's now feasible to run a watcher in the background which can emit declarations quickly in response to changes. For projects with isolated declarations enabled, their dependent projects might be able to achieve a similar level of responsiveness by just reading from declaration files rather than from source files if there is a watcher emitting changes running in the background.

In a composite project where all sub-projects have isolatedDeclarations enabled, it would suffice to just enable disableSourceOfProjectReferenceRedirect for all projects, run watchers for each project, and reap all the benefits, but that might be suboptimal if there's a mix of projects with and without isolatedDeclarations enabled. This state can arise from there being a transition period as projects are being migrated.

A hybrid approach that might have the best of both worlds would be to enable the behavior of disableSourceOfProjectReferenceRedirect but only for the referenced projects that don't have isolated declarations enabled. To me, this would be more natural to specify on the referenced project, where it is effectively saying, "don't use my source files for typechecking, buddy", rather than on the dependent project. Regardless, I'll try to enumerate some concrete options:

Option A: New option on referenced project disallowRedirectingToSources (default: false) - Prevent projects that reference this project from using the sources of this project for typechecking, via TSServer, in the editor. Can be used orthogonally with isolatedDeclarations but if it somehow eases implementation, could be restricted to usable only when isolatedDeclarations is enabled.

Option B: New value for existing disableSourceOfProjectReferenceRedirect option on dependent project disableSourceOfProjectReferenceRedirect: "nonIsolatedDeclarations"

Option C: New option on dependent project I don't feel like thinking of a name for this flag since I don't think it's a good idea anyway and it's annoying for end-users to have to consider how this flag interacts with others like disableSourceOfProjectReferenceRedirect.

💻 Use Cases

Better editing experience for large composite projects with a mix of isolated declarations usage.

sheetalkamat commented 3 months ago

We are not too for adding flags for things that can be achieved with existing flags. Given that this is specific scenario: isolatedDeclarations need to be on and watcher should be running, it's really user's decision on how and when to enable disableSourceOfProjectReferenceRedirect flag. By default, this is false and the advantage of that is it includes source files and does not depend on d.ts files. This also helps with "unsaved" contents to be reflected correctly which watcher doesn't handle because it can only watch for saved changes. So I am not sure this is desirable. If someone is going to add a new flag to their tsconfig, might as well add the existing one after making conscious choice of the references and setup. It is not clear why new flag is more beneficial. Also note that if you are relying on "d.ts" files you also need to enable declarationMap on the build since otherwise we cant jump files to source correctly.

MichaelMitchell-at commented 3 months ago

@sheetalkamat What a new flag achieves can't be achieved with disableSourceOfProjectReferenceRedirect. If project A has references to project B and C, then enabling disableSourceOfProjectReferenceRedirect for project A causes it to include source files for both B and C. There's no way to only include source files for B or only source files for C.

sheetalkamat commented 3 months ago

I dont think we want this based on referenced project at all.. Its either true for your project where we will include all the declarations files instead of source files from all referenced projects or not depending on the option. This helps in providing seemless approach even if you arent referencing project directly and there is too much project management involved with little ROI to do this per project.

MichaelMitchell-at commented 3 months ago

there is too much project management involved

Too much project management for whom? At Airtable we use a build system, Bazel, to generate tsconfig files so we would just automatically enable the new flag for any project with isolated declarations

with little ROI to do this per project.

Can you explain what the ROI is? I don't understand the technical details of TSServer well enough to quantify this. One of our primary concerns is memory usage of TSServer. Right now TSServer often consumes over 4 GB of memory, which means we have to run TSServer with Node instead of a VSCode Electron process, which is much faster than Node because it enables pointer compression, but limits it to only 4GB of memory (more details here). So anything that can help us get back to under 4 GB would lead to a devex huge improvement.

sheetalkamat commented 3 months ago

Too much project management for whom? At Airtable we use a build system, Bazel, to generate tsconfig files so we would just automatically enable the new flag for any project with isolated declarations

This is for tsserver where the host and program and project need to keep track of this option per referenced project instead and manage all the host functions like file existence check etc.

Can you explain what the ROI is? I don't understand the technical details of TSServer well enough to quantify this. One of our primary concerns is memory usage of TSServer. Right now TSServer often consumes over 4 GB of memory, which means we have to run TSServer with Node instead of a VSCode Electron process, which is much faster than Node because it enables pointer compression, but limits it to only 4GB of memory (https://github.com/microsoft/vscode/issues/127105). So anything that can help us get back to under 4 GB would lead to a devex huge improvement.

ROI i dont see anything different from having the current option. The pros of enabling that option is you may endup not loading more files depending on what your d.ts looks like but as soon as you call go to definition, you do end up loading the project and hence the source files as well. The cons of enabling usage of "d,ts" instead of "ts" is that you dont see edits reflected until you have saved and built. Note that when you use d.ts files, the referenced project if opened any file will have source files instead of d.ts files and your project will have d.ts files so you are anyways increasing memory pressure there depending on what user clicks. So there is not much advantage of setting that current flag as its not very functional. As i said you also need to generate source maps and thats additional data in memory so from memory perspective i dont think you gain at all or if any it will be very small.

MichaelMitchell-at commented 3 months ago

That explanation makes sense, thanks!