microsoft / TypeScript

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

auto import with tsconfig alias paths to node_modules #51058

Open paul-vd opened 2 years ago

paul-vd commented 2 years ago

Does this issue occur when all extensions are disabled?: Yes

Steps to Reproduce:

  1. Configure a tsconfig.json which has alias paths to node_modules

    {
      "compilerOptions": {
        "allowJs": true,
        "jsx": "react",
        "baseUrl": ".",
        "checkJs": true,
        "noEmit": true,
        "esModuleInterop": true,
        "resolveJsonModule": true,
        "paths": {
          "theme/*": [
            "./src/web/theme/*",
            "./node_modules/my-library/my-module-a/web/theme/*",
            "./node_modules/my-library/my-module-b/web/theme/*",
            "./node_modules/my-library/my-module-c/web/theme/*",
          ],
          "web/*": [
            "./src/web/*",
            "./node_modules/my-library/my-module-a/web/*",
            "./node_modules/my-library/my-module-b/web/*",
            "./node_modules/my-library/my-module-c/web/*",
          ],
          "server/*": ["./node_modules/my-library/src/server/*"],
          "config/*": [
            "./src/config/*",
            "./node_modules/my-library/my-module-a/config/*",
            "./node_modules/my-library/my-module-b/config/*",
            "./node_modules/my-library/my-module-c/config/*",
          ],
        }
      }
    }
  2. Try to import an existing file under the alias in node_modules

The autocomplete would prefer to use the my-library as a package instead of the shorter alias path, for example it would suggest:

import Image from "my-library/my-module-a/web/theme/Image"
import Image from "my-library/my-module-b/web/theme/Image"
import Image from "my-library/my-module-c/web/theme/Image"

But it will not suggest:

import Image from "theme/Image"

although that alias works perfectly fine when adding it manually.

cabal95 commented 4 months ago

I think this might be fixed/resolved already. Possibly by multiple PRs related to #53116.

The setup I have is a reference to package @rockrms/obsidian-framework - but I needed to use @Obsidian/* in the import lines - for various historical reasons. To make this work required a couple minor changes.

  1. In the package.json consuming the library put the reference in dependencies instead of devDependencies (putting it in devDependencies was probably a mistake on my part but I had assumed that was correct since it is not bundled at build time and instead provided at runtime by the host application).
  2. Add the alias to tsconfig.json as
    "paths": {
        "@Obsidian/*": [ "./node_modules/@rockrms/obsidian-framework/types/*" ]
    }
  3. Also in tsconfig.json, set moduleResolution to NodeNext.
  4. In the @rockrms/obsidian-framework package, set exports value to ./types/*.d.ts.

All 4 of these steps were required, but it now correctly recommends @Obsidian/* instead of @rockrms/obsidian-framework/*.

image

(Edit: This was tested in VS Code 1.90.0)

phulin commented 1 week ago

I have not been able to get the "correct" behavior here. For example, in this minimal example: https://github.com/phulin/import-test. We set up a path alias into lodash in tsconfig.json:

{
  "compilerOptions": {
    "module": "ESNext",
    "moduleResolution": "Bundler",
    "paths": {
      "isEqual": ["./node_modules/lodash/isEqual"],
    }
  },
  "include": ["src"]
}

But then VS Code (vis TS) suggests only to add the import from lodash - using the isEqual alias is not even an option. Entirely possible this is my misconfiguration, but it doesn't seem to me like this is fixed. VS Code 1.93.0, TS 5.6.3.

Image

phulin commented 1 week ago

Further digging: the following test fails.

/// <reference path="fourslash.ts" />

// @Filename: /package1/tsconfig.json
//// {
////   "compilerOptions": {
////     "paths": {
////       "dist": ["./node_modules/package2/dist"]
////     }
////   },
////   "include": [
////     "src",
////     "node_modules/package2"
////   ]
//// }

// @Filename: /package1/src/file1.ts
//// bar/**/

// @Filename: /package1/node_modules/package2/dist/index.ts
//// export const bar = 0;

verify.importFixModuleSpecifiers("", ["dist"], {});

But removing node_modules causes it to succeed:

/// <reference path="fourslash.ts" />

// @Filename: /package1/tsconfig.json
//// {
////   "compilerOptions": {
////     "paths": {
////       "dist": ["./package2/dist"]
////     }
////   },
////   "include": [
////     "src",
////     "package2"
////   ]
//// }

// @Filename: /package1/src/file1.ts
//// bar/**/

// @Filename: /package1/package2/dist/index.ts
//// export const bar = 0;

verify.importFixModuleSpecifiers("", ["dist"], {});
phulin commented 1 week ago

OK. Here is the issue with the above, in case it's helpful to anyone:

The priority order in TS 5.6.3 for autofix module paths is the following, from computeModuleSpecifiers in moduleSpecifiers.ts:

    // Module specifier priority:
    //   1. "Bare package specifiers" (e.g. "@foo/bar") resulting from a path through node_modules to a package.json's "types" entry
    //   2. Specifiers generated using "paths" from tsconfig
    //   3. Non-relative specfiers resulting from a path through node_modules (e.g. "@foo/bar/path/to/file")
    //   4. Relative paths

This means that your import cannot resolve to a bare package specifier in #1. In practice, the way to block that is to ensure that the exporting package, package2 in the below example, defines the exports field in package.json but does NOT include an export for any of the paths files you are trying to target, and it means that your importing package must use moduleResolution other than classic (NodeNext and Bundler both work).

So, to get correct import suggestions for a paths variable that points deeply into a node_modules package:

  1. Use any moduleResolution other than classic.
  2. Include the node_modules path in tsconfig include, in addition to paths.
  3. Set up the exports field in the node_modules package such that it does not include an export for your paths targeted files.

Here is a minimal test specifying files that generate this behavior, in TS's mocha test format.

/// <reference path="fourslash.ts" />

// @Filename: /package1/tsconfig.json
//// {
////   "compilerOptions": {
////     "module": "ESNext",
////     "moduleResolution": "Bundler",
////     "paths": {
////       "package2-otherexport": ["./node_modules/package2/dist/otherexport"]
////     }
////   },
////   "include": [
////     "src",
////     "node_modules/package2"
////   ]
//// }

// @Filename: /package1/src/file1.ts
//// bar/**/

// @Filename: /package1/node_modules/package2/dist/index.ts
//// export default {};

// @Filename: /package1/node_modules/package2/dist/otherexport/index.ts
//// export const bar = 0;

// @Filename: /package1/node_modules/package2/package.json
//// { 
////   "name": "package2",
////   "exports": {
////     ".": "./dist/index.ts"
////   }
//// }

verify.importFixModuleSpecifiers("", ["package2-otherexport"], {});