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

Add string literal completions for package.json imports field #57718

Merged
merged 27 commits into from
Oct 31, 2024
Merged

Add string literal completions for package.json imports field #57718

Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
238651d
Add string literal completions for `package.json` `imports` field
Andarist Mar 11, 2024
2adb14c
format
Andarist Mar 11, 2024
22b6148
Merge remote-tracking branch 'origin/main' into pkg-json-imports-comp…
Andarist Sep 28, 2024
08eaa16
fixed server fixtures
Andarist Sep 28, 2024
083c5b8
drop trailing slash support
Andarist Sep 29, 2024
b48f752
fix support for trailing slash
Andarist Oct 9, 2024
07f6c3c
small tweaks
Andarist Oct 9, 2024
117415a
fixed lint
Andarist Oct 9, 2024
db0d416
pass `moduleSpecifierResolutionHost` everywhere
Andarist Oct 12, 2024
294c7da
`getMatchesWithPrefix` helper
Andarist Oct 12, 2024
0d0b881
add more tests but break some of the existing ones
Andarist Oct 13, 2024
475c0b1
fixed the incorrectly copy-pasted test
Andarist Oct 13, 2024
a0ea1b9
handle `inputDirectories`
Andarist Oct 14, 2024
c00448b
add more tests, some of them still failing
Andarist Oct 14, 2024
e96bd5a
add test with `allowImportingTsExtensions`
Andarist Oct 14, 2024
c36d20a
add extra tests with wildcard in the middle
Andarist Oct 15, 2024
0f4a6f8
add pkg scope test
Andarist Oct 16, 2024
6759383
remove broken path
Andarist Oct 16, 2024
48b2c78
limit imports lookup
Andarist Oct 16, 2024
56a1563
dedupe imports/exports lookup
Andarist Oct 16, 2024
b84468a
handle declarationDir separately
Andarist Oct 16, 2024
a80cdfa
use `ValidationFlags.AllowWildcard`
Andarist Oct 17, 2024
96209a9
allowJs accepts a boolean, not String(boolea), doh
Andarist Oct 17, 2024
17b2aae
split `importsLookup` out of `nodeModulesDirectoryOrImportsLookup` fo…
Andarist Oct 17, 2024
22e4d49
Relax check for `inputExtension`
Andarist Oct 17, 2024
a18104e
swap `matchingSuffixes` to change the priority order for `trimPrefixA…
Andarist Oct 17, 2024
36bed48
fmt
Andarist Oct 17, 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
38 changes: 19 additions & 19 deletions src/compiler/moduleNameResolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2618,8 +2618,8 @@ function loadModuleFromExports(scope: PackageJsonInfo, extensions: Extensions, s
mainExport = (scope.contents.packageJsonContent.exports as MapLike<unknown>)["."];
}
if (mainExport) {
const loadModuleFromTargetImportOrExport = getLoadModuleFromTargetImportOrExport(extensions, state, cache, redirectedReference, subpath, scope, /*isImports*/ false);
return loadModuleFromTargetImportOrExport(mainExport, "", /*pattern*/ false, ".");
const loadModuleFromTargetExportOrImport = getLoadModuleFromTargetExportOrImport(extensions, state, cache, redirectedReference, subpath, scope, /*isImports*/ false);
return loadModuleFromTargetExportOrImport(mainExport, "", /*pattern*/ false, ".");
}
}
else if (allKeysStartWithDot(scope.contents.packageJsonContent.exports as MapLike<unknown>)) {
Expand All @@ -2629,7 +2629,7 @@ function loadModuleFromExports(scope: PackageJsonInfo, extensions: Extensions, s
}
return toSearchResult(/*value*/ undefined);
}
const result = loadModuleFromImportsOrExports(extensions, state, cache, redirectedReference, subpath, scope.contents.packageJsonContent.exports, scope, /*isImports*/ false);
const result = loadModuleFromExportsOrImports(extensions, state, cache, redirectedReference, subpath, scope.contents.packageJsonContent.exports, scope, /*isImports*/ false);
if (result) {
return result;
}
Expand Down Expand Up @@ -2663,7 +2663,7 @@ function loadModuleFromImports(extensions: Extensions, moduleName: string, direc
return toSearchResult(/*value*/ undefined);
}

const result = loadModuleFromImportsOrExports(extensions, state, cache, redirectedReference, moduleName, scope.contents.packageJsonContent.imports, scope, /*isImports*/ true);
const result = loadModuleFromExportsOrImports(extensions, state, cache, redirectedReference, moduleName, scope.contents.packageJsonContent.imports, scope, /*isImports*/ true);
if (result) {
return result;
}
Expand Down Expand Up @@ -2693,30 +2693,30 @@ export function comparePatternKeys(a: string, b: string): Comparison {
return Comparison.EqualTo;
}

function loadModuleFromImportsOrExports(extensions: Extensions, state: ModuleResolutionState, cache: ModuleResolutionCache | undefined, redirectedReference: ResolvedProjectReference | undefined, moduleName: string, lookupTable: object, scope: PackageJsonInfo, isImports: boolean): SearchResult<Resolved> | undefined {
const loadModuleFromTargetImportOrExport = getLoadModuleFromTargetImportOrExport(extensions, state, cache, redirectedReference, moduleName, scope, isImports);
function loadModuleFromExportsOrImports(extensions: Extensions, state: ModuleResolutionState, cache: ModuleResolutionCache | undefined, redirectedReference: ResolvedProjectReference | undefined, moduleName: string, lookupTable: object, scope: PackageJsonInfo, isImports: boolean): SearchResult<Resolved> | undefined {
const loadModuleFromTargetExportOrImport = getLoadModuleFromTargetExportOrImport(extensions, state, cache, redirectedReference, moduleName, scope, isImports);

if (!endsWith(moduleName, directorySeparator) && !moduleName.includes("*") && hasProperty(lookupTable, moduleName)) {
const target = (lookupTable as { [idx: string]: unknown; })[moduleName];
return loadModuleFromTargetImportOrExport(target, /*subpath*/ "", /*pattern*/ false, moduleName);
const target = (lookupTable as MapLike<unknown>)[moduleName];
return loadModuleFromTargetExportOrImport(target, /*subpath*/ "", /*pattern*/ false, moduleName);
}
const expandingKeys = toSorted(filter(getOwnKeys(lookupTable as MapLike<unknown>), k => hasOneAsterisk(k) || endsWith(k, "/")), comparePatternKeys);
for (const potentialTarget of expandingKeys) {
if (state.features & NodeResolutionFeatures.ExportsPatternTrailers && matchesPatternWithTrailer(potentialTarget, moduleName)) {
const target = (lookupTable as { [idx: string]: unknown; })[potentialTarget];
const target = (lookupTable as MapLike<unknown>)[potentialTarget];
const starPos = potentialTarget.indexOf("*");
const subpath = moduleName.substring(potentialTarget.substring(0, starPos).length, moduleName.length - (potentialTarget.length - 1 - starPos));
return loadModuleFromTargetImportOrExport(target, subpath, /*pattern*/ true, potentialTarget);
return loadModuleFromTargetExportOrImport(target, subpath, /*pattern*/ true, potentialTarget);
}
else if (endsWith(potentialTarget, "*") && startsWith(moduleName, potentialTarget.substring(0, potentialTarget.length - 1))) {
const target = (lookupTable as { [idx: string]: unknown; })[potentialTarget];
const target = (lookupTable as MapLike<unknown>)[potentialTarget];
const subpath = moduleName.substring(potentialTarget.length - 1);
return loadModuleFromTargetImportOrExport(target, subpath, /*pattern*/ true, potentialTarget);
return loadModuleFromTargetExportOrImport(target, subpath, /*pattern*/ true, potentialTarget);
}
else if (startsWith(moduleName, potentialTarget)) {
const target = (lookupTable as { [idx: string]: unknown; })[potentialTarget];
const target = (lookupTable as MapLike<unknown>)[potentialTarget];
const subpath = moduleName.substring(potentialTarget.length);
return loadModuleFromTargetImportOrExport(target, subpath, /*pattern*/ false, potentialTarget);
return loadModuleFromTargetExportOrImport(target, subpath, /*pattern*/ false, potentialTarget);
}
}

Expand All @@ -2736,9 +2736,9 @@ function hasOneAsterisk(patternKey: string): boolean {
/**
* Gets the self-recursive function specialized to retrieving the targeted import/export element for the given resolution configuration
*/
function getLoadModuleFromTargetImportOrExport(extensions: Extensions, state: ModuleResolutionState, cache: ModuleResolutionCache | undefined, redirectedReference: ResolvedProjectReference | undefined, moduleName: string, scope: PackageJsonInfo, isImports: boolean) {
return loadModuleFromTargetImportOrExport;
function loadModuleFromTargetImportOrExport(target: unknown, subpath: string, pattern: boolean, key: string): SearchResult<Resolved> | undefined {
function getLoadModuleFromTargetExportOrImport(extensions: Extensions, state: ModuleResolutionState, cache: ModuleResolutionCache | undefined, redirectedReference: ResolvedProjectReference | undefined, moduleName: string, scope: PackageJsonInfo, isImports: boolean) {
return loadModuleFromTargetExportOrImport;
function loadModuleFromTargetExportOrImport(target: unknown, subpath: string, pattern: boolean, key: string): SearchResult<Resolved> | undefined {
if (typeof target === "string") {
if (!pattern && subpath.length > 0 && !endsWith(target, "/")) {
if (state.traceEnabled) {
Expand Down Expand Up @@ -2801,7 +2801,7 @@ function getLoadModuleFromTargetImportOrExport(extensions: Extensions, state: Mo
if (condition === "default" || state.conditions.includes(condition) || isApplicableVersionedTypesKey(state.conditions, condition)) {
traceIfEnabled(state, Diagnostics.Matched_0_condition_1, isImports ? "imports" : "exports", condition);
const subTarget = (target as MapLike<unknown>)[condition];
const result = loadModuleFromTargetImportOrExport(subTarget, subpath, pattern, key);
const result = loadModuleFromTargetExportOrImport(subTarget, subpath, pattern, key);
if (result) {
traceIfEnabled(state, Diagnostics.Resolved_under_condition_0, condition);
traceIfEnabled(state, Diagnostics.Exiting_conditional_exports);
Expand All @@ -2826,7 +2826,7 @@ function getLoadModuleFromTargetImportOrExport(extensions: Extensions, state: Mo
return toSearchResult(/*value*/ undefined);
}
for (const elem of target) {
const result = loadModuleFromTargetImportOrExport(elem, subpath, pattern, key);
const result = loadModuleFromTargetExportOrImport(elem, subpath, pattern, key);
if (result) {
return result;
}
Expand Down
17 changes: 17 additions & 0 deletions src/compiler/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@ import {
getParseTreeNode,
getPathComponents,
getPathFromPathComponents,
getRelativePathFromDirectory,
getRelativePathToDirectoryOrUrl,
getResolutionModeOverride,
getRootLength,
Expand Down Expand Up @@ -496,6 +497,7 @@ import {
ResolvedModuleWithFailedLookupLocations,
ResolvedTypeReferenceDirective,
ResolvedTypeReferenceDirectiveWithFailedLookupLocations,
resolvePath,
returnFalse,
ReturnStatement,
returnUndefined,
Expand Down Expand Up @@ -6493,6 +6495,21 @@ export function getPossibleOriginalInputExtensionForExtension(path: string): Ext
[Extension.Tsx, Extension.Ts, Extension.Jsx, Extension.Js];
Andarist marked this conversation as resolved.
Show resolved Hide resolved
}

/** @internal */
export function getPossibleOriginalInputPathWithoutChangingExt(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is kinda a reverse of getOutputPathWithoutChangingExt:

TypeScript/src/compiler/emitter.ts

Lines 542 to 554 in 3fca8c8

function getOutputPathWithoutChangingExt(
inputFileName: string,
ignoreCase: boolean,
outputDir: string | undefined,
getCommonSourceDirectory: () => string,
): string {
return outputDir ?
resolvePath(
outputDir,
getRelativePathFromDirectory(getCommonSourceDirectory(), inputFileName, ignoreCase),
) :
inputFileName;
}

filePath: string,
ignoreCase: boolean,
outputDir: string | undefined,
getCommonSourceDirectory: () => string,
): string {
return outputDir ?
resolvePath(
getCommonSourceDirectory(),
getRelativePathFromDirectory(outputDir, filePath, ignoreCase),
) :
filePath;
}

/**
* Returns 'undefined' if and only if 'options.paths' is undefined.
*
Expand Down
2 changes: 1 addition & 1 deletion src/harness/vfsUtil.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1148,7 +1148,7 @@ export class FileSystem {
for (const key of Object.keys(files)) {
const value = normalizeFileSetEntry(files[key]);
const path = dirname ? vpath.resolve(dirname, key) : key;
vpath.validate(path, vpath.ValidationFlags.Absolute);
vpath.validate(path, vpath.ValidationFlags.Absolute | vpath.ValidationFlags.AllowWildcard);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea if this is completely correct. I added it to make pathCompletionsPackageJsonImportsSrcNoDistWildcard6 pass as it's using ? in the component path component.

I have added that test based on the existing pathCompletionsPackageJsonExportsWildcard6. The difference is that the test I have added is a fourslash/server test and it follows a slightly different codepath.

The existing one validates here:

TypeScript/src/harness/vfsUtil.ts

Lines 1104 to 1108 in b8e4ed8

private _resolve(path: string) {
return this._cwd
? vpath.resolve(this._cwd, vpath.validate(path, vpath.ValidationFlags.RelativeOrAbsolute | vpath.ValidationFlags.AllowWildcard))
: vpath.validate(path, vpath.ValidationFlags.Absolute | vpath.ValidationFlags.AllowWildcard);
}

And the ValidationFlags.AllowWildcard was added there here

On the other hand, the fourslash/server validates here, where I'm adding this comment. This codepath was added as part of #20763 - which proceeds the PR that introduced ValidationFlags.AllowWildcard. So perhaps this was just a harmless omission in that newer PR. I don't know why this is a flag in the first place though so 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

@rbuckton can you advise?


// eslint-disable-next-line no-restricted-syntax
if (value === null || value === undefined || value instanceof Rmdir || value instanceof Unlink) {
Expand Down
Loading
Loading