Add auto-import for the `package.json` `imports` field #55015

emmatown · 2023-07-14T02:57:51Z

This treats package.json#imports as roughly equivalent to compilerOptions#paths in terms of deciding which specifier to use, it also prefers package.json#imports over compilerOptions#paths

While doing this, I found some broken behaviour around extension replacement which was already observable with the package.json#exports field auto-import but is more visible with package.json#imports so this fixes that as well. autoImportPackageJsonExportsSpecifierEndsInTs shows an example of what's fixed for package.json#exports. That fix and test is in a seperate commit if that's easier to review

src/compiler/moduleSpecifiers.ts

andrewbranch

Thank you for adding this!

tests/cases/fourslash/autoImportPackageJsonImportsLength1.ts

andrewbranch · 2023-08-15T15:50:37Z

src/compiler/moduleSpecifiers.ts

+    if (typeof cachedPackageJson !== "object" && cachedPackageJson !== undefined) {
+        return undefined;
+    }
+    const packageJsonContent = cachedPackageJson?.contents.packageJsonContent || readJson(packageJsonPath, host as { readFile(fileName: string): string | undefined });


Use JSON.parse for package.json files. readJson uses the TypeScript parser to construct an AST and then converts that to an object 😵 I’ve been meaning to audit existing uses of readJson and then rename it; I know some can be replaced.

Yeah, I saw that 😅. I was aiming to consistent with the implementation of auto-import for the exports field. I was thinking maybe that should be left for a seperate PR to avoid more ad-hoc handling of package.json reading? Sorry, I was thinking of some other code, I've updated it to JSON.parse.

Use JSON.parse for package.json files. readJson uses the TypeScript parser to construct an AST and then converts that to an object 😵 I’ve been meaning to audit existing uses of readJson and then rename it; I know some can be replaced.

I've actually checked btw and all the usages of readJson are for reading package.jsons and the readJsonOrUndefined function that's used by readJson is only used for build info so all of them can really be replaced with JSON.parse

I'm not sure that replacing everything with JSON.parse is generally correct, because it'll throw if the file has malformed JSON. The services project has a helper, tryParseJson, which catches this, and that's used by its createPackageJsonInfo (which then makes its package.json cache).

Yes, JSON.parse should be in a try/catch, particularly when reading a local package.json as opposed to one in node_modules. It’s virtually impossible for a node_modules package.json to be malformed so I don’t care as much there. My point was only that we shouldn’t be creating TypeScript AST nodes and issuing parse diagnostics just to get an object representation of a JSON file.

diff --git a/src/compiler/moduleSpecifiers.ts b/src/compiler/moduleSpecifiers.ts index 4dcf24ea4b..d6022afe4b 100644 --- a/src/compiler/moduleSpecifiers.ts +++ b/src/compiler/moduleSpecifiers.ts @@ -90,7 +90,6 @@ import { pathIsBareSpecifier, pathIsRelative, PropertyAccessExpression, - readJson, removeExtension, removeFileExtension, removeSuffix, @@ -894,11 +893,11 @@ function tryGetModuleNameFromPackageJsonImports(moduleFileName: string, sourceDi } const packageJsonPath = combinePaths(ancestorDirectoryWithPackageJson, "package.json"); const cachedPackageJson = host.getPackageJsonInfoCache?.()?.getPackageJsonInfo(packageJsonPath); - if (typeof cachedPackageJson !== "object" && cachedPackageJson !== undefined) { + if (typeof cachedPackageJson !== "object" && cachedPackageJson !== undefined || !host.fileExists(packageJsonPath)) { return undefined; } - const packageJsonContent = cachedPackageJson?.contents.packageJsonContent || readJson(packageJsonPath, host as { readFile(fileName: string): string | undefined }); - const imports = (packageJsonContent as any).imports; + const packageJsonContent = cachedPackageJson?.contents.packageJsonContent || JSON.parse(host.readFile(packageJsonPath)!); + const imports = packageJsonContent?.imports; if (!imports) { return undefined; }

jakebailey · 2023-08-16T17:05:09Z

src/compiler/moduleSpecifiers.ts

+    if (typeof cachedPackageJson !== "object" && cachedPackageJson !== undefined || !host.fileExists(packageJsonPath)) {
+        return undefined;
+    }
+    const packageJsonContent = cachedPackageJson?.contents.packageJsonContent || JSON.parse(host.readFile(packageJsonPath)!);


I don't think this is a new problem as this file already has code that looks exactly like this, but I'm somewhat certain that we should be using readJson for this; if the JSON is malformed, we'll throw here and then I don't know what will happen. readJson is what's used to get stuff into the cache, and generally speaking other raw JSON.parse calls all appear to at least try/catch.

I see that there's another thread about this at #55015 (comment); if we want to leave this JSON.parse in here given we already do that, that's fine, but overall if we're avoiding readJson because it calls the wrong JSON parser, but the alternatiive is to call JSON.parse unguarded... that feels spooky to me.

tests/cases/fourslash/autoImportPackageJsonImportsLength1.ts

tests/cases/fourslash/autoImportPackageJsonImportsLength2.ts

jakebailey · 2023-08-16T17:23:13Z

src/compiler/moduleSpecifiers.ts

+    if (typeof cachedPackageJson !== "object" && cachedPackageJson !== undefined) {
+        return undefined;
+    }
+    const packageJsonContent = cachedPackageJson?.contents.packageJsonContent || readJson(packageJsonPath, host as { readFile(fileName: string): string | undefined });


I'm not sure that replacing everything with JSON.parse is generally correct, because it'll throw if the file has malformed JSON. The services project has a helper, tryParseJson, which catches this, and that's used by its createPackageJsonInfo (which then makes its package.json cache).

jakebailey

Reading the tests, I'm a little confused about the import conditions which map to .ts files...

jakebailey · 2023-08-16T17:24:38Z

src/compiler/moduleSpecifiers.ts

+    if (typeof cachedPackageJson !== "object" && cachedPackageJson !== undefined || !host.fileExists(packageJsonPath)) {
+        return undefined;
+    }
+    const packageJsonContent = cachedPackageJson?.contents.packageJsonContent || JSON.parse(host.readFile(packageJsonPath)!);


I see that there's another thread about this at #55015 (comment); if we want to leave this JSON.parse in here given we already do that, that's fine, but overall if we're avoiding readJson because it calls the wrong JSON parser, but the alternatiive is to call JSON.parse unguarded... that feels spooky to me.

tests/cases/fourslash/autoImportPackageJsonImportsLength2.ts

weswigham

None of the new tests test conditional imports - those probably need to be checked. Complex examples of nested conditions like

{
  "imports": {
    "#thing": {
        "types": { "import": "./types-esm/thing.d.mts", "require": "./types/thing.d.ts" },
        "default": { "import": "./esm/thing.mjs", "require": "./dist/thing.js" }
     }
  }
}

are actually more common in the wild than you'd think for exports, and likely have just as much utility for imports.

lenaggar · 2023-08-27T10:14:13Z

Just wanted to say, I'm excited about this 😍
Thank you 🙏🏻

MrHBS · 2023-09-03T20:40:31Z

@emmatown Almost there!

andrewbranch · 2023-09-11T23:12:47Z

I would love to get this into 5.3. @emmatown, are you interested in continuing? If not, someone on the team can probably address the remaining feedback and get it ready to merge.

src/compiler/moduleSpecifiers.ts

MrHBS · 2023-11-15T05:27:47Z

So umm what is left with this PR?

andrewbranch · 2023-11-15T19:50:41Z

@typescript-bot pack this

typescript-bot · 2023-11-15T19:50:43Z

Heya @andrewbranch, I've started to run the tarball bundle task on this PR at 1f9f214. You can monitor the build here.

typescript-bot · 2023-11-15T19:52:48Z

Hey @andrewbranch, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/158604/artifacts?artifactName=tgz&fileId=6668D7F09CE1B8B51DFE74B4A90ADB04B9F2C5DCE09CEE086EFFCF435E45884202&fileName=/typescript-5.4.0-insiders.20231115.tgz"
    }
}

and then running npm install.

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.4.0-pr-55015-10".;

andrewbranch · 2023-11-15T20:48:55Z

I deduplicated the emitter code per @sheetalkamat’s suggestion and found a bug by testing on a real project. I added a failing test but I’m out of time until the end of the day or maybe tomorrow. Will pick it back up and finish it unless @Andarist tells me he’s going to beat me to it.

src/compiler/moduleSpecifiers.ts

…rectory` cases

Andarist · 2023-11-15T23:35:44Z

Will pick it back up and finish it unless @Andarist tells me he’s going to beat me to it.

I pushed out a fix for this particular issue :P but I believe that a similar case is still broken within MatchingMode.Pattern case. To fix it there I propose to tweak startsWith and endsWith to optionally accept ignoreCase, WDYT?

andrewbranch · 2023-11-15T23:56:49Z

Sounds good to me 👍

Andarist · 2023-11-16T09:03:47Z

cool, done! :)

…son-imports

andrewbranch

Big thanks to @emmatown and @Andarist! 🌟

sheetalkamat · 2023-11-16T18:06:03Z

src/compiler/moduleSpecifiers.ts

                break;
            case MatchingMode.Pattern:
                const starPos = pathOrPattern.indexOf("*");
                const leadingSlice = pathOrPattern.slice(0, starPos);
                const trailingSlice = pathOrPattern.slice(starPos + 1);
-                if (startsWith(targetFilePath, leadingSlice) && endsWith(targetFilePath, trailingSlice)) {
+                if (extensionSwappedTarget && startsWith(extensionSwappedTarget, leadingSlice, ignoreCase) && endsWith(extensionSwappedTarget, trailingSlice, ignoreCase)) {


I think instead of changing starts with and endsWith, may be we need helper that uses getCanonicalFileName intead for startsWith(extensionSwappedTarget, leadingSlice, ignoreCase) && endsWith(extensionSwappedTarget, trailingSlice, ignoreCase) type of pattern (similar to tryRemovePrefix)

Is there a test case that I should add and cover for? Code style-wise, I think that I prefer the current approach since it feels simpler to me. But I, of course, know way less about potential edge cases of this whole section and overall coding guidelines in this codebase.

emmatown added 2 commits July 14, 2023 12:45

Fix extension replacement in auto-import for package.json#exports

7c0fef4

Add auto-import for package.json imports field

60ec33b

typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Jul 14, 2023

Jack-Works reviewed Jul 14, 2023

View reviewed changes

src/compiler/moduleSpecifiers.ts Show resolved Hide resolved

andrewbranch self-assigned this Jul 14, 2023

sandersn added this to Not started in PR Backlog Jul 28, 2023

sandersn requested a review from sheetalkamat July 28, 2023 23:06

sandersn moved this from Not started to Waiting on reviewers in PR Backlog Jul 28, 2023

andrewbranch reviewed Aug 15, 2023

View reviewed changes

Andarist mentioned this pull request Aug 15, 2023

Relax package.json#imports prefix nodejs/node#49182

Open

emmatown added 3 commits August 16, 2023 15:06

Fix using incorrect directory and add more tests

faef5fd

Merge branch 'main' into auto-import-package-json-imports

1d49909

andrewbranch approved these changes Aug 16, 2023

View reviewed changes

PR Backlog automation moved this from Waiting on reviewers to Needs merge Aug 16, 2023

andrewbranch requested review from weswigham and jakebailey August 16, 2023 16:53

jakebailey reviewed Aug 16, 2023

View reviewed changes

andrewbranch requested changes Aug 16, 2023

View reviewed changes

tests/cases/fourslash/autoImportPackageJsonImportsLength2.ts Show resolved Hide resolved

PR Backlog automation moved this from Needs merge to Waiting on author Aug 16, 2023

weswigham requested changes Aug 16, 2023

View reviewed changes

emmatown added 2 commits August 17, 2023 09:40

Merge branch 'main' into auto-import-package-json-imports

747e769

Use readJson again so they can all be replaced at the same time

01ceb65

emmatown added 2 commits September 12, 2023 09:18

Add test with conditions

864b9a3

Merge branch 'main' into auto-import-package-json-imports

d0407a9

typescript-bot removed the For Backlog Bug PRs that fix a backlog bug label Sep 11, 2023

support declaration remapping

566e543

sheetalkamat requested changes Oct 31, 2023

View reviewed changes

src/compiler/moduleSpecifiers.ts Outdated Show resolved Hide resolved

src/compiler/moduleSpecifiers.ts Outdated Show resolved Hide resolved

src/compiler/moduleSpecifiers.ts Outdated Show resolved Hide resolved

github-advanced-security bot found potential problems Oct 31, 2023

View reviewed changes

src/compiler/moduleSpecifiers.ts Fixed Show resolved Hide resolved

reprioritize checks

1f9f214

github-advanced-security bot found potential problems Oct 31, 2023

View reviewed changes

src/compiler/moduleSpecifiers.ts Fixed Show fixed Hide fixed

andrewbranch added 3 commits November 15, 2023 12:29

Deduplicate some emitter code

7a5c164

Fix lints

45194b2

Add failing test for when full path contains capital letters

8f2d0c3

sheetalkamat requested changes Nov 15, 2023

View reviewed changes

src/compiler/moduleSpecifiers.ts Outdated Show resolved Hide resolved

src/compiler/moduleSpecifiers.ts Show resolved Hide resolved

src/compiler/moduleSpecifiers.ts Outdated Show resolved Hide resolved

andrewbranch and others added 3 commits November 15, 2023 13:53

Move tryParseJson to be usable in compiler

8bef690

import tryParseJson after it got moved

cd84162

Fixed casing comparisons in MatchingMode.Exact and `MatchingMode.Di…

8d68392

…rectory` cases

fixed the casing issue in the MatchingMode.Pattern case

a9634ed

Andarist added 2 commits November 16, 2023 10:31

Merge remote-tracking branch 'origin/main' into auto-import-package-j…

7f52af2

…son-imports

fixed endsWith

4aab191

andrewbranch approved these changes Nov 16, 2023

View reviewed changes

sheetalkamat reviewed Nov 16, 2023

View reviewed changes

andrewbranch added 2 commits December 21, 2023 09:10

Merge branch 'main' into auto-import-package-json-imports

c435528

Fix bad merge artifacts

4f29d3a

andrewbranch merged commit fbcdb8c into microsoft:main Dec 21, 2023
19 checks passed

PR Backlog automation moved this from Waiting on author to Done Dec 21, 2023

Andarist deleted the auto-import-package-json-imports branch December 21, 2023 17:52

Add auto-import for the `package.json` `imports` field #55015

Add auto-import for the `package.json` `imports` field #55015

emmatown commented Jul 14, 2023

andrewbranch left a comment

andrewbranch Aug 15, 2023

emmatown Aug 16, 2023 •

edited

jakebailey Aug 16, 2023

andrewbranch Aug 16, 2023

jakebailey Aug 16, 2023

jakebailey Aug 16, 2023

jakebailey Aug 16, 2023

jakebailey left a comment

jakebailey Aug 16, 2023

weswigham left a comment

lenaggar commented Aug 27, 2023

MrHBS commented Sep 3, 2023

andrewbranch commented Sep 11, 2023

MrHBS commented Nov 15, 2023

andrewbranch commented Nov 15, 2023

typescript-bot commented Nov 15, 2023 •

edited

typescript-bot commented Nov 15, 2023 •

edited

andrewbranch commented Nov 15, 2023

Andarist commented Nov 15, 2023

andrewbranch commented Nov 15, 2023

Andarist commented Nov 16, 2023

andrewbranch left a comment

sheetalkamat Nov 16, 2023

Andarist Nov 22, 2023

Add auto-import for the package.json imports field #55015

Add auto-import for the package.json imports field #55015

Conversation

emmatown commented Jul 14, 2023

andrewbranch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emmatown Aug 16, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakebailey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weswigham left a comment

Choose a reason for hiding this comment

lenaggar commented Aug 27, 2023

MrHBS commented Sep 3, 2023

andrewbranch commented Sep 11, 2023

MrHBS commented Nov 15, 2023

andrewbranch commented Nov 15, 2023

typescript-bot commented Nov 15, 2023 • edited

typescript-bot commented Nov 15, 2023 • edited

andrewbranch commented Nov 15, 2023

Andarist commented Nov 15, 2023

andrewbranch commented Nov 15, 2023

Andarist commented Nov 16, 2023

andrewbranch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add auto-import for the `package.json` `imports` field #55015

Add auto-import for the `package.json` `imports` field #55015

emmatown Aug 16, 2023 •

edited

typescript-bot commented Nov 15, 2023 •

edited

typescript-bot commented Nov 15, 2023 •

edited