Elide Unused Re-exports In TSServer

[gluxon]

This is a proof of concept pull request to gather initial feedback. It should not merge and close after some discussion. I’m mostly looking for comments on whether this is viable right now.

The change applies a simple heuristic for excluding some source files from dependencies when tsserver traverses a program’s module dependency graph. If a dependency “re-exports” a file with the export { ... } from "..." syntax, the file of that export will not be added to Program if none of the identifiers are used.

Suppose a package example uses only SOME_CONST_A from example-dep.

// packages/example-dep/index.ts
export { SomeComponentA } from "./components/SomeComponentA";
export { SomeComponentB } from "./components/SomeComponentB";
export { someUtilA } from "./utils/someUtilA";
export { someUtilB } from "./utils/someUtilB";
export { SOME_CONST_A, SOME_CONST_B } from "./components/consts";

// packages/example/index.ts
import { SOME_CONST_A } from "@ref/example-dep"

// packages/example/tsconfig.json
{
  "compilerOptions": {
    "composite": true,
    "baseUrl": "./",
    "paths": {
       "@ref/a": [ "../a" ]
    }
  },
  "references": [
    "path": "../example-dep"
  ]
}

In this case files from components and utils are no longer added. This can create big savings in memory and performance if components/utils loaded large dependencies themselves.

Current Issues

• Elided identifiers no longer appear in completion responses. In the example above, SomeComponentA would not show while working on packages/example/index.ts. (More on this later.)
• The current implementation in this PR doesn’t add elided files to the program later if the identifier is added.
• Haven’t written any new tests for this yet.

Questions

I’m curious if the TypeScript team is willing to work with me on this change. Most of the problems with this involve handling identifiers defined in elided source files.

1. After this change, identifiers from elided source files don’t appear in completion responses. Do they need to? (I assume the answer is yes, but wanted to verify.)
   1. Despite not appearing in completions, they do appear in the "Quick Fix..." menu for auto-imports.
2. If the elided identifiers are re-added, they won’t have full typing information in the completion prompt initially. We could possibly fetch this when a user hovers hover the entry though. Is that an acceptable trade-off?
3. Is the function call stack the best way to carry over Identifiers being searched for? This made calls to processImportedModules a bit ugly.

[ghost]

All CLA requirements met.

[amcasey]

Reminds me of this change: aws/aws-sdk-js#3398

[amcasey]

I agree that being able to pull in some files without pulling in all files is a huge perf win, but I'd like to hear from someone more knowledgeable that this is a good way to achieve it. I would have guessed it had more to do with the shape of the library being imported.

@gluxon Do you have numbers? Did you apply your change to a sample project and see a big perf win?

[gluxon]

Thanks for giving initial thoughts @amcasey!

I agree that being able to pull in some files without pulling in all files is a huge perf win, but I'd like to hear from someone more knowledgeable that this is a good way to achieve it.

Happy to wait for additional thoughts. The way I did it in this PR definitely needs improvements. I wrote the V0 here quickly to see what the perf differences would be and to check whether obvious problems would get caught by tests.

There don't seem to be any tests failures, which I was surprised by. I think this is mostly because the files elided here are coming from project references which didn't get source code added until TypeScript 3.7 recently.

I would have guessed it had more to do with the shape of the library being imported.

Definitely. I should add that this improved editor startup times in our monorepo specifically because we have a lot of index.ts files from packages re-exporting its internals. Commonly new packages import from a shared React components package but would only need a few of the many in there.

@gluxon Do you have numbers? Did you apply your change to a sample project and see a big perf win?

There's a large package on my team that got its createProgram time cut to 1/3 of the original time. (~30s to ~11s). Another large package we've had problems with went from ~28s to ~20s. I didn't give numbers initially because the differences depend heavily on project reference structure.

[andrewbranch]

It’s pretty cool that you were able to get a proof of concept working with such a small change, and it would be awesome if we could safely do something like this. Unfortunately, it could very easily break a build, and there’s (currently) no way to know whether or not it would in a particular instance without doing most of the work of a full build.

There is an exact analogy to how bundlers opt into this same behavior of dead code elimination or live code inclusion—because loading JavaScript modules may have important side effects, they can’t do this in general; they have to know that the code they are omitting is “pure.” So, they read the pure field in package.json to opt into this behavior on a per-package basis. Similarly, loading a TypeScript file into a program may have side effects on the program state: the file may introduce globals, ambient module declarations, or module augmentations. In your example, it’s clear that only SOME_CONST_A is imported, but that isn‘t sufficient to tell us that it’s the only thing from @ref/example-dep that is used. If we omitted all the other modules imported by the index file, it’s very possible that we would be omitting global declarations that are referenced in files we are including.

Of course, you’re absolutely right that this behavior would often work and would often have significant benefits. It makes me wonder if, extending the bundler analogy, we could let individual packages indicate that their typings are “pure,” and enable this behavior on a per-package basis. If we had something like that, we’d probably want the authors of those packages to be able to (or have to) validate their claim that their declarations are side-effect-free: modules would not be allowed to import scripts, declare globals, or augment other modules.

It’s also worth clarifying that what you’ve done here so far is not specific to TS Server, despite the PR title—which is a good thing. Including a different set of modules between tsc and the language service / TS Server is a non-starter. (You’d wind up in situations where your editor shows errors but command-line compilation fails, or vice versa.) As another implementation note (which is probably not super relevant at this stage), suppose that your index file used a series of export * from, which I think is more common for barrels, instead of using export specifiers—I can’t tell at a glance if this proof of concept would handle that, but I think perhaps it wouldn’t? I think in a hypothetical world where we could implement something like this, the implementation would get a bit more complicated. It’s an interesting experiment though, so thanks for getting a conversation started!

[gluxon]

Thanks for that fantastic in-depth explanation. We've been relying on webpack tree-shaking to reduce bundle sizes on my team, so this PR was definitely inspired by that. I wasn't aware of side-effects to the typing system through globals and module augmentations, but I see it's well-documented on typescriptlang.org and makes sense. Thanks for calling that out. It did feel like I was missing something that made this PR too easy. 😅

Of course, you’re absolutely right that this behavior would often work and would often have significant benefits. It makes me wonder if, extending the bundler analogy, we could let individual packages indicate that their typings are “pure,” and enable this behavior on a per-package basis. If we had something like that, we’d probably want the authors of those packages to be able to (or have to) validate their claim that their declarations are side-effect-free: modules would not be allowed to import scripts, declare globals, or augment other modules.

I'd love to explore this if the team is open to it. It sounds like the specific requirements here are to add a new flag and error on side-effectful TypeScript statements if they're seen. I'll file a new issue linking your comment to discuss that prerequisite further.

It’s also worth clarifying that what you’ve done here so far is not specific to TS Server, despite the PR title—which is a good thing. Including a different set of modules between tsc and the language service / TS Server is a non-starter. (You’d wind up in situations where your editor shows errors but command-line compilation fails, or vice versa.)

Yes, you're right. I was specifically aiming for a tsserver improvement but this does affect tsc after rethinking. My thought was that tsc would always load root names, but dependencies are still affected in general.

Ensuring errors are consistent between compilation and editing makes sense. 👍

As another implementation note (which is probably not super relevant at this stage), suppose that your index file used a series of export * from, which I think is more common for barrels, instead of using export specifiers—I can’t tell at a glance if this proof of concept would handle that, but I think perhaps it wouldn’t? I think in a hypothetical world where we could implement something like this, the implementation would get a bit more complicated.

Yup, you're right again. This PR doesn't handle export * from syntax because that would require peeking into the file itself (recursively) to see what it exports. The PR would be much larger if it handled that.

It’s an interesting experiment though, so thanks for getting a conversation started!

I really appreciate you and everyone else being receptive. This is a great open source community. Thanks again.

[gluxon]

I'll file a new issue linking your comment to discuss that prerequisite further.

#41104

Going to close this experiment for now.