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: tsc unexpectedly rewrites the import path when exports field contains multiple entries resolving to the same file #56290

Open
haoqunjiang opened this issue Nov 2, 2023 · 4 comments
Labels
Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript

Comments

@haoqunjiang
Copy link

haoqunjiang commented Nov 2, 2023

Demo Repo

https://github.com/sodatea/ts-exports-rewrite

Which of the following problems are you reporting?

Something else more complicated which I'll explain in more detail

Demonstrate the defect described above with a code sample.

In the reproduction repo, the parse.ts file indirectly depends on the lru-cache module's type by importing the cache.ts file:

import { createCache } from './cache'
export const parseCache = createCache<{}>()

After compilation, in parse.d.ts, we would normally expect a import('lru-cache').LRUCache expression.
However, it actually becomes import("lru-cache/min").LRUCache.
On the other hand, the compiled cache.d.ts file still imports from lru-cache, not lru-cache/min.

This seems to be related to [email protected]'s exports field: https://unpkg.com/browse/[email protected]/package.json#L34
It contains 2 entries, ./min and ., both pointing to the same file. TypeScript picks the first entry.

If I modify the exports field to make . appear before ./min, then the compiled declaration file would use import("lru-cache") as expected.

While this isn't a very severe bug and has easy workarounds, this is quite unexpected.
Because lru-cache/min is an entry only available in exports field, this compilation result breaks the compatibility with older build tools. And it is quite hard to debug, because no documentation ever mentioned that the order of keys in the exports field would make a difference.

Run tsc --showConfig and paste its output here

{
    "compilerOptions": {
        "outDir": "./temp",
        "target": "esnext",
        "module": "esnext",
        "moduleResolution": "bundler",
        "esModuleInterop": true,
        "declaration": true,
        "emitDeclarationOnly": true,
        "types": []
    },
    "files": [
        "./cache.ts",
        "./parse.ts"
    ],
    "include": [
        "cache.ts",
        "parse.ts"
    ],
    "exclude": [
        "temp"
    ]
}

Run tsc --traceResolution and paste its output here

======== Resolving module 'lru-cache' from '/Users/haoqun/Reproductions/ts-exports-rewrite/cache.ts'. ========
Explicitly specified module resolution kind: 'Bundler'.
Resolving in CJS mode with conditions 'import', 'types'.
Found 'package.json' at '/Users/haoqun/Reproductions/ts-exports-rewrite/package.json'.
Loading module 'lru-cache' from 'node_modules' folder, target file types: TypeScript, JavaScript, Declaration, JSON.
Searching all ancestor node_modules directories for preferred extensions: TypeScript, Declaration.
Found 'package.json' at '/Users/haoqun/Reproductions/ts-exports-rewrite/node_modules/lru-cache/package.json'.
Entering conditional exports.
Matched 'exports' condition 'import'.
Entering conditional exports.
Matched 'exports' condition 'types'.
Using 'exports' subpath '.' with target './dist/mjs/index.d.ts'.
File '/Users/haoqun/Reproductions/ts-exports-rewrite/node_modules/lru-cache/dist/mjs/index.d.ts' exists - use it as a name resolution result.
Resolved under condition 'types'.
Exiting conditional exports.
Resolved under condition 'import'.
Exiting conditional exports.
Resolving real path for '/Users/haoqun/Reproductions/ts-exports-rewrite/node_modules/lru-cache/dist/mjs/index.d.ts', result '/Users/haoqun/Reproductions/ts-exports-rewrite/node_modules/lru-cache/dist/mjs/index.d.ts'.
======== Module name 'lru-cache' was successfully resolved to '/Users/haoqun/Reproductions/ts-exports-rewrite/node_modules/lru-cache/dist/mjs/index.d.ts' with Package ID 'lru-cache/dist/mjs/[email protected]'. ========
======== Resolving module './cache' from '/Users/haoqun/Reproductions/ts-exports-rewrite/parse.ts'. ========
Explicitly specified module resolution kind: 'Bundler'.
Resolving in CJS mode with conditions 'import', 'types'.
Loading module as file / folder, candidate module location '/Users/haoqun/Reproductions/ts-exports-rewrite/cache', target file types: TypeScript, JavaScript, Declaration, JSON.
File '/Users/haoqun/Reproductions/ts-exports-rewrite/cache.ts' exists - use it as a name resolution result.
======== Module name './cache' was successfully resolved to '/Users/haoqun/Reproductions/ts-exports-rewrite/cache.ts'. ========

Paste the package.json of the importing module, if it exists

{
  "name": "ts-exports-rewrite",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "build": "tsc"
  },
  "keywords": [],
  "author": "",
  "license": "ISC",
  "dependencies": {
    "lru-cache": "10.0.1",
    "typescript": "^5.2.2"
  }
}

Paste the package.json of the target module, if it exists

{
  "name": "lru-cache",
  "description": "A cache object that deletes the least-recently-used items.",
  "version": "10.0.1",
  "author": "Isaac Z. Schlueter <[email protected]>",
  "keywords": [
    "mru",
    "lru",
    "cache"
  ],
  "sideEffects": false,
  "scripts": {
    "build": "npm run prepare",
    "preprepare": "rm -rf dist",
    "prepare": "tsc -p tsconfig.json && tsc -p tsconfig-esm.json",
    "postprepare": "bash fixup.sh",
    "pretest": "npm run prepare",
    "presnap": "npm run prepare",
    "test": "c8 tap",
    "snap": "c8 tap",
    "preversion": "npm test",
    "postversion": "npm publish",
    "prepublishOnly": "git push origin --follow-tags",
    "format": "prettier --write .",
    "typedoc": "typedoc --tsconfig tsconfig-esm.json ./src/*.ts",
    "benchmark-results-typedoc": "bash scripts/benchmark-results-typedoc.sh",
    "prebenchmark": "npm run prepare",
    "benchmark": "make -C benchmark",
    "preprofile": "npm run prepare",
    "profile": "make -C benchmark profile"
  },
  "main": "./dist/cjs/index.js",
  "module": "./dist/mjs/index.js",
  "exports": {
    "./min": {
      "import": {
        "types": "./dist/mjs/index.d.ts",
        "default": "./dist/mjs/index.min.js"
      },
      "require": {
        "types": "./dist/cjs/index.d.ts",
        "default": "./dist/cjs/index.min.js"
      }
    },
    ".": {
      "import": {
        "types": "./dist/mjs/index.d.ts",
        "default": "./dist/mjs/index.js"
      },
      "require": {
        "types": "./dist/cjs/index.d.ts",
        "default": "./dist/cjs/index.js"
      }
    }
  },
  "repository": "git://github.com/isaacs/node-lru-cache.git",
  "devDependencies": {
    "@size-limit/preset-small-lib": "^7.0.8",
    "@types/node": "^20.2.5",
    "@types/tap": "^15.0.6",
    "benchmark": "^2.1.4",
    "c8": "^7.11.2",
    "clock-mock": "^1.0.6",
    "esbuild": "^0.17.11",
    "eslint-config-prettier": "^8.5.0",
    "marked": "^4.2.12",
    "mkdirp": "^2.1.5",
    "prettier": "^2.6.2",
    "size-limit": "^7.0.8",
    "tap": "^16.3.4",
    "ts-node": "^10.9.1",
    "tslib": "^2.4.0",
    "typedoc": "^0.24.6",
    "typescript": "^5.0.4"
  },
  "license": "ISC",
  "files": [
    "dist"
  ],
  "engines": {
    "node": "14 || >=16.14"
  },
  "prettier": {
    "semi": false,
    "printWidth": 70,
    "tabWidth": 2,
    "useTabs": false,
    "singleQuote": true,
    "jsxSingleQuote": false,
    "bracketSameLine": true,
    "arrowParens": "avoid",
    "endOfLine": "lf"
  },
  "tap": {
    "coverage": false,
    "node-arg": [
      "--expose-gc",
      "-r",
      "ts-node/register"
    ],
    "ts": false
  },
  "size-limit": [
    {
      "path": "./dist/mjs/index.js"
    }
  ]
}

Any other comments can go here

Have a nice day!

@jakebailey
Copy link
Member

jakebailey commented Nov 2, 2023

This is the same code from #56261 and #56265. Does this still happen in the nightly build? You didn't fill out the template oh, this is the module resolution template...

@haoqunjiang
Copy link
Author

Oh, I should've noticed the pull request! Thanks for the fix.
But unfortunately, it's still broken.

I've updated my reproduction repo with [email protected], it still emits the same parse.d.ts.

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Nov 3, 2023
@andrewbranch
Copy link
Member

I noticed this when investigating #56261 and suspected this would happen. I don’t think it’s a bug per se, but I understand why it’s undesirable in this case. But I don’t know what rule you could make that would catch this. I think the main reason you’re expecting the inferred module specifier to be "lru-cache" is because a different file resolved to the symbol in question that way, but it’s not generally true that two files can use the same module specifier (even when it’s a bare specifier) to refer to the same file. That’s true for two files of the same module format in the same directory, but if the files were in different directories, we’d have to see if they have different package.jsons in scope or different node_modules directories in scope, and at that point we’re pretty much just starting from scratch to see what module specifier we can use to resolve to the target file. And if we’re not explicitly relying on a cache entry that tells us we can use "lru-cache", there seems to be no good reason why we shouldn’t infer "lru-cache/min".

@andrewbranch andrewbranch added Suggestion An idea for TypeScript Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. and removed Needs Investigation This issue needs a team member to investigate its status. labels Nov 3, 2023
@haoqunjiang
Copy link
Author

Thanks for the detailed explanation!
Yeah, I agree it's an edge case that is hard to optimize given the implementation restrictions.
And the current behavior isn't very unreasonable - as long as it's consistent.

I'll take this reply as a reference and make pull requests to both vue and lru-cache. Thanks!

haoqunjiang added a commit to haoqunjiang/node-lru-cache that referenced this issue Nov 6, 2023
As both `./min` and `.` points to the same type definition file,
sometimes TypeScript will resolve the type definition file to the
`./min` key as it appears first.

microsoft/TypeScript#56290 (comment)

While this is not incorrect behavior, it breaks compatibility with older
build environments that do not support the `exports` field. (e.g.
TypeScript with `moduleResolution: "node"` instead of `bundler` or
`node16`), as the `./min` entry is only available through the `exports`
field.

Some Vue.js users are experiencing this issue:
vuejs/core#9521
@andrewbranch andrewbranch removed their assignment Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

No branches or pull requests

4 participants