Skip to content
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.

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

Expand auto-import to all dependencies in package.json #37812

Closed
DanielRosenwasser opened this issue Apr 6, 2020 · 19 comments · Fixed by #38923
Closed

Expand auto-import to all dependencies in package.json #37812

DanielRosenwasser opened this issue Apr 6, 2020 · 19 comments · Fixed by #38923
Assignees
Labels
Domain: Auto-import In Discussion Not yet reached consensus Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript

Comments

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Apr 6, 2020

Background

TypeScript's auto-import functionality has some really unpredictable behavior today where the feature doesn't work on for a package until a user manually imports from that package. This experience only affects projects that provide their own types because, by default, TypeScript always includes @types packages as part of every compilation; however, it doesn't include other packages in node_modules[1]. Apart from being unfair to TypeScript library authors, this is just plain surprising for users.

For a while now, we've received consistent feedback that this is a confusing experience.

Additionally, we've been seeing weird hacks in projects like Angular CLI, where typeRoots are set to node_modules and node_modules/@types to try to fix auto-imports.

Proposal

In #31893 and #32517, we improved some rough areas of completions by making sure that we only provided completions from packages that are listed in package.json. That way clearTimeout doesn't appear in your completion list unless you specifically list @types/node as a dependency.

What I'd like to suggest is an expansion of completions sources to those in package.json. Specifically what I'm looking for is that for every dependency in package.json, we should try to provide auto-imports. This could be done by changing the default behavior for typeRoots (potentially breaking) or by adding an editor-specific behavior for automatic package inclusion.

There's things to keep in mind as we do this:

  • we don't want to accidentally load up 100s of .d.ts files unnecessarily
  • we don't want to have duplicate entries when a package ships its own .d.ts files but a user has @types packages installed
  • we don't want to add new logic that affects module resolution and changes the behavior of go-to-definition
  • we don't want to drastically diverge in behavior from the command-line

Maybe for the general case, this won't cause a drastic increase in memory usage - most packages with explicit dependencies might get installed/eventually used anyway. But if it is, there might be some workarounds to consider:

  • only go through 2-3 levels of imports for these expanded completions
    • this might not work well for "barrels" (i.e. files with a lot of export * from statements), but it might just be good enough.
  • only expand auto-completions to dependencies and not devDependencies
    • this would be really confusing for utilities and test code, but not application code.

I'd like to get people's thoughts on whether an approach like this would even be possible, and if so, whether we could implement it without degrading the current experience.


[1] The reason for this is the asumption that having an @types package in your node_modules directory is a pretty good sign that you intend to use it (with caveats - see #31893 and #32273). But in order to save on time/memory, we don't load up .d.ts files from other node_modules packages unless we really need to.

@DanielRosenwasser DanielRosenwasser added Suggestion An idea for TypeScript Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. In Discussion Not yet reached consensus Domain: Auto-import labels Apr 6, 2020
@DanielRosenwasser
Copy link
Member Author

Also, thanks to @andrewbranch who explained why this behavior was happening, and who gave me the rundown of what his PRs did in #31893 and #32517 - both which were the inspiration for using the package.json-focused approach proposed here.

@andrewbranch
Copy link
Member

First I want to link #36042, which I’ve been using as the canonical issue to reconsider this behavior. I also explained in some detail how auto-imports work today and some ideas (and corresponding pitfalls) about how we could change them to solve this.

This could be done by changing the default behavior for typeRoots (potentially breaking) or by adding an editor-specific behavior for automatic package inclusion.

I want to expound on this a little bit for the benefit of others who aren’t as familiar with what this means, and to emphasize “potentially breaking” a bit more. What you’re getting at here, which I also explained in #36042, is that today, auto-imports for a particular module are offered if and only if that module is already part of your whole-program compilation, which is determined by:

  • the files covered by files/include/exclude in your tsconfig.json (called “root files”, defaulting to everything outside of node_modules with a ts/tsx extension)
  • the lib files covered by lib in your tsconfig.json
  • the type definition covered by types and typeRoots in your tsconfig.json (defaulting to everything in node_modules/@types)
  • the recursive dependencies of all of the above (anything imported or triple-slash referenced by something already included by any of these four criteria)

Any file matched by these criteria are automatically parsed and bound, and type-checked to some degree, depending on the file location and skipLibCheck. The information produced by parsing, binding, and checking these files is central to the ability to offer auto-imports from them. At the same time, though, these criteria are also used when compiling a program with tsc on the command line. On one hand, we’ve seen in the past that automatic type inclusion, or even inclusion of additional types by recursive dependencies, creates problems of conflicting environmental globals, which has been the subject of many other discussions and proposals. I have personally advocated in the past for easing some of the pain of this problem by not eagerly including all of node_modules/@types in programs. The details of global/environment collisions aren’t super important here; rather, my point is that we know automatic type inclusion does break people, and there are potentially some merits (with tradeoffs) to doing less of it when we look at other problems. But on the other hand, doing less of it can also make TypeScript unaware of globals declared by @types packages, which is also breaking. In other words, any change to the rules that govern what files a program contains, including default values of related tsconfig options, is a significant breaking change.

A part of me thinks it would make a lot of sense to use package.json files to guide type definition inclusion, and stop giving special behavior to @types. It would solve the auto-import problem automatically, and in some cases it would prevent unwanted inclusion of definitions like @types/node that can sometimes cause problems. But another part of me thinks that’s a really hard sell for a breaking change to tsc. I think it’s worth talking about, but my assumption in thinking about this so far has been that it’s not going to be on the table.


In that case, the only option is, as you said, adding a language-service-specific implementation that doesn’t affect program semantics, and doesn’t touch tsc at all. So, a couple thoughts on that:

Maybe for the general case, this won't cause a drastic increase in memory usage - most packages with explicit dependencies might get installed/eventually used anyway.

I completely agree with this. If people have packages listed in their package.json, they’re going to import or reference them somehow. I’m not at all concerned about the memory impact of loading types from package.json-listed dependencies; I’m only concerned about their semantic impact on the program. So if we need a way to isolate these types from influencing the core program, how do we do that? Load them into a second program? That would take care of all the parsing, binding, and checking that auto-imports relies on, but it would duplicate a lot of work and types and symbols from common lib files (not to mention the weight of all the closed-over functions in the program/checker)—that’s the concern I have with memory. We should do some experiments and see what the cost of that really is. I also think it’s worth seeing how much we can do with parsing and binding alone. But both of these ideas are pretty complicated in an editor scenario, where the user is continually installing and uninstalling node modules and changing the shape of the core program by adding and removing imports.


Takeaways:

  • I guess I didn’t comment on whether your proposal is the right expected behavior, because I’ve thought it’s the right expected behavior since I got here, and I always imagined Updated: Only auto-import from package.json #32517 as the first step in a process toward that goal.
  • I want to hear thoughts about changing default type definition inclusion rules to look at package.json before ruling it out.
  • Assuming we don’t change type definition inclusion rules, I want to hear more thoughts on an editor-specific implementation. What have I missed in my analysis? Is there a simpler approach? Do others agree with my assessment of the complexity of the approaches I’ve mentioned?

@DanielRosenwasser
Copy link
Member Author

I’m not at all concerned about the memory impact of loading types from package.json-listed dependencies; I’m only concerned about their semantic impact on the program. So if we need a way to isolate these types from influencing the core program

One of the things that @ahejlsberg mentioned in a conversation was potentially using a second TypeChecker instance for auto-imports and the like.

@uniqueiniquity
Copy link
Contributor

I'll need to read through this tomorrow for any lucid feedback, but is there anything we can learn from Roslyn's version of this for C#? I think theirs even recommends things from packages you don't yet have installed, so I imagine they've put work into the appropriate heuristics for this, albeit tuned to their user base.

@sheetalkamat
Copy link
Member

Assuming we don’t change type definition inclusion rules, I want to hear more thoughts on an editor-specific implementation. What have I missed in my analysis? Is there a simpler approach? Do others agree with my assessment of the complexity of the approaches I’ve mentioned?

Having different files in program from what they are in tsc is bad idea as that results in discrepancies or errors/non errors and that gets confusing. I think the idea of different program to support the auto import seems better one..

@andrewbranch
Copy link
Member

Yeah, I agree. I haven’t seriously considered that as an option. Just to make sure it’s clear, when I say

I want to hear thoughts about changing default type definition inclusion rules to look at package.json before ruling it out.

I’m referring to changing the behavior both for tsc and for the language service (which is why it would be such a significant breaking change).

@DanielRosenwasser
Copy link
Member Author

@uniqueiniquity

is there anything we can learn from Roslyn's version of this for C#

Really glad you brought this up - I briefly spoke with @CyrusNajmabadi to understand more of what Roslyn does.

I think theirs even recommends things from packages you don't yet have installed

As far as I understand, that's a quick fix, not code completion functionality. We provide an Install '@types/whatever'. quick fix, but not one for installing missing packages. If you're interested in that, it's tracked by #20649.

I also think that auto-import completions could be a bit too noisy for npm packages. There's over a million packages on npm, and while Roslyn seems to rely on an index of the most popular NuGet packages, it's partially out of date and we might have a harder time distinguishing between common identifiers, especially functions named assert or it or describe from different testing frameworks. Maybe this would be fine as a quick fix, though we already have a few of these for identifiers like $, Buffer, etc. (details here).

This all did make me wonder: instead of digging through .d.ts files and creating a separate TypeChecker or Program, whether there's some potential to create a digest of identifiers ahead of time; something to know which exports exist without resolving/scanning/parsing/binding each package's .d.ts files. Maybe LSIF could be useful? But the question is when it's generated and how to get it.

@uniqueiniquity
Copy link
Contributor

As far as I understand, that's a quick fix, not code completion functionality. We provide an Install '@types/whatever'. quick fix, but not one for installing missing packages. If you're interested in that, it's tracked by #20649.

Not necessarily interested; my point was just that it seemed like a good sign they had substantially thought through this. :)

Maybe LSIF could be useful? But the question is when it's generated and how to get it.

I think LSIF could definitely be useful for this - in fact, this is kinda what it's for (see lsif-npm). I'm not sure where this information is currently hosted or how often it's generated, though.

@s4m0r4m4
Copy link

s4m0r4m4 commented May 4, 2020

As the person who opened #38176, which was linked to this issue, let me just provide one user's viewpoint. I looked at #32517 and saw that only imports from package.json are included. I can understand needing to limit the scope of what you provide auto-imports for, and if package.json is the limit then I will work with that.

That being said, sometimes packages provide functions/etc. that reference types (either as input parameters or return types) from other packages, which may be in their own package.json dependencies. You could consider a scheme that imports from a project's package.json, and then through all dependencies of the packages listed in that package's package.json (1 level deep). It could go 2 levels deep, or even fully recursive. Perhaps the number of levels could be configurable by some property to allow users to chose the balance betwee performance and coverage.

I don't pretend to know all the under-the-hood details here, and looking at the linked issues I can see that there has been signficant discussion on this issue to date. And I understand that this is just one use case in a sea of many. So take my input with a grain of salt, and perhaps it will prove useful!

@Lonli-Lokli
Copy link

I have related issue but not sure if it the same.
I have an Angular project with this structure
~

  • projects
    --projectName
    ---componentLevel1
    ----componentLevel2
    -----componentLevel3
    ---shared

So when Im trying to import smth from shared into componentLevel3 it goes with import projects/shared while I as joing to see imports from ../../../shared.

@andrewbranch
Copy link
Member

andrewbranch commented May 15, 2020

I want to give an update on the state of my experiments here. I’ve been working on an implementation of using one or more auxiliary programs that contain only type definition files from node_modules packages that are listed in a package.json file to see what the memory and performance impact is.

A simple test

I created an extremely simple approach with lots of room for optimization and tested it on the boilerplate generated by the Angular CLI. (It was a good candidate because it’s realistic, not entirely empty, and includes quite a few unreferenced dependencies that ship their own types, e.g. @angular/forms.) I created a non-diagnostics-producing noLib program with rootNames comprising the declaration file entrypoint from each non-@types package.json dependency. (Notably, this means that dependencies already imported by the main program, like @angular/core, were redundantly included in the auxiliary program.)

After the auxiliary program was created, the difference in response time for completions increased by about 50%. I haven’t yet investigated whether there are opportunities for improvements here yet, but I expect that much of the increased time is just the natural cost of processing more modules for auto import—that is, if all these dependencies were in @types, or already imported into the main program, we’d realize that same cost, which we’ve always considered to be worthwhile. As a result of that extra work, we get a better list of completions.

Resolving all the package.json dependencies and creating the program took around 1700 ms, so we need to be sure that happens during a non-interactive time. Also, creating the program without the redundant dependencies already in the main program could bring this time down. The approximate weight of the extra program was 56 MiB, which was a 47% increase over the baseline weight. Again, deduplication of dependency inclusion would help here.

Upcoming challenges

The big things that need to be done to make this approach viable are

  1. Efficiently respond to changes to package.json files and node_modules, in part by hosting the auxiliary program in a way that caches dependency resolution and source files between updates. (For the simple experiment, updating the program after a package.json change took about as long as creating the program initially because almost nothing was recycled.)
  2. Deduplicate dependencies between the main program and auxiliary program at some point in the auto-import pipeline so that at the very least, you don’t get duplicate auto-import suggestions for dependencies you’ve already imported somewhere.

The latter is particularly challenging in the project references scenario (particularly project references in a monorepo context where there are lots of package.jsons and node_modules folders), because there are multiple “main programs,” each with a different constituency of files already included. Suppose we have a project like:

node_modules/mobx/index.d.ts
package.json
src/
  project-a/
    tsconfig.json
    index.ts
    store.ts

  project-b/
    tsconfig.json
    index.ts
    utils.ts

Let’s say that mobx is listed in package.json, and src/project-a/index.ts already imports it, but project-b does not yet import it at all.

If you open src/project-a/store.ts, the auxiliary program doesn’t need to include mobx, because the main program for project-a already includes it. But if you open anything in project-b, you’d want the auxiliary project to include mobx, because project-b has its own program that currently lacks mobx. Once you import it into project-b, though, it would be reasonable to remove it from the auxiliary program. If you create some file that doesn’t belong to project-a or project b, you’d want to add it back. So if we only have one auxiliary program for auto-imports, we can’t aggressively shrink it down, which is unfortunate because a) the program will take more resources to store and to update, and b) we’ll have to deduplicate auto-import suggestions as they’re queried, since we can never guarantee no overlap between the main program and auxiliary program.

On the other hand, @sheetalkamat pointed out that if you host one auxiliary program per language service, you can solve both problem 1 and problem 2 above. Taking the same mobx example again, when you create the language service for project-a, you know everything that its main program already imports, so you can avoid creating the auxiliary program for it entirely if you see that all the package.json dependencies are already included in the main program. Then when you start editing project-b and create its language service, you see that mobx is not part of the main program, so you spin up the auxiliary program. When generating auto-import suggestions, you don’t have to deduplicate, because you know the contents of the two programs are mutually exclusive. At the same time, language service objects are already well-equipped to host programs with caching and efficient updating, so this solves problem 1 as well.

The downside of this approach is that in a large monorepo, you could end up creating a lot of programs, and even though each auxiliary program’s contents is mutually exclusive with its main program, all the auxiliary programs’ contents could easily be redundant with each other (particularly in a monorepo where all dependencies are hoisted to the top level), which just feels like a waste of memory. (Note: my understanding is that symbols and types would be redundant across programs, but source file objects can be shared through a common document registry.) The hope is that most projects will use 100% of their dependencies most of the time, so most projects could avoid having this auxiliary program except for the short time between npm install new-dependency and importing that new dependency for the first time, but it would be good to research how often this holds. (@amcasey sent me a script to crawl some projects on GitHub so I could find this out.)

Next steps

  • I’m going to research how often we can expect individual packages within monorepos to use 100% of package.json-listed dependencies from all package.jsons visible to them.
  • I’m going to see how much weight can be shared between two programs with the same files by letting them share a document registry.
  • I’m interested to hear thoughts on mitigating the memory concerns in the auxiliary-program-per-language-service approach. I guess we could be more aggressive about disposing these programs when they’re not being actively used, but that increases the work we’d have to do to recreate them later.

@arogg
Copy link

arogg commented Jun 11, 2020

Currently you cannot trigger an import suggestion for any newly added files for a project reference. So I added all files from project reference to my tsconfig "include". Now it works. Not sure to what extent this defeats project references. BUT it's the only way I can work somewhat productively. Just wanted to add my 2c. Is there anything I am missing?

@IlCallo
Copy link

IlCallo commented Jun 16, 2020

Jumping in (if I understood correctly the topic) just to confirm this could cause a problem for projects which uses testing frameworks with conflicting global types (see Jest + Cypress combo).
In these scenarios, types of some dependencies found into package.json could be needed only into a particular tests/my-testing-framework folder.

I think there is an ongoing effort to make jest more ESM-ish style, which could potentially avoid global-scope conflicts, but dunno about mocha, chai, jasmine, cypress, etc etc

@DanielRosenwasser
Copy link
Member Author

Currently you cannot trigger an import suggestion for any newly added files for a project reference.

@arogg I think you'd be better off filing a separate issue

Jumping in (if I understood correctly the topic) just to confirm this could cause a problem for projects which uses testing frameworks with conflicting global types (see Jest + Cypress combo).
In these scenarios, types of some dependencies found into package.json could be needed only into a particular tests/my-testing-framework folder.

@IlCallo can you elaborate a little bit? This proposal expands auto-imports, so how would this cause problems?

@amcasey
Copy link
Member

amcasey commented Jun 16, 2020

@IlCallo I hope I'm not over-simplifying, but wouldn't test frameworks, etc usually be devDependencies? I believe this change excludes those by default.

@andrewbranch
Copy link
Member

andrewbranch commented Jun 16, 2020

@amcasey is correct, but it sounds like @IlCallo’s concern was (please correct me if I misunderstood) that types discovered by the auto-import provider would leak into type checking, negating any carefully defined project boundaries and include/exclude/types configuration settings. That’s not the case—any types discovered by the auto-import provider are isolated into a separate type checker, so this PR has zero effect on compilation or errors exposed by your editor. Even if types within the auto-import provider conflict, those errors will never be surfaced. @IlCallo did I understand your concern correctly, and does my answer address it?


Edit: I thought this comment was on my PR, not on the original issue, which might be a source of confusion. Some of the approaches discussed here would have created the problem I just described, but we have pretty much settled on an approach that avoids it, which is in progress here: #38923

@IlCallo
Copy link

IlCallo commented Jun 16, 2020

@andrewbranch yes, that was my concern.
I came here from the TS4 roadmap issue, I didn't see the PR and missed the "it is on an auxiliary program" bit 😅
All good then 👍

@e-cloud
Copy link
Contributor

e-cloud commented Mar 21, 2021

@andrewbranch hey guy.

I'm using latest vscode v1.54.3, typescript 4.1.5, angular v11. Still no luck to trigger auto import for @angular/material.

Is there a clear guide for solving this problem?

I have @angular/material as my dependency.

@andrewbranch
Copy link
Member

@angular/material has an empty index.d.ts file for its typings, generated from https://github.com/angular/components/blob/master/src/material/index.ts. If that entrypoint doesn’t reference the other modules in the package, there’s nothing we can do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: Auto-import In Discussion Not yet reached consensus Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.