Skip to content

Commit 0921eac

Browse files
andrewbranchandrewbranch
authored
Fix path completions for typesVersions (microsoft#49154)
* Fix path completions for typesVersions * Add more tests * Fix case when * is a fragment of a path component * Once a path pattern matches, only return completions from that pattern and higher priority ones * Fix iteration order issue * Aesthetics
1 parent f6a1713 commit 0921eac

10 files changed

+339
-27
lines changed

‎src/compiler/core.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -2299,7 +2299,7 @@ namespace ts {
22992299
return startsWith(getCanonicalFileName(str), getCanonicalFileName(prefix)) ? str.substring(prefix.length) : undefined;
23002300
}
23012301

2302-
function isPatternMatch({ prefix, suffix }: Pattern, candidate: string) {
2302+
export function isPatternMatch({ prefix, suffix }: Pattern, candidate: string) {
23032303
return candidate.length >= prefix.length + suffix.length &&
23042304
startsWith(candidate, prefix) &&
23052305
endsWith(candidate, suffix);

‎src/services/stringCompletions.ts

+60-24
Original file line numberDiff line numberDiff line change
@@ -448,10 +448,28 @@ namespace ts.Completions.StringCompletions {
448448

449449
fragment = ensureTrailingDirectorySeparator(fragment);
450450

451-
// const absolutePath = normalizeAndPreserveTrailingSlash(isRootedDiskPath(fragment) ? fragment : combinePaths(scriptPath, fragment)); // TODO(rbuckton): should use resolvePaths
452451
const absolutePath = resolvePath(scriptPath, fragment);
453452
const baseDirectory = hasTrailingDirectorySeparator(absolutePath) ? absolutePath : getDirectoryPath(absolutePath);
454453

454+
// check for a version redirect
455+
const packageJsonPath = findPackageJson(baseDirectory, host);
456+
if (packageJsonPath) {
457+
const packageJson = readJson(packageJsonPath, host as { readFile: (filename: string) => string | undefined });
458+
const typesVersions = (packageJson as any).typesVersions;
459+
if (typeof typesVersions === "object") {
460+
const versionPaths = getPackageJsonTypesVersionsPaths(typesVersions)?.paths;
461+
if (versionPaths) {
462+
const packageDirectory = getDirectoryPath(packageJsonPath);
463+
const pathInPackage = absolutePath.slice(ensureTrailingDirectorySeparator(packageDirectory).length);
464+
if (addCompletionEntriesFromPaths(result, pathInPackage, packageDirectory, extensions, versionPaths, host)) {
465+
// A true result means one of the `versionPaths` was matched, which will block relative resolution
466+
// to files and folders from here. All reachable paths given the pattern match are already added.
467+
return result;
468+
}
469+
}
470+
}
471+
}
472+
455473
const ignoreCase = !(host.useCaseSensitiveFileNames && host.useCaseSensitiveFileNames());
456474
if (!tryDirectoryExists(host, baseDirectory)) return result;
457475

@@ -505,37 +523,51 @@ namespace ts.Completions.StringCompletions {
505523
}
506524
}
507525

508-
// check for a version redirect
509-
const packageJsonPath = findPackageJson(baseDirectory, host);
510-
if (packageJsonPath) {
511-
const packageJson = readJson(packageJsonPath, host as { readFile: (filename: string) => string | undefined });
512-
const typesVersions = (packageJson as any).typesVersions;
513-
if (typeof typesVersions === "object") {
514-
const versionResult = getPackageJsonTypesVersionsPaths(typesVersions);
515-
const versionPaths = versionResult && versionResult.paths;
516-
const rest = absolutePath.slice(ensureTrailingDirectorySeparator(baseDirectory).length);
517-
if (versionPaths) {
518-
addCompletionEntriesFromPaths(result, rest, baseDirectory, extensions, versionPaths, host);
519-
}
520-
}
521-
}
522-
523526
return result;
524527
}
525528

529+
/** @returns whether `fragment` was a match for any `paths` (which should indicate whether any other path completions should be offered) */
526530
function addCompletionEntriesFromPaths(result: NameAndKind[], fragment: string, baseDirectory: string, fileExtensions: readonly string[], paths: MapLike<string[]>, host: LanguageServiceHost) {
531+
let pathResults: { results: NameAndKind[], matchedPattern: boolean }[] = [];
532+
let matchedPathPrefixLength = -1;
527533
for (const path in paths) {
528534
if (!hasProperty(paths, path)) continue;
529535
const patterns = paths[path];
530536
if (patterns) {
531-
for (const { name, kind, extension } of getCompletionsForPathMapping(path, patterns, fragment, baseDirectory, fileExtensions, host)) {
532-
// Path mappings may provide a duplicate way to get to something we've already added, so don't add again.
533-
if (!result.some(entry => entry.name === name)) {
534-
result.push(nameAndKind(name, kind, extension));
535-
}
537+
const pathPattern = tryParsePattern(path);
538+
if (!pathPattern) continue;
539+
const isMatch = typeof pathPattern === "object" && isPatternMatch(pathPattern, fragment);
540+
const isLongestMatch = isMatch && (matchedPathPrefixLength === undefined || pathPattern.prefix.length > matchedPathPrefixLength);
541+
if (isLongestMatch) {
542+
// If this is a higher priority match than anything we've seen so far, previous results from matches are invalid, e.g.
543+
// for `import {} from "some-package/|"` with a typesVersions:
544+
// {
545+
// "bar/*": ["bar/*"], // <-- 1. We add 'bar', but 'bar/*' doesn't match yet.
546+
// "*": ["dist/*"], // <-- 2. We match here and add files from dist. 'bar' is still ok because it didn't come from a match.
547+
// "foo/*": ["foo/*"] // <-- 3. We matched '*' earlier and added results from dist, but if 'foo/*' also matched,
548+
// } results in dist would not be visible. 'bar' still stands because it didn't come from a match.
549+
// This is especially important if `dist/foo` is a folder, because if we fail to clear results
550+
// added by the '*' match, after typing `"some-package/foo/|"` we would get file results from both
551+
// ./dist/foo and ./foo, when only the latter will actually be resolvable.
552+
// See pathCompletionsTypesVersionsWildcard6.ts.
553+
matchedPathPrefixLength = pathPattern.prefix.length;
554+
pathResults = pathResults.filter(r => !r.matchedPattern);
555+
}
556+
if (typeof pathPattern === "string" || matchedPathPrefixLength === undefined || pathPattern.prefix.length >= matchedPathPrefixLength) {
557+
pathResults.push({
558+
matchedPattern: isMatch,
559+
results: getCompletionsForPathMapping(path, patterns, fragment, baseDirectory, fileExtensions, host)
560+
.map(({ name, kind, extension }) => nameAndKind(name, kind, extension)),
561+
});
536562
}
537563
}
538564
}
565+
566+
const equatePaths = host.useCaseSensitiveFileNames?.() ? equateStringsCaseSensitive : equateStringsCaseInsensitive;
567+
const equateResults: EqualityComparer<NameAndKind> = (a, b) => equatePaths(a.name, b.name);
568+
pathResults.forEach(pathResult => pathResult.results.forEach(pathResult => pushIfUnique(result, pathResult, equateResults)));
569+
570+
return matchedPathPrefixLength > -1;
539571
}
540572

541573
/**
@@ -659,11 +691,15 @@ namespace ts.Completions.StringCompletions {
659691

660692
const pathPrefix = path.slice(0, path.length - 1);
661693
const remainingFragment = tryRemovePrefix(fragment, pathPrefix);
662-
return remainingFragment === undefined ? justPathMappingName(pathPrefix) : flatMap(patterns, pattern =>
663-
getModulesForPathsPattern(remainingFragment, baseUrl, pattern, fileExtensions, host));
694+
if (remainingFragment === undefined) {
695+
const starIsFullPathComponent = path[path.length - 2] === "/";
696+
return starIsFullPathComponent ? justPathMappingName(pathPrefix) : flatMap(patterns, pattern =>
697+
getModulesForPathsPattern("", baseUrl, pattern, fileExtensions, host)?.map(({ name, ...rest }) => ({ name: pathPrefix + name, ...rest })));
698+
}
699+
return flatMap(patterns, pattern => getModulesForPathsPattern(remainingFragment, baseUrl, pattern, fileExtensions, host));
664700

665701
function justPathMappingName(name: string): readonly NameAndKind[] {
666-
return startsWith(name, fragment) ? [directoryResult(name)] : emptyArray;
702+
return startsWith(name, fragment) ? [directoryResult(removeTrailingDirectorySeparator(name))] : emptyArray;
667703
}
668704
}
669705

‎tests/cases/fourslash/completionForStringLiteralNonrelativeImport13.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,6 @@
3131

3232
verify.completions({
3333
marker: test.markerNames(),
34-
exact: ["aaa", "index", "ts3.1", "zzz"],
34+
exact: ["index", "zzz"],
3535
isNewIdentifierLocation: true,
3636
});

‎tests/cases/fourslash/completionsPaths_pathMapping_topLevel.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,6 @@
1515

1616
verify.completions({
1717
marker: "",
18-
exact: ["src", "foo/"].map(name => ({ name, kind: "directory" })),
18+
exact: ["src", "foo"].map(name => ({ name, kind: "directory" })),
1919
isNewIdentifierLocation: true,
2020
});

‎tests/cases/fourslash/pathCompletionsTypesVersionsWildcard1.ts

+42
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
/// <reference path="fourslash.ts" />
2+
3+
// @module: commonjs
4+
5+
// @Filename: /node_modules/foo/package.json
6+
//// {
7+
//// "types": "index.d.ts",
8+
//// "typesVersions": {
9+
//// "*": {
10+
//// "*": ["dist/*"]
11+
//// }
12+
//// }
13+
//// }
14+
15+
// @Filename: /node_modules/foo/nope.d.ts
16+
//// export const nope = 0;
17+
18+
// @Filename: /node_modules/foo/dist/index.d.ts
19+
//// export const index = 0;
20+
21+
// @Filename: /node_modules/foo/dist/blah.d.ts
22+
//// export const blah = 0;
23+
24+
// @Filename: /node_modules/foo/dist/subfolder/one.d.ts
25+
//// export const one = 0;
26+
27+
// @Filename: /a.ts
28+
//// import { } from "foo//**/";
29+
30+
verify.completions({
31+
marker: "",
32+
isNewIdentifierLocation: true,
33+
exact: ["blah", "index", "subfolder"],
34+
});
35+
36+
edit.insert("subfolder/");
37+
38+
verify.completions({
39+
marker: "",
40+
isNewIdentifierLocation: true,
41+
exact: ["one"],
42+
});

‎tests/cases/fourslash/pathCompletionsTypesVersionsWildcard2.ts

+34
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
/// <reference path="fourslash.ts" />
2+
3+
// @module: commonjs
4+
5+
// @Filename: /node_modules/foo/package.json
6+
//// {
7+
//// "types": "index.d.ts",
8+
//// "typesVersions": {
9+
//// "<=3.4.1": {
10+
//// "*": ["ts-old/*"]
11+
//// }
12+
//// }
13+
//// }
14+
15+
// @Filename: /node_modules/foo/nope.d.ts
16+
//// export const nope = 0;
17+
18+
// @Filename: /node_modules/foo/ts-old/index.d.ts
19+
//// export const index = 0;
20+
21+
// @Filename: /node_modules/foo/ts-old/blah.d.ts
22+
//// export const blah = 0;
23+
24+
// @Filename: /node_modules/foo/ts-old/subfolder/one.d.ts
25+
//// export const one = 0;
26+
27+
// @Filename: /a.ts
28+
//// import { } from "foo//**/";
29+
30+
verify.completions({
31+
marker: "",
32+
isNewIdentifierLocation: true,
33+
exact: ["nope", "ts-old"],
34+
});

‎tests/cases/fourslash/pathCompletionsTypesVersionsWildcard3.ts

+48
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
/// <reference path="fourslash.ts" />
2+
3+
// @module: commonjs
4+
5+
// @Filename: /node_modules/foo/package.json
6+
//// {
7+
//// "types": "index.d.ts",
8+
//// "typesVersions": {
9+
//// ">=4.3.5": {
10+
//// "browser/*": ["dist/*"]
11+
//// }
12+
//// }
13+
//// }
14+
15+
// @Filename: /node_modules/foo/nope.d.ts
16+
//// export const nope = 0;
17+
18+
// @Filename: /node_modules/foo/dist/index.d.ts
19+
//// export const index = 0;
20+
21+
// @Filename: /node_modules/foo/dist/blah.d.ts
22+
//// export const blah = 0;
23+
24+
// @Filename: /node_modules/foo/dist/subfolder/one.d.ts
25+
//// export const one = 0;
26+
27+
// @Filename: /a.ts
28+
//// import { } from "foo//**/";
29+
30+
verify.completions({
31+
marker: "",
32+
isNewIdentifierLocation: true,
33+
exact: ["browser", "nope", "dist"],
34+
});
35+
36+
edit.insert("browser/");
37+
38+
verify.completions({
39+
isNewIdentifierLocation: true,
40+
exact: ["blah", "index", "subfolder"],
41+
});
42+
43+
edit.insert("subfolder/");
44+
45+
verify.completions({
46+
isNewIdentifierLocation: true,
47+
exact: ["one"],
48+
});

‎tests/cases/fourslash/pathCompletionsTypesVersionsWildcard4.ts

+41
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
/// <reference path="fourslash.ts" />
2+
3+
// @module: commonjs
4+
5+
// @Filename: /node_modules/foo/package.json
6+
//// {
7+
//// "types": "index.d.ts",
8+
//// "typesVersions": {
9+
//// ">=4.3.5": {
10+
//// "component-*": ["cjs/components/*"]
11+
//// }
12+
//// }
13+
//// }
14+
15+
// @Filename: /node_modules/foo/nope.d.ts
16+
//// export const nope = 0;
17+
18+
// @Filename: /node_modules/foo/cjs/components/index.d.ts
19+
//// export const index = 0;
20+
21+
// @Filename: /node_modules/foo/cjs/components/blah.d.ts
22+
//// export const blah = 0;
23+
24+
// @Filename: /node_modules/foo/cjs/components/subfolder/one.d.ts
25+
//// export const one = 0;
26+
27+
// @Filename: /a.ts
28+
//// import { } from "foo//**/";
29+
30+
verify.completions({
31+
marker: "",
32+
isNewIdentifierLocation: true,
33+
exact: ["component-blah", "component-index", "component-subfolder", "nope", "cjs"],
34+
});
35+
36+
edit.insert("component-subfolder/");
37+
38+
verify.completions({
39+
isNewIdentifierLocation: true,
40+
exact: ["one"],
41+
});

‎tests/cases/fourslash/pathCompletionsTypesVersionsWildcard5.ts

+54
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
/// <reference path="fourslash.ts" />
2+
3+
// @module: commonjs
4+
5+
// @Filename: /node_modules/foo/package.json
6+
//// {
7+
//// "types": "index.d.ts",
8+
//// "typesVersions": {
9+
//// "*": {
10+
//// "*": ["dist/*"],
11+
//// "foo/*": ["dist/*"],
12+
//// "bar/*": ["dist/*"],
13+
//// "exact-match": ["dist/index.d.ts"]
14+
//// }
15+
//// }
16+
//// }
17+
18+
// @Filename: /node_modules/foo/nope.d.ts
19+
//// export const nope = 0;
20+
21+
// @Filename: /node_modules/foo/dist/index.d.ts
22+
//// export const index = 0;
23+
24+
// @Filename: /node_modules/foo/dist/blah.d.ts
25+
//// export const blah = 0;
26+
27+
// @Filename: /node_modules/foo/dist/foo/onlyInFooFolder.d.ts
28+
//// export const foo = 0;
29+
30+
// @Filename: /node_modules/foo/dist/subfolder/one.d.ts
31+
//// export const one = 0;
32+
33+
// @Filename: /a.ts
34+
//// import { } from "foo//**/";
35+
36+
verify.completions({
37+
marker: "",
38+
isNewIdentifierLocation: true,
39+
exact: ["blah", "index", "foo", "subfolder", "bar", "exact-match"],
40+
});
41+
42+
edit.insert("foo/");
43+
44+
verify.completions({
45+
isNewIdentifierLocation: true,
46+
exact: ["blah", "index", "foo", "subfolder"],
47+
});
48+
49+
edit.insert("foo/");
50+
51+
verify.completions({
52+
isNewIdentifierLocation: true,
53+
exact: ["onlyInFooFolder"],
54+
});

0 commit comments

Comments
 (0)