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.

Sign up for GitHub

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

Let AutoImportProvider find non-declaration files in wildcard exports... #56848

Merged
merged 5 commits into from
Jan 3, 2024
Merged

Let AutoImportProvider find non-declaration files in wildcard exports... #56848

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ec6b6e0
Let AutoImportProvider find non-declaration files in wildcard exports
andrewbranch Dec 21, 2023
437d1c7
Fix lint
andrewbranch Dec 21, 2023
d77d452
Add helper for changing full extension
andrewbranch Jan 3, 2024
775554f
Handle when `newExtension` does not start with .
andrewbranch Jan 3, 2024
a9c9548
Fix ternary/addition precedence
andrewbranch Jan 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions src/compiler/moduleNameResolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ import {
GetCanonicalFileName,
getCommonSourceDirectory,
getCompilerOptionValue,
getDeclarationEmitExtensionForPath,
getDirectoryPath,
GetEffectiveTypeRootsHost,
getEmitModuleKind,
Expand Down Expand Up @@ -2292,8 +2291,8 @@ function loadEntrypointsFromExportMap(
/*excludes*/ undefined,
[
isDeclarationFileName(target)
? replaceFirstStar(target, "**/*")
: changeAnyExtension(replaceFirstStar(target, "**/*"), getDeclarationEmitExtensionForPath(target)),
? changeAnyExtension(replaceFirstStar(target, "**/*"), ".*", supportedDeclarationExtensions, /*ignoreCase*/ true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like we are only filtering out replacing to *.* only if extension is of type ".d.someext.ts"
Was that intentional. I wonder if we can just always do changeAnyExtension(replaceFirstStar(target, "**/*"), ".*") irrespective of extension .

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is changeAnyExtension(path, ".*") only replaces from the last ., so that would result in foo.d.ts becoming foo.d.*, when we need it to become foo.* instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but if i locally change this to do changeAnyExtension(replaceFirstStar(target, "**/*"), ".*"), none of the tests fail ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. There's currently no test where the exports target has a declaration file extension but we find a .ts on disk instead. While that should arguably work, it's not very realistic.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably we don't have many tests with export maps pointing explicitly at declaration files with js or implementation files alongside them that we'd prefer to lookup - it's usually the other way around, so only finding matches for name.d.* gets us what we want almost all the time.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, exactly. Still, I running with an include of name.d.* feels too much like a mistake to me. I made a new helper changeFullExtension that recognizes and swaps all declaration file extensions (including ones for arbitrary file extensions, which the current code doesn't handle correctly), and am just trying to figure out how to push the commit I made on my host machine from my dev box 😅

: changeAnyExtension(replaceFirstStar(target, "**/*"), ".*"),
],
).forEach(entry => {
entrypoints = appendIfUnique(entrypoints, {
Expand Down
16 changes: 12 additions & 4 deletions tests/baselines/reference/tsserver/fourslashServer/autoImportProvider_wildcardExports1.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,9 +171,10 @@ Info seq [hh:mm:ss:mss] Files (4)

Info seq [hh:mm:ss:mss] -----------------------------------------------
Info seq [hh:mm:ss:mss] FileWatcher:: Added:: WatchInfo: /package.json 250 undefined WatchType: package.json file
Info seq [hh:mm:ss:mss] AutoImportProviderProject: found 5 root files in 1 dependencies in * ms
Info seq [hh:mm:ss:mss] AutoImportProviderProject: found 6 root files in 1 dependencies in * ms
Info seq [hh:mm:ss:mss] FileWatcher:: Added:: WatchInfo: /node_modules/pkg/a/a1.d.ts 500 undefined WatchType: Closed Script info
Info seq [hh:mm:ss:mss] FileWatcher:: Added:: WatchInfo: /node_modules/pkg/b/b1.d.ts 500 undefined WatchType: Closed Script info
Info seq [hh:mm:ss:mss] FileWatcher:: Added:: WatchInfo: /node_modules/pkg/b/b2.d.mts 500 undefined WatchType: Closed Script info
Info seq [hh:mm:ss:mss] FileWatcher:: Added:: WatchInfo: /node_modules/pkg/c/c1.d.ts 500 undefined WatchType: Closed Script info
Info seq [hh:mm:ss:mss] FileWatcher:: Added:: WatchInfo: /node_modules/pkg/c/subfolder/c2.d.mts 500 undefined WatchType: Closed Script info
Info seq [hh:mm:ss:mss] FileWatcher:: Added:: WatchInfo: /node_modules/pkg/d/d1.d.mts 500 undefined WatchType: Closed Script info
Expand All @@ -183,9 +184,10 @@ Info seq [hh:mm:ss:mss] FileWatcher:: Added:: WatchInfo: /node_modules/pkg/b/pa
Info seq [hh:mm:ss:mss] FileWatcher:: Added:: WatchInfo: /node_modules/pkg/c/package.json 2000 undefined Project: /dev/null/autoImportProviderProject1* WatchType: File location affecting resolution
Info seq [hh:mm:ss:mss] Finishing updateGraphWorker: Project: /dev/null/autoImportProviderProject1* Version: 1 structureChanged: true structureIsReused:: Not Elapsed:: *ms
Info seq [hh:mm:ss:mss] Project '/dev/null/autoImportProviderProject1*' (AutoImportProvider)
Info seq [hh:mm:ss:mss] Files (5)
Info seq [hh:mm:ss:mss] Files (6)
/node_modules/pkg/a/a1.d.ts Text-1 "export const a1: number;"
/node_modules/pkg/b/b1.d.ts Text-1 "export const b1: number;"
/node_modules/pkg/b/b2.d.mts Text-1 "export const NOT_REACHABLE: number;"
Comment on lines -186 to +190
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I could have kept the same strict extension filtering while mixing in the allowed TS implementation file extensions, keeping this unreachable file out of the AutoImportProviderProject compilation. But it doesn’t seem realistic for packages to ship declaration files that are literally unreachable, so doing the simpler thing and allowing all supported extensions seems fine. Updated the test to show that this file doesn’t result in a new auto-import.

/node_modules/pkg/c/c1.d.ts Text-1 "export const c1: number;"
/node_modules/pkg/c/subfolder/c2.d.mts Text-1 "export const c2: number;"
/node_modules/pkg/d/d1.d.mts Text-1 "export const d1: number;"
Expand All @@ -197,6 +199,8 @@ Info seq [hh:mm:ss:mss] Files (5)
node_modules/pkg/b/b1.d.ts
Root file specified for compilation
File is CommonJS module because 'node_modules/pkg/package.json' does not have field "type"
node_modules/pkg/b/b2.d.mts
Root file specified for compilation
node_modules/pkg/c/c1.d.ts
Root file specified for compilation
File is CommonJS module because 'node_modules/pkg/package.json' does not have field "type"
Expand Down Expand Up @@ -231,7 +235,7 @@ Info seq [hh:mm:ss:mss] Files (4)

Info seq [hh:mm:ss:mss] -----------------------------------------------
Info seq [hh:mm:ss:mss] Project '/dev/null/autoImportProviderProject1*' (AutoImportProvider)
Info seq [hh:mm:ss:mss] Files (5)
Info seq [hh:mm:ss:mss] Files (6)

Info seq [hh:mm:ss:mss] -----------------------------------------------
Info seq [hh:mm:ss:mss] Project '/dev/null/inferredProject1*' (Inferred)
Expand All @@ -257,6 +261,8 @@ watchedFiles::
{"pollingInterval":2000}
/node_modules/pkg/b/b1.d.ts: *new*
{"pollingInterval":500}
/node_modules/pkg/b/b2.d.mts: *new*
{"pollingInterval":500}
/node_modules/pkg/b/package.json: *new*
{"pollingInterval":2000}
/node_modules/pkg/c/c1.d.ts: *new*
Expand Down Expand Up @@ -315,7 +321,7 @@ Info seq [hh:mm:ss:mss] forEachExternalModuleToImportFrom autoImportProvider: *
Info seq [hh:mm:ss:mss] getExportInfoMap: done in * ms
Info seq [hh:mm:ss:mss] DirectoryWatcher:: Added:: WatchInfo: /node_modules 1 undefined WatchType: node_modules for closed script infos and package.jsons affecting module specifier cache
Info seq [hh:mm:ss:mss] Elapsed:: *ms DirectoryWatcher:: Added:: WatchInfo: /node_modules 1 undefined WatchType: node_modules for closed script infos and package.jsons affecting module specifier cache
Info seq [hh:mm:ss:mss] collectAutoImports: resolved 5 module specifiers, plus 0 ambient and 0 from cache
Info seq [hh:mm:ss:mss] collectAutoImports: resolved 5 module specifiers, plus 0 ambient and 1 from cache
Info seq [hh:mm:ss:mss] collectAutoImports: response is complete
Info seq [hh:mm:ss:mss] collectAutoImports: *
Info seq [hh:mm:ss:mss] getCompletionData: Semantic work: *
Expand Down Expand Up @@ -1108,6 +1114,8 @@ watchedFiles::
{"pollingInterval":2000}
/node_modules/pkg/b/b1.d.ts:
{"pollingInterval":500}
/node_modules/pkg/b/b2.d.mts:
{"pollingInterval":500}
/node_modules/pkg/b/package.json:
{"pollingInterval":2000}
/node_modules/pkg/c/c1.d.ts:
Expand Down
Loading
Loading