microsoft / TypeScript

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

TypeScript 3.9.7 doesn't auto import from dependancies of dependancies (3.6.3 does) #39911

Closed cjdreiss closed 4 years ago

cjdreiss commented 4 years ago

I have a "parent" app which is a base library that we use in other apps. We recently moved all the dependancies into the parent package.json, and removed them all from the children apps keeping only the parent. Making sure we had the right versions was cumbersome, and NPM would install all the dependancies of the parent app anyway so things work.

The problem we noticed is that we can only auto import from the parent (which is in the child app's package.json), and not things like Angular or RxJS.

TypeScript Version: 3.9.7 (also 3.8.3, and 4.0.0-dev.20200803 with a VS Code plugin)

Search Terms: auto import package.json dependancies types

Code

I have created an example repo here: https://github.com/cjdreiss/ts-import-error

It contains the "parent" app, and a "child" app with instructions in the readme.

Parent package.json

"dependencies": {
    "@angular/animations": "~10.0.6",
    "@angular/common": "~10.0.6",
    "@angular/compiler": "~10.0.6",
    "@angular/core": "~10.0.6",
    "@angular/forms": "~10.0.6",
    "@angular/platform-browser": "~10.0.6",
    "@angular/platform-browser-dynamic": "~10.0.6",
    "@angular/router": "~10.0.6",
    "rxjs": "~6.5.5"
}

Child package.json

"dependencies": {
  "@cjdreiss/ts-import-error-parent" : ">0.0.0"
}

Component trying to import things from rxjs

import { Component } from '@angular/core';
import { Observable } from 'rxjs';
import { ExampleComponent } from '@cjdreiss/ts-import-error-parent';

@Component({
  selector: 'app-root',
  templateUrl: './app.component.html',
  styleUrls: ['./app.component.scss']
})
export class AppComponent {
  title = 'child';
  test: Observable<any>;
  // test2: BehaviorSubject<any>; // won't auto import even though there is an rxjs import already...
  parent: ExampleComponent; // auto imports because @cjdreiss/ts-import-error-parent is in package.json

}

Expected behavior: Auto imports from things like rxjs and @angular/xxx should work

Actual behavior: Only auto imports from dependancies listed in package.json work (my library as an example). If we change the TypeScript version VS Code is using to 3.6.3 it can import it.

Non working import in 3.9.7 Aug-04-2020 16-33-16

Working import after switching the TS version to 3.6.3 Aug-04-2020 16-33-50

Related Issues: I believe this issue might be related (although it says its in Milestone 4.0.0), and there are other related issues in that https://github.com/microsoft/TypeScript/issues/37812

This seems to demonstrate the same issue: https://github.com/microsoft/TypeScript/issues/37187 which was closed as a duplicate of https://github.com/microsoft/TypeScript/issues/36042

DanielRosenwasser commented 4 years ago

Can you try out a nightly release of TypeScript to see whether the problem is fixed?

https://marketplace.visualstudio.com/items?itemName=ms-vscode.vscode-typescript-next

cjdreiss commented 4 years ago

@DanielRosenwasser I've added that plugin to VS Code, selected the latest (4.0.0-dev.20200803), restarted VS code, and still having the same issue.

weswigham commented 4 years ago

Pretty sure this is intentional - we scoped autoimports to only the contents of the package.json we found and the current projects existing imports, iirc.

cjdreiss commented 4 years ago

@weswigham is there a config option we can use? I was trying to add things like "node_modules/@angular" to typeRoots but that didn't seem to help (or maybe I just put it in the wrong place?).

It works in an older version of typescript though, my work machine still can switch to 3.6.3 and it works then (but has other issues because we use newer TS features)

DanielRosenwasser commented 4 years ago

Sorry, that's correct, I misread.

The behavior is intentional. I'm not sure if @andrewbranch had any other workarounds, but if you do intend to auto-import from this package, the recommended workflow is to add it as a dependency of some sort. A big reason why we did this was that this sort of import won't even work with pnpm or certain versions of npm and yarn. You can also re-export declarations from those packages, but we don't necessarily recommend that.

Were you just trying to avoid duplication with your dependencies?

andrewbranch commented 4 years ago

I believe this is an exact duplicate of https://github.com/microsoft/TypeScript/issues/38768 and I explained the rationale there, although Daniel pretty much covered it. As far as workarounds go, I think you could put a dummy file with triple slash references (/// <reference types="@angular/core" />) to everything you want to show up that isn’t. That seems of limited use to me though, since they’ll show up after you import from them once.

cjdreiss commented 4 years ago

Yes, we want to avoid duplicates, and sometimes other versions of our apps would also become libraries and if they had miss matched versions, multiple get installed which could cause issue. Also it was a pain to keep all the apps in sync as well.

I see that issue now, "transient" was the word I was looking for in terms of the dependancies.

We only upgrade our TS versions when Angular does (and not right away either) which is why "filter 3" took a while to hit for us.

  (3)> Files that are either not in node_modules or part of packages listed in your package.json deps

Am I misunderstanding what typeRoots is for? It sounds like it might help us.

I'll try the triple slash method as well, but the issue is even in the same file it does not auto import even after we have other imports already.

andrewbranch commented 4 years ago

Am I misunderstanding what typeRoots is for? It sounds like it might help us.

Yes, more or less. typeRoots is mostly to accommodate systems that predate @types on npm, which worked similarly but were often downloaded to a folder called typings instead of going inside node_modules. typeRoots lets people continue using that, potentially in combination with node_modules/@types. If I recall correctly, adding node_modules/@angular to typeRoots works partially somewhat by coincidence, but will give you errors if you have @angular/cli installed, for example, because that package doesn’t contain type definitions, and the assumption is that typeRoots exclusively contains folders that contain type definitions.

the issue is even in the same file it does not auto import even after we have other imports already

Ah yeah, I forgot about this, even though I mentioned it in #38768:

It does seem reasonable to stop filtering them out within the same file, although I’m not sure how big of a performance penalty that will incur (but it will incur one).

I still believe the pattern you’re using is one that should strictly be avoided (although I, along with every JS developer, can sympathize with the difficulties of package version management). I think I’ll look into bypassing the filter for modules that have already been imported from in the same file, but your experience will still be sub-par since normally this would work cross-file. I would strongly recommend solving your version syncing/deduplication issues another way, maybe by using a looser version range in the “parent” package, and/or clever use of peerDependencies somewhere.

cjdreiss commented 4 years ago

The pattern we're using seemed a bit out of the norm, we had been working with different peerDependencies to get something that worked, and we figured let's try this. And it was great because it worked! Till we upgraded Angular, which brought a new version of TypeScript along which broke the functionality.

I've tried the triple slash imports but either I'm doing them wrong, or its still getting filtered out.

I've tried adding them in the parent library, in both the public_api.ts file, one of the exported module.ts files, and even including them directly in a file inside the child app. My build is not removing them, the final JS for my parent library has those triple slash comments in them still.

It still doesn't want to ever auto import, even if there is an import in the file already. Is this expected?

It's just frustrating that it worked in one version, and then was disabled in a newer minor version with out a way to opt out (and the seemingly other ways to do it don't appear to work either).

Failed Triple Slash Examples

Commit adding the triple slash to public_api.ts https://github.com/cjdreiss/ts-import-error/commit/fbbd5118d85a3e708d947c2383b01d56747dad6f#diff-64cc85193ba4ef4f8c02f0187c48e3ab

Commit trying the triple slash in a shared module file from the parent https://github.com/cjdreiss/ts-import-error/commit/88596f434be1914f9c9252e4b42fc8f165c9bc15#diff-ebe96b65194c05f616e8b988d70cf718

Commit adding the triple slashes directly in the app.component.ts in the child app https://github.com/cjdreiss/ts-import-error/commit/ac4a20ad8b3790e1f467099a2a0814302ee70b32#diff-aa6dfe7d2762a02702f781c7cd35f38a

MgSam commented 4 years ago

Is it possible for the TS team to add some option to specify which dependencies it should use to resolve auto-imports for? That way you could opt in to the old behavior for a specific dependency.

Anyone who builds a "base class library" type situation used to have this scenario work and it's now broken. That seems like a major regression to me...

igalnassi commented 4 years ago

We are having similar issues. We have 20+ apps utilizing the same framework, which acts as a central place for common dependencies.

An opt out of some sort would really be helpful.

RyanCavanaugh commented 4 years ago

This is very much the intentional behavior; dependencies of your dependencies are not part of the dependency contract and it's fundamentally incorrect for us to encourage violating that assumption. You are always free to write the imports manually.

MgSam commented 4 years ago

@RyanCavanaugh

TypeScript used to work with this use-case (having a shared framework). It no longer does. You guys inadvertently broke an important use case we and others were using in production. A use case that is important for enabling enterprise-level web development (which was one of the original goals of TypeScript...). Just because you philosophically disagree with it doesn't change any of those facts.

Couldn't the TypeScript compiler at least provide a compiler flag to opt-in to the old behavior?

"Writing imports manually" isn't a solution when you have dozens of developers with varying degrees of expertise all utilizing the same common framework.

GitStorageOne commented 4 years ago

This is very much the intentional behavior; dependencies of your dependencies are not part of the dependency contract and it's fundamentally incorrect for us to encourage violating that assumption. You are always free to write the imports manually.

Looks like TS team should talk with other well-known teams, like framework devs to find out a way, that will still provide import suggestions (at least for frameworks classes and objects). Like an option or separate feature.
Just disabling such feature, is not a correct way. Huge step back.

thecodejack commented 3 years ago

why can't we provide option to atleast allow some indirect dependant modules via some config...seems legit behavior to expect auto imports from indirect dependencies...

abdusco commented 3 years ago

Hey @cjdreiss, have you been able to find any workaround for this?

I've managed to get it working by adding a postinstall script, which mutates package.json and adds a bunch of peerDependencies.

{
  "peerDependencies": {
    "__comment__": "these dependencies are necessary for auto-import to work properly",
    "axios": "*",
    "react-admin": "*",
    "@material-ui/core": "*",
    "@material-ui/icons": "*",
    "@material-ui/lab": "*",
    "react-router-dom": "*",
    "@types/react-router-dom": "*",
    "react-final-form": "*"
  }
}

This gets autoimports working again, because Typescript compiler uses dependencies and peerDependencies to populate import search index.

https://github.com/microsoft/TypeScript/blob/04205ca32c2b6e60a6c38cbf47961319566a8ea4/src/server/project.ts#L1904-L1907

abdusco commented 3 years ago

Typescript traverses up the directories and scans package.json files it finds. https://github.com/microsoft/TypeScript/blob/04205ca32c2b6e60a6c38cbf47961319566a8ea4/src/server/project.ts#L1903

This means if I create a dummy package.json inside src/ I can get auto-imports to work without messing with the project's own package.json.

./package.json

{
  "name": "hello",
  "private": true,
  "dependencies": {
    "package-that-contains-dependencies": "^1.0"
  },
  "scripts": {
    "postinstall": "script-that-populates-dummy-package.json"
  }
}

./src/package.json

{
  "peerDependencies": {
    "__comment": "these dependencies are necessary for auto-import to work properly",
    "__comment2": "these come from the package 'package-that-contains-dependencies'",
    "axios": "*",
    "react-admin": "*",
    "@material-ui/core": "*",
    "@material-ui/icons": "*",
    "@material-ui/lab": "*",
    "react-router-dom": "*",
    "@types/react-router-dom": "*",
    "react-final-form": "*"
  }
}

The script that generates the dummy package.json:

// PackageManager is an abstraction for npm/yarn
const pm = new PackageManager(cwd);

const version = await pm.installedVersion(CORE_PACKAGE_NAME); // npm list $packageName -> then get version, or read package.json directly from node_modules/$packageName/package.json
const corePkg = await pm.info(CORE_PACKAGE_NAME, version);

logger.info("Generating a dummy package.json for auto-import suggestions");
const pkg = {
    notes: [
        "DO NOT DELETE OR MODIFY THIS FILE!",
        "These dependencies are necessary for Typescript auto-import suggestions to work",
        `This file will get updated after every install with dependencies from ${CORE_PACKAGE_NAME}`,
    ],
    peerDependencies: {
        ...Object.fromEntries(corePkg.injectableDependencies!.map((d) => [d, "*"])), // not all dependencies need to be added for import suggestions. injectableDependencies is a list of dependencies that we want users of the framework to have access to
    },
};
await writeJson(path.resolve(cwd, "src", "package.json"), pkg);
cjdreiss commented 3 years ago

@abdusco we didn't find a work around. We went back to using full, "normal", package.json files in our consuming apps and built a CLI upgrade tool to help manage the package.json files in our apps during upgrades.

You're solution seems pretty clever though, if we get tired of the "normal" way of doing it in the future, maybe I'll try this but for now we're going to stick with the usual way of having a proper package.json file.

abdusco commented 3 years ago

One problem we keep having is that mutating with package.json with preinstall & postinstall scripts breaks lock files. This means we cannot guarantee immutable builds on CI/CD machines.

The method we used was to inject dependencies from a "super package" (the one whose dependencies are controlled and maintained by us) into package.json using postinstall script. This means after a package is installed/updated and lock files are regenerated, postinstall script runs and breaks the link between the lock file and the package.json. This means you can't use npm ci, as it refuses to continue (you can disable integrity check, but then what's the point). Nasty stuff.

This led me to a journey through Typescript source and I realized that I could use a separate package.json for it.

abdusco commented 3 years ago

Another method that I've discovered is to add a types.d.ts file that re-exports types from subdependencies.

export * from 'react-admin'
export * from '@material-ui/core'
// ...

This works, but if two packages export a symbol with the same name, then the first one wins. For example react-admin exports <List/> component, so does @material-ui/core (albeit with very different functionalities), but you get import suggestions for only the one from react-admin.

Using a second package.json doesn't have that problem, though. I get suggestions for <List> components from both packages.

josephdpurcell commented 2 years ago

This comment is intended to summarize approaches to the problem identified in this ticket.

First, I'll share that I too am running this problem. I have package A whose package.json lists peerDependencies B and C. I want consumers of package A to be able to npm install package A and have B and C installed automatically, this works great. However, I want them to also be able to use VSCode to autocomplete anything from A, B, or C. This does not work, only A will autocomplete. This thread and other threads explain very well that this is the intended behavior. (Note that in my use case these are private packages, but I think these principles apply the same.) And, to put a real-world use case to the problem I want to bundle a suite of NestJS and related packages together as a starting point for building applications, conceptually similar to how Drupal has distros. A "bundle" package would allow distributing a single package that does the hard work of specifying the right version compatibility of all packages in the bundle.

I see 5 umbrella approaches to this problem. In the examples below I'll refer to these fake package names:

1 - Don't import dependencies of dependencies

In this scenario, you change the assumptions. Instead of expecting consumers of package A to be able to autocomplete from B and C, you say that consumers of package A can only autocomplete package A. It would be the responsibility of A to expose any functionality that B or C provide. I think this perspective was succinctly phrased in @RyanCavanaugh's comment.

CON: A "bundle" package isn't valid in this scenario. If a consumer needs functionality from B, then package A must expose that functionality, e.g. using adapter pattern.

I don't have an example of how this would work; it would be highly specific to the code in A, B, and C.

2 - Directly list dependencies

Have consumers of package A list B and C as direct dependencies, e.g. using peerDependencies. See @abdusco's comment. To avoid version conflicts between your project's specific versions of B and C with package A's versions of B and C, consider using loose specificity in your project, e.g. "@vendor1/package1": "*".

CON: Bubbling up dependencies B and C requires manual upkeep or custom tooling, and may break lockfiles in CI tooling (see @abdusco's comment).

Expand to see example The package.json for your project: ``` { "dependencies": { "@vendor1/package1": "^1.0.0" }, "peerDependencies": { "@vendor2/package1": "*", "@vendor3/package1": "*" } } ``` The package.json for package A: ``` { "peerDependencies": { "@vendor2/package1": "^1.0.0", "@vendor3/package1": "^1.0.0" } } ```

3 - Add a dummy package.json solely for autocomplete

Have consumers of package A list B and C as direct dependencies in a dummy package.json that is within the ts include path, e.g. src/package.json. See @abdusco's comment.

CON: Bubbling up dependencies B and C requires manual upkeep or custom tooling, and the fact you have multiple package.json files may be confusing.

Expand to see example Your project's directory structure: ``` |-- package.json |-- src |-- package.json # This is a dummy, not read by npm but is read by ts / vscode for autocomplete |-- somecode.ts ``` The package.json in the root of your project: ``` { "dependencies": { "@vendor1/package1": "^1.0.0" } } ``` The dummy package.json in `src/`: ``` { "peerDependencies": { "@vendor2/package1": "*", "@vendor3/package1": "*" } } ``` IMPORTANT! If your project' structure is more complicated, e.g. a monorepo with NestJS or NX, you will need a dummy package.json in each directory like so: ``` |-- package.json |-- apps/ |-- package.json # This dummy is used for all sub-directories |-- app1/ |-- src/ |-- somecode.ts # When editing this file, autocomplete will read from "apps/package.json" |-- app2/ |-- src/ |-- somecode.ts # When editing this file, autocomplete will read from "apps/package.json" |-- libs/ |-- package.json # This dummy is used for all sub-directories |-- lib1/ |-- src/ |-- somecode.ts # When editing this file, autocomplete will read from "libs/package.json" |-- lib2/ |-- src/ |-- somecode.ts # When editing this file, autocomplete will read from "libs/package.json" ```

4 - Re-export using type definitions

Have consumers of package A use a type definition that re-exports types from sub-dependencies B and C. See @abdusco's comment.

CON: Bubbling up dependencies B and C requires manual upkeep or custom tooling.

Expand to see example The tsconfig.json for your project: ``` { "compilerOptions": { "paths": { "@vendor2/*": [ "typings/vendor2/*" ], "@vendor3/*": [ "typings/vendor3/*" ], } } ``` The type definition for B at `typings/vendor2/index.d.ts`: ``` export * from '@vendor2/package1'; # Note: if there were multiple packages you could simply add them as long as they are from @vendor2. export * from '@vendor2/package2'; ``` The type definition for C at `typings/vendor3/index.d.ts`: ``` export * from '@vendor3/package1'; ```

5 - Package inheritance

Have consumers of package A use package inheritance. A StackOverflow thread points out that npm does not support package inheritance and is unlikely to do so. However, there are a few alternatives that could be explored to see if they are valid for your situation:

CON: NPM does not support inheritance, so custom tooling is required or switching dependency managers.

For examples, look at the relevant tools listed above.


I hope this is a helpful and accurate summary; there are a lot of related tickets that I have spent considerable time wading through and I think this is the proper spot to comment. I also hope this saves others time and inspires identification of a simpler solution. If I have said anything in error please provide correction and I will update.

Edit 2022-07-08 15:47 ET: Expanded from 3 to 5 approaches.

abdusco commented 2 years ago

To mitigate versioning complications, the consumer should use loose specificity.

@josephdpurcell This is not quite true.

You could have a package.json containing a dependency at a specific version:

{
  "dependencies": {
    "core-pkg": "1.2.3"
  }
}

when you npm install, it always installs the version 1.2.3. (The specificity of the dependency isn't loosened).

You then create another "fake" package.json containing the packages you need autocomplete for. You can specify any version for these, as no one should run npm install on that package.json.
Even if they do, defining those dependencies as peerDependencies will mean that the will be skipped during npm install, and nothing will be installed (there will be an empty node_modules directory under src/).

{
  "peerDependencies": {
    "sub-pkg-a": "1.2.3" // a real version
    "sub-pkg-b": "*" // or any version
  }
}

It's just used to signal Typescript to enable code completions for those packages, nothing more.

josephdpurcell commented 2 years ago

Well said, @abdusco, I like this statement: It's just used to signal Typescript to enable code completions for those packages, nothing more. I see now that you uncovered 3 different approaches! I edited my previous comment to reflect that. Also, I added an approach called "package inheritance" which isn't supported by NPM but is possible with custom tooling, I felt it worthy to include.

std4453 commented 1 year ago

Adding to @abdusco 's comment, you could add src/package.json to a personal .gitignore file to ignore the package.json directly inside src directories, so that your workaround won't affect other collaborators.

For the personal .gitignore file to work, you will have to set core.excludesfile with:

# or change that path to wherever you like
git config --global core.excludesfile $HOME/.gitignore