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

Module resolution: Module resolved to indirect dependency rather than direct local one when versions are the same #60556

Closed
grigasp opened this issue Nov 22, 2024 · 6 comments
Labels
Duplicate An existing issue was already created

Comments

@grigasp
Copy link

grigasp commented Nov 22, 2024

πŸ”Ž Search Terms

"local module resolution", "module resolution same version"

πŸ•— Version & Regression Information

  • Noticed in 5.6.3, also reproduced with next, 4.6, 4.8, 5.0.
  • I reviewed the FAQ for entries including "resol".

⏯ Playground Link

https://github.com/grigasp/ts-deps-issue

πŸ’» Code

Package [email protected] is released with the following:

export type A1 = number;

Locally we have this package at the same version, but with an additional type:

export type A1 = number;
export type A2 = string;

Locally we also have a package b, that has a dependency on a released version of [email protected] and exports the following type:

import { A1 } from "a";
export type B = A1;

Finally, locally we also have a package c, that has dependencies on local workspace versions of packages b and a and imports types from them both:

// Switching order of these import fixes the build
import { B } from "b";
import { A2 } from "a";

const b: B = 1;
const a2: A2 = "a2";

πŸ™ Actual behavior

When building package c, the build fails with error:

> [email protected] build
> tsc

src/index.ts:3:10 - error TS2305: Module '"a"' has no exported member 'A2'.

3 import { A2 } from "a";

πŸ™‚ Expected behavior

When building package c, TypeScript resolves package a to our local version which does have the A2 type, and the build succeeds.

Additional information about the issue

I tried running tsc -traceResolution for package c and it seems that both - released package and the local one - are assigned the same Package ID:

...
======== Resolving module 'a' from 'E:/temp/ts-deps-issue/packages/c/src/index.ts'. ========
...
======== Module name 'a' was successfully resolved to 'E:/temp/ts-deps-issue/packages/a/lib/index.d.ts' with Package ID 'a/lib/[email protected]'. ========

======== Resolving module 'a' from 'E:/temp/ts-deps-issue/packages/b/lib/index.d.ts'. ========
...
======== Module name 'a' was successfully resolved to 'E:/temp/ts-deps-issue/node_modules/.pnpm/a@file+packages+a+a-1.0.0.tgz/node_modules/a/lib/index.d.ts' with Package ID 'a/lib/[email protected]'. ========
...

I suppose that could cause a cache collision or something like that.

@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Nov 28, 2024
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 5.8.0 milestone Nov 28, 2024
@andrewbranch
Copy link
Member

I suppose that could cause a cache collision or something like that.

Exactly; we deduplicate packages by name and version; a different symptom of the same cause was closed as a design limitation here: #51976. I’m not sure what we could do about this.

@andrewbranch andrewbranch removed their assignment Dec 3, 2024
@andrewbranch andrewbranch added Duplicate An existing issue was already created and removed Bug A bug in TypeScript labels Dec 3, 2024
@andrewbranch andrewbranch removed this from the TypeScript 5.8.0 milestone Dec 3, 2024
@grigasp
Copy link
Author

grigasp commented Dec 5, 2024

I suppose that could cause a cache collision or something like that.

Exactly; we deduplicate packages by name and version; a different symptom of the same cause was closed as a design limitation here: #51976. I’m not sure what we could do about this.

Just throwing out a couple of ideas...

  • Could you use full path to the module for package id? So instead of a/lib/[email protected], it would be E:/temp/ts-deps-issue/packages/a/lib/index.d.ts or E:/temp/ts-deps-issue/node_modules/.pnpm/a@file+packages+a+a-1.0.0.tgz/node_modules/a/lib/index.d.ts.

  • Could you detect that the module is from a workspace package and for such packages use a special version identifier, e.g. a/lib/index.d.ts@workspace?

@andrewbranch
Copy link
Member

We deduplicate packages for a reason, so defeating the deduplication is just trading one problem for another. Including the same code multiple times can cause bogus errors. While it would fix your specific use case here, it could just as easily create duplicate declaration errors for someone else.

@grigasp
Copy link
Author

grigasp commented Dec 6, 2024

We deduplicate packages for a reason, so defeating the deduplication is just trading one problem for another. Including the same code multiple times can cause bogus errors. While it would fix your specific use case here, it could just as easily create duplicate declaration errors for someone else.

Isn't it the job of package manager to do that?

If a package is unintentionally duplicated, then there can be many more problems than just duplicate declarations. To name a few that quickly come to mind: instanceof checks, Symbol, global state. Deduplication on TS side will hide all that. IMO, all that should be left up to package manager and if it decides to duplicate a package, then it's either intentional, or a problem with dependencies setup, in which case I'd rather get an error at build time rather than run time.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Dec 6, 2024

Isn't it the job of package manager to do that?

This hasn't always been the case; older versions of npm would not deduplicate when sub-dependencies needed to be nested, and it's a huge performance problem for TS if we need to e.g. resolve 30 copies of [email protected] in different subfolders.

You can argue that an ideally set-up package manager wouldn't do this, but I'd also argue that a correctly set-up package manager can't produce two packages with the same version and the same name but with different contents, which is the setup your repro depends on. It doesn't really fly to say we should rely on one invariant while ignoring the other.

@typescript-bot
Copy link
Collaborator

This issue has been marked as "Duplicate" and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@typescript-bot typescript-bot closed this as not planned Won't fix, can't repro, duplicate, stale Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

4 participants