Expand auto-import to all package.json dependencies #38923

andrewbranch · 2020-06-03T22:57:15Z

This PR creates an auxiliary program in the language service layer that contains node_modules packages that are specified as a direct dependency (including ~~devDependencies~~ peerDependencies) in a package.json file visible to the project’s root directory and not already present in the main program. This program is then supplied to the auto-imports machinery as a provider of additional modules that can be used for auto-import. Effectively, this means that packages that ship their own types should be available for auto-import immediately after npm installing them, lifting a long-despised limitation of “you have to write out the import manually once in your project before auto-imports will work, unless it’s in @types, because @types is special.”

Performance

I compared a few operations against master on the output of @angular/cli (with routing). (I chose this as a test because it’s a realistic scenario where a user’s package.json will contain more dependencies with self-shipped types than are actually used in the program. For example, @angular/forms is a listed dependency but isn’t used in the boilerplate.) Measurements are an average of three trials run in VS Code 1.46.0-insider (b1ef2bf) with extensions disabled.

	master	PR	Diff
Startup time (ms)	2729	2636	-92 ms (-3%)

Type “P”*
updateGraph	18	15	-4 ms (-20%)
completionInfo	102	115	+13 ms (+13%)

Backspace, Type “P”**
completionInfo	44	38	-6 ms (-13%)

Comment out import***
getApplicableRefactors	44	62	+18 ms (+42%)

^{* This triggers completions, including import suggestions for PatternValidator from @angular/forms in the PR trials, a suggestion that is not included in the master trials.}

^{** This triggers completions again without disrupting the auto-import cache populated in the previous action.}

^{*** This changes the structure of the primary program, which triggers an update of the auto-import provider program. The time it takes to do so is included in the subsequent getApplicableRefactors measurement.}

Observations:

Editing operations that don’t change the structure of the program are unaffected (looking at updateGraph time)
When generating completions, there’s an added cost associated with looking through more modules and generating auto-import suggestions for them. The penalty isn’t huge, though, and seems to be worth it for the added utility.
When something happens that triggers an update of the auto-import provider, there’s an associated cost that scales with the number of typed dependencies in package.json files that are not already in the main program (i.e., the size of the auto-import provider program). Because the Angular CLI generates boilerplate that has more unused typed dependencies than most real codebases (normally when you install a dependency, you import it shortly thereafter), I suspect that it will be rare for users to see much more delay than the 18 ms measured here.

Limitations

Only package.jsons found in the project’s currentDirectory and upward are considered. So in an unusual project structure like
```
tsconfig.json
project/
  node_modules/
  package.json
  index.ts
```
the project/package.json file won’t be found.
This doesn’t currently resolve to JavaScript files—that is, to get auto imports, the package has to have .d.ts files. Processing plain JS for auto-imports could be explored if demand were high, but when I tried it, most of the added packages were CLI tools and plugins, not things people want to import from, so it was a performance hit for little (or even negative) utility.

Memory considerations

The auxiliary program is configured to be as light as possible, but in a project references editing scenario, it’s possible to have many projects open at once. Each project has its own language service, so each open project will typically have one of these auxiliary programs. Some possibilities for mitigating high memory usage in these scenarios: (updated: decided to exclude devDependencies for UX reasons)

The auxiliary program won’t be created if there are no packages listed that aren’t already in the main program, but an analysis of around 500 popular open source TypeScript repos shows that this rarely happens, largely due to devDependencies that ship their own types but are never used in the program (prominent example: typescript). Excluding devDependencies significantly increases the chances that creation an auxiliary program can be skipped, at the cost of those devDependencies being unavailable for auto-import (which can be useful when writing build scripts and tests).
The auxiliary program isn’t created until there’s an open file that belongs¹ to the project. This prevents us from creating these programs as we do a cross-project find-all-references.
If we detect that memory usage is a problem, we could dispose or stop creating these programs. I think @sheetalkamat has looked into this in the past.
We could either hard-code into TypeScript or indicate in third-party projects that a package should opt out of being eagerly auto-importable. In one real-world project I tested, the contents of the auxiliary programs were limited to typescript, typescript-eslint, ts-jest, jest, and prettier, none of which are commonly imported.

However, at this point I’m not convinced that any of these mitigations are necessary. The size of these programs is usually in the ballpark of a browser tab (around 50 MB from one early test). But we have avenues to explore if needed.

Fixes #37812
Fixes #36042

¹ “file that belongs to the project” here means “file whose default project is the project in question,” in contrast to “file that is contained by the project”

…ange

e.g., don’t create them during cross-project find-all-refs

…-json

src/server/editorServices.ts

src/server/scriptInfo.ts

src/services/codefixes/importFixes.ts

src/services/services.ts

andrewbranch · 2020-06-03T23:31:30Z

@typescript-bot pack this

typescript-bot · 2020-06-03T23:31:33Z

Heya @andrewbranch, I've started to run the tarball bundle task on this PR at 86151e1. You can monitor the build here.

typescript-bot · 2020-06-03T23:48:40Z

Hey @andrewbranch, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/75621/artifacts?artifactName=tgz&fileId=45DA6FF44370AD15DEFF5E61EDC74241CF262D815E40DE2D87AB199B145FE63502&fileName=/typescript-4.0.0-insiders.20200603.tgz"
    }
}

and then running npm install.

There is also a playground for this build.

andrewbranch · 2020-06-05T17:40:27Z

After further thought, I do think devDependencies should be excluded by default (we could add a configuration option to include them if enough people really want them). Including them by default means that virtually every project written in TypeScript would get auto-imports for the compiler, which nobody wants. If you do want to import the compiler API, typescript is either going to be in your "dependencies" or "peerDependencies"—and we should include peer dependencies by default.

src/server/project.ts

sheetalkamat · 2020-06-22T22:24:58Z

src/server/project.ts

@@ -1815,7 +1815,7 @@ namespace ts.server {
                    name,
                    rootFileName,
                    compilerOptions,
-                    hostProject));
+                    moduleResolutionHost));


You dont need to pass moduleResolutionHost here but instead you want this to also handle trace (if logging enabled going to log) and currentDIrectory
ModuleResolutionHost = hostProject.projectService ?

I don’t think I understand the suggestion—ProjectService isn’t assignable to ModuleResolutionHost.

Merging for the beta, feel free to follow up with me

You want it to be {
boundMethods from hostProject.projectService.host
getCurrentDirectory : () => hostProject.getCurrentDirectory(),
trace: mayBeBind(hostProject, hostProject.trace)
}

Is there a realistic example of why hostProject itself wouldn’t work? If it wants to mimic the existence of some files, why shouldn’t it be allowed to do that during type directive resolution?

Co-authored-by: Sheetal Nandi <shkamat@microsoft.com>

* upstream/master: (58 commits) Variadic tuple types (microsoft#39094) chore: resolve suggestions Expand auto-import to all package.json dependencies (microsoft#38923) inline local functions Update bigint declaration file (microsoft#38526) Update user baselines (microsoft#39077) LEGO: check in for master to temporary branch. Add missing index.ts files to user projects (microsoft#39163) Add reason for a disabled code action (microsoft#37871) Minor fix for assertion predicates (microsoft#38710) Update LKG (microsoft#39173) Reparse top level 'await' in modules (microsoft#39084) change chore: more change chore: resolve review chore: save space fix: lint error test: add test for it chore: make isJsxAttr required chore: revert change in checker ... # Conflicts: # src/compiler/binder.ts # src/compiler/checker.ts # src/compiler/parser.ts # src/compiler/types.ts

* Start experiment * Add logging * Go back to a single program * Fix forEachExternalModuleToImportFrom * Move auxiliary program to language service * Add logging * Don’t use resolution cache * Fix(?) containingProjects for ScriptInfo in auxiliary program * Fix ScriptInfo project inclusion * Add test for default project of auto-importable ScriptInfo * Add fourslash server test * Don’t create auto import provider inside node_modules * Add monorepo-like test * WIP * Naively ensure autoImportProvider is up to date after package.json change * Start limiting when auto update provider gets updated * Respond to changes in node_modules * Don’t create auto-import provider until a file is open that would use it e.g., don’t create them during cross-project find-all-refs * Clean up naming, @internal marking, and fix empty project creation bug * Drop devDependencies, include peerDependencies * Add additional compiler options * Fix interaction with importSuggestionsCache * Move option to UserPreferences, allow inclusion of devDependencies * Don’t filter out peerDependencies * Watch unparseable package.jsons * But don’t filter packages out due to an invalid package.json * Update test * Don’t use autoImportProvider in codefixes where it can never be used (or any refactors) * Add CompletionEntry property for telemetry * Add assertion for isPackageJsonImport to fourslash * Fix missing pushSymbol argument * Add isPackageJsonImport to tests and API baselines * Fix unit test * Host auto import provider in new Project kind * Fix InferredProject attaching on AutoImportProvider-included files, load eagerly * Update Public APIs * Simplify PackageJsonCache host * Remove unneeded markAsDirty * Defer project finished event until after AutoImportProvider is created * Make AutoImportProviderProject always report isOrphan = true * Close and remove AutoImportProviderProject when host project closes * Don’t set pendingEnsureProjectForOpenFiles * Use hasAddedOrRemovedFiles instead of hasNewProgram * Use host-wide watchOptions for package.json watching * Add to `printProjects` * Clean up * Get autoImportProvider directly from LanguageServiceHost * Clean up * Clean up * Close auto import provider on disableLanguageService * Move AutoImportProvider preload to project updateGraph * Clear auto import suggestion cache when provider program changes * Fix tests * Revert yet-unneeded change * Use projectService host for module resolution host * Don’t re-resolve type directives if nothing has changed * Update src/server/project.ts Co-authored-by: Sheetal Nandi <shkamat@microsoft.com> * Use ts.emptyArray Co-authored-by: Sheetal Nandi <shkamat@microsoft.com>

woodgear · 2020-10-12T14:03:52Z

After further thought, I do think devDependencies should be excluded by default (we could add a configuration option to include them if enough people really want them). Including them by default means that virtually every project written in TypeScript would get auto-imports for the compiler, which nobody wants. If you do want to import the compiler API, typescript is either going to be in your "dependencies" or "peerDependencies"—and we should include peer dependencies by default.

sometimes we do want to auto import in devDependencies, for example, some test only package. is there some better way to avoid the "typescript" problem? maybe some kind of filter?

andrewbranch · 2020-10-12T16:30:47Z

Once you manually import your devDependency once, it will be available for auto-import throughout the rest of your project. So... it's kind of already a reverse filter that you configure implicitly as you code.

nandorojo · 2021-11-16T17:41:17Z

Is there a way to specify which package.json to use for this?

I have a monorepo, structured like this:

apps
- nextjs
packages
- components

My package.json inside of apps/nextjs has all of my dependencies, such as react, etc.

However, VSCode only checks the root package.json of my monorepo, which has zero dependencies inside of it. As a result, none of the monorepo packages get autoimport.

Is there a way to set the setting to specify a packageJsonPath or something like that?

  "typescript.preferences.includePackageJsonAutoImports": "on",
  "typescript.preferences.packageJsonPath": "./apps/nextjs",

andrewbranch · 2021-11-16T18:07:09Z

@nandorojo are you primarily a TypeScript or JavaScript user? (Or more to the point, is this particular project written in TypeScript?) The expected structure is that each package of a monorepo should have a tsconfig.json file. As a JS user, you can still have a tsconfig.json or jsconfig.json file there, and even just putting {} as its contents should be enough for package.json auto-imports to work. Note that we have an open issue for investigating inferring this structure for JS users without any tsconfig/jsconfig files, but it’s complicated: #45558.

nandorojo · 2021-11-16T18:13:13Z

@andrewbranch really appreciate the fast reply.

I'm a TS user exclusively. I tried adding a tsconfig.json to each packages/* folder as well, but no dice on autoimports.

Interestingly, autoimport does get solved if I duplicate my dependencies from apps/nextjs/package.json → root package.json's peerDependencies. But it's a definite antipattern to put dependencies in the root of a workspace like that.

I don't mind having a tsconfig in each packages/* and apps/* folder, but I don't think this solves it for me.

I am able to autoimport my monorepo packages (@beatgig/components does autoimport). But the dependencies from apps/nextjs, such as react, still don't autoimport inside of packages/*.

Let me know if you need me to clarify anything there, it might be a bit hard to follow.

andrewbranch · 2021-11-16T18:20:25Z

Are you able to either share the project you’re working in or reproduce this in a little example? The only possible explanation I can think of besides this being a bug is that if we discover too many unused dependencies, we disable the package.json auto imports:

This would happen if you have a bunch of dependencies listed in a package.json but the project in question has not already imported them. You can try toggling this setting to on and see if that makes a difference. Otherwise, go ahead and open a new issue with repro steps. Thanks!

nandorojo · 2021-11-16T18:25:11Z

I did already set that to on inside of .vscode/settings.json, actually, but unfortunately it didn't help. I don't think I can share the current repo since it's private, but I'll try to reproduce it in one you can use and will post an issue. Thank you again!

nandorojo · 2021-11-16T18:57:14Z

@andrewbranch I have a repro here: https://github.com/nandorojo/vscode-monorepo-autoimport-repro

It's very minimal (these are all the dependencies):

Do you need me to open an issue, or is this sufficient? I added a few comments in the only two ts files that should give you what you need.

andrewbranch · 2021-11-16T19:10:42Z

This is working as intended—react is not listed in any package.json visible to packages/components/index.ts. We filter out transitive dependencies from auto-imports because importing from packages you don’t explicitly depend on is fragile—see #30033 and #38768. You don’t need to add react to the root package.json, but it should be in packages/components/package.json. I believe yarn/npm/lerna will all hoist this to the root node_modules so the dependency isn’t duplicated, so there’s no reason not to specify it as a dependency.

nandorojo · 2021-11-16T19:23:15Z

I see, thanks for clarifying.

It works fine if I manually import it, though, so it's as if the autoimport doesn't match the actual behavior. Both the Next.js app and the type checker work without explicitly adding these dependencies to each package.

My hesitation with adding it to packages/components/package.json is that I'll have tons of duplicate items in each package.json when it isn't actually required, either at runtime or for types. I would only add these dependencies to each package to get auto-import to work.

Is there a way to possibly extend the dependencies from apps/nextjs? (My guess is there isn't, but just in case...)

I believe yarn/npm/lerna will all hoist this to the root node_modules so the dependency isn’t duplicated, so there’s no reason not to specify it as a dependency.

I agree that practically speaking, since they won't duplicate, there's no real downside to this.

But in a large monorepo with over a dozen packages/* folders and tons of dependencies, it's a pretty rough DX to copy every dependency into so many places, given that it's all only to get the auto-imports to work.

If there are no intentions to allow transitive dependencies (even behind a flag) for autoimports, then I'll just add each dependency to each subpackage. My guess is, you've thought about the downsides more than I have. But I thought I would leave my thoughts on the DX.

I don't necessarily want all transitive dependencies to autoimport – only the ones I've explicitly put in a subpackage's package.json.

nandorojo · 2021-11-16T19:41:43Z

I did try explicitly adding these to my monorepo packages, as recommended, and it does solve the autoimports.

However, to prevent duplicate dependencies, I have to fix the versions in packages/components/package.json to the same exact ones in apps/nextjs/package.json. If I don't do this, then my lockfile will reflect an additional dependency.

I was hoping this would result in no changes to my lockfile, but it seems it does:

{
  "name": "@repo/components",
  "version": "0.0.0",
  "main": "index.ts",
  "devDependencies": {
    "moti": "*",
  }
}

For example, moti now has 0.16.1 released. Originally, apps/nextjs had version 0.16.0. But now that I added it to @repo/components, I now have both versions installed:

I can put versions there explicitly instead to avoid duplication, but it makes updating a dependency a bit of a hassle. I now have to increment the version in 15+ folders each time I want to update.

I know this is intended, but I figured it would be useful to share the downside. Thanks again!

andrewbranch · 2021-11-16T19:47:18Z

I think package/workspace managers have tools for doing these kind of bulk install operations. I also think that if you’re not publishing these packages individually, but rather the monorepo structure is just for code organization, there’s no great downside to putting common dependencies in the root package.json. And if you are publishing each package individually, then I think it is an antipattern to import transitive dependencies. There are people who vehemently disagree with me on that point in #38768, but I’m pretty firm on it.

nandorojo · 2021-11-16T19:54:37Z

I also think that if you’re not publishing these packages individually, but rather the monorepo structure is just for code organization, there’s no great downside to putting common dependencies in the root package.json.

Interesting, good to know. I'm only concerned for the times that I'm using this to organize shared packages across our internal apps. When it comes to open source monorepos I publish, I do in fact copy over the dependencies in each package. But for an app with code organization, I don't know why importing transitive dependencies would really be an issue. I suppose I'll just put the deps in the root package.json, and hopefully that works fine across the apps themselves.

I think package/workspace managers have tools for doing these kind of bulk install operations.

Maybe lerna does this, I've only used it for publishable packages so I'm not sure. If there aren't downsides to using the root package on internal projects, I might just do that. But yarn workspaces warns against it whenever you install something in the root package, so I figured it wasn't a good pattern.

nandorojo · 2021-11-16T21:25:48Z

In case anyone else lands, here, I found a nice monorepo tool that extends Lerna's capabilities to keep versions tied together between packages when you install/upgrade: https://www.npmjs.com/package/lerna-update-wizard

Heard through lerna/lerna#2142 (comment)

I wrote up a guide here: https://github.com/nandorojo/ts-monorepo-autoimport-guide

andrewbranch added 20 commits May 13, 2020 09:59

Start experiment

f67fb5b

Add logging

8e128d8

Go back to a single program

23f72c7

Fix forEachExternalModuleToImportFrom

913c380

Move auxiliary program to language service

c1d1721

Add logging

feb719a

Don’t use resolution cache

3bad9ee

Fix(?) containingProjects for ScriptInfo in auxiliary program

d0c2d50

Fix ScriptInfo project inclusion

3554044

Add test for default project of auto-importable ScriptInfo

3d03027

Add fourslash server test

bba64e2

Don’t create auto import provider inside node_modules

304b8a3

Add monorepo-like test

4fed93d

WIP

958adc0

Naively ensure autoImportProvider is up to date after package.json ch…

1ff2db5

…ange

Start limiting when auto update provider gets updated

af64cd2

Respond to changes in node_modules

3d07e97

Don’t create auto-import provider until a file is open that would use it

936763a

e.g., don’t create them during cross-project find-all-refs

Merge branch 'master' into experiment/auto-import/program-per-package…

c0066b3

…-json

Clean up naming, @internal marking, and fix empty project creation bug

86151e1

typescript-bot assigned andrewbranch Jun 3, 2020

typescript-bot added the Author: Team label Jun 3, 2020

andrewbranch commented Jun 3, 2020

View reviewed changes

andrewbranch requested review from sheetalkamat and amcasey June 3, 2020 23:31

kyliau mentioned this pull request Jun 5, 2020

Type definitions not exported correctly for vscode (intellisense not working on clean projects) angular/angular-cli#16879

Closed

sheetalkamat reviewed Jun 22, 2020

View reviewed changes

src/server/project.ts Outdated Show resolved Hide resolved

sheetalkamat reviewed Jun 22, 2020

View reviewed changes

Update src/server/project.ts

7b1aaf4

Co-authored-by: Sheetal Nandi <shkamat@microsoft.com>

andrewbranch changed the title ~~Experiment: expand auto-import to all package.json dependencies~~ Expand auto-import to all package.json dependencies Jun 22, 2020

Use ts.emptyArray

85c6938

andrewbranch merged commit 086e00d into microsoft:master Jun 22, 2020

andrewbranch deleted the experiment/auto-import/program-per-package-json branch June 22, 2020 23:34

This was referenced Jul 21, 2020

[typescript-language-features] Add setting for package.json auto imports microsoft/vscode#103037

Merged

[typescript-language-features] Add telemetry for package.json auto imports microsoft/vscode#103126

Merged

DanielRosenwasser mentioned this pull request Jul 31, 2020

Design Meeting Notes, 7/31/2020 #39852

Closed

This was referenced Oct 12, 2021

Initial support for module: node12 #44501

Closed

Auto imports not working for modules exported via exports in package.json #46339

Closed

IllusionMH mentioned this pull request Aug 2, 2022

Language Services should recognize transitive dependencies for intellisense microsoft/vscode#156892

Closed

github-actions bot mentioned this pull request Oct 4, 2022

[Korean] Fix weird expressions like '_제네릭_' microsoft/TypeScript-Website-Localizations#171

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expand auto-import to all package.json dependencies #38923

Expand auto-import to all package.json dependencies #38923

andrewbranch commented Jun 3, 2020 •

edited

Loading

andrewbranch commented Jun 3, 2020

typescript-bot commented Jun 3, 2020 •

edited

Loading

typescript-bot commented Jun 3, 2020 •

edited

Loading

andrewbranch commented Jun 5, 2020

sheetalkamat Jun 22, 2020

andrewbranch Jun 22, 2020

andrewbranch Jun 22, 2020

sheetalkamat Jun 23, 2020

andrewbranch Jun 23, 2020

woodgear commented Oct 12, 2020

andrewbranch commented Oct 12, 2020

nandorojo commented Nov 16, 2021

andrewbranch commented Nov 16, 2021

nandorojo commented Nov 16, 2021

andrewbranch commented Nov 16, 2021

nandorojo commented Nov 16, 2021 •

edited

Loading

nandorojo commented Nov 16, 2021

andrewbranch commented Nov 16, 2021

nandorojo commented Nov 16, 2021 •

edited

Loading

nandorojo commented Nov 16, 2021

andrewbranch commented Nov 16, 2021

nandorojo commented Nov 16, 2021 •

edited

Loading

nandorojo commented Nov 16, 2021 •

edited

Loading

Expand auto-import to all package.json dependencies #38923

Expand auto-import to all package.json dependencies #38923

Conversation

andrewbranch commented Jun 3, 2020 • edited Loading

Performance

Limitations

Memory considerations

andrewbranch commented Jun 3, 2020

typescript-bot commented Jun 3, 2020 • edited Loading

typescript-bot commented Jun 3, 2020 • edited Loading

andrewbranch commented Jun 5, 2020

sheetalkamat Jun 22, 2020

Choose a reason for hiding this comment

andrewbranch Jun 22, 2020

Choose a reason for hiding this comment

andrewbranch Jun 22, 2020

Choose a reason for hiding this comment

sheetalkamat Jun 23, 2020

Choose a reason for hiding this comment

andrewbranch Jun 23, 2020

Choose a reason for hiding this comment

woodgear commented Oct 12, 2020

andrewbranch commented Oct 12, 2020

nandorojo commented Nov 16, 2021

andrewbranch commented Nov 16, 2021

nandorojo commented Nov 16, 2021

andrewbranch commented Nov 16, 2021

nandorojo commented Nov 16, 2021 • edited Loading

nandorojo commented Nov 16, 2021

andrewbranch commented Nov 16, 2021

nandorojo commented Nov 16, 2021 • edited Loading

nandorojo commented Nov 16, 2021

andrewbranch commented Nov 16, 2021

nandorojo commented Nov 16, 2021 • edited Loading

nandorojo commented Nov 16, 2021 • edited Loading

andrewbranch commented Jun 3, 2020 •

edited

Loading

typescript-bot commented Jun 3, 2020 •

edited

Loading

typescript-bot commented Jun 3, 2020 •

edited

Loading

nandorojo commented Nov 16, 2021 •

edited

Loading

nandorojo commented Nov 16, 2021 •

edited

Loading

nandorojo commented Nov 16, 2021 •

edited

Loading

nandorojo commented Nov 16, 2021 •

edited

Loading