Skip to content

Commit 4d29859

Browse files
andrewbranchandrewbranch
authored
Fix duplicate completions from two different copies of a node_modules package (microsoft#47584)
* Fix duplicate completions from two different copies of a node_modules package * Fix logic for scoped packages * Fix errors from merge * Less gross way to reconcile these two conflicting PRs
1 parent 61b7bbb commit 4d29859

8 files changed

+395
-75
lines changed

Diff for: ‎src/compiler/moduleSpecifiers.ts

-64
Original file line numberDiff line numberDiff line change
@@ -789,70 +789,6 @@ namespace ts.moduleSpecifiers {
789789
}
790790
}
791791

792-
interface NodeModulePathParts {
793-
readonly topLevelNodeModulesIndex: number;
794-
readonly topLevelPackageNameIndex: number;
795-
readonly packageRootIndex: number;
796-
readonly fileNameIndex: number;
797-
}
798-
function getNodeModulePathParts(fullPath: string): NodeModulePathParts | undefined {
799-
// If fullPath can't be valid module file within node_modules, returns undefined.
800-
// Example of expected pattern: /base/path/node_modules/[@scope/otherpackage/@otherscope/node_modules/]package/[subdirectory/]file.js
801-
// Returns indices: ^ ^ ^ ^
802-
803-
let topLevelNodeModulesIndex = 0;
804-
let topLevelPackageNameIndex = 0;
805-
let packageRootIndex = 0;
806-
let fileNameIndex = 0;
807-
808-
const enum States {
809-
BeforeNodeModules,
810-
NodeModules,
811-
Scope,
812-
PackageContent
813-
}
814-
815-
let partStart = 0;
816-
let partEnd = 0;
817-
let state = States.BeforeNodeModules;
818-
819-
while (partEnd >= 0) {
820-
partStart = partEnd;
821-
partEnd = fullPath.indexOf("/", partStart + 1);
822-
switch (state) {
823-
case States.BeforeNodeModules:
824-
if (fullPath.indexOf(nodeModulesPathPart, partStart) === partStart) {
825-
topLevelNodeModulesIndex = partStart;
826-
topLevelPackageNameIndex = partEnd;
827-
state = States.NodeModules;
828-
}
829-
break;
830-
case States.NodeModules:
831-
case States.Scope:
832-
if (state === States.NodeModules && fullPath.charAt(partStart + 1) === "@") {
833-
state = States.Scope;
834-
}
835-
else {
836-
packageRootIndex = partEnd;
837-
state = States.PackageContent;
838-
}
839-
break;
840-
case States.PackageContent:
841-
if (fullPath.indexOf(nodeModulesPathPart, partStart) === partStart) {
842-
state = States.NodeModules;
843-
}
844-
else {
845-
state = States.PackageContent;
846-
}
847-
break;
848-
}
849-
}
850-
851-
fileNameIndex = partStart;
852-
853-
return state > States.NodeModules ? { topLevelNodeModulesIndex, topLevelPackageNameIndex, packageRootIndex, fileNameIndex } : undefined;
854-
}
855-
856792
function getPathRelativeToRootDirs(path: string, rootDirs: readonly string[], getCanonicalFileName: GetCanonicalFileName): string | undefined {
857793
return firstDefined(rootDirs, rootDir => {
858794
const relativePath = getRelativePathIfInDirectory(path, rootDir, getCanonicalFileName);

Diff for: ‎src/compiler/utilities.ts

+64
Original file line numberDiff line numberDiff line change
@@ -7501,4 +7501,68 @@ namespace ts {
75017501
export function isThisTypeParameter(type: Type): boolean {
75027502
return !!(type.flags & TypeFlags.TypeParameter && (type as TypeParameter).isThisType);
75037503
}
7504+
7505+
export interface NodeModulePathParts {
7506+
readonly topLevelNodeModulesIndex: number;
7507+
readonly topLevelPackageNameIndex: number;
7508+
readonly packageRootIndex: number;
7509+
readonly fileNameIndex: number;
7510+
}
7511+
export function getNodeModulePathParts(fullPath: string): NodeModulePathParts | undefined {
7512+
// If fullPath can't be valid module file within node_modules, returns undefined.
7513+
// Example of expected pattern: /base/path/node_modules/[@scope/otherpackage/@otherscope/node_modules/]package/[subdirectory/]file.js
7514+
// Returns indices: ^ ^ ^ ^
7515+
7516+
let topLevelNodeModulesIndex = 0;
7517+
let topLevelPackageNameIndex = 0;
7518+
let packageRootIndex = 0;
7519+
let fileNameIndex = 0;
7520+
7521+
const enum States {
7522+
BeforeNodeModules,
7523+
NodeModules,
7524+
Scope,
7525+
PackageContent
7526+
}
7527+
7528+
let partStart = 0;
7529+
let partEnd = 0;
7530+
let state = States.BeforeNodeModules;
7531+
7532+
while (partEnd >= 0) {
7533+
partStart = partEnd;
7534+
partEnd = fullPath.indexOf("/", partStart + 1);
7535+
switch (state) {
7536+
case States.BeforeNodeModules:
7537+
if (fullPath.indexOf(nodeModulesPathPart, partStart) === partStart) {
7538+
topLevelNodeModulesIndex = partStart;
7539+
topLevelPackageNameIndex = partEnd;
7540+
state = States.NodeModules;
7541+
}
7542+
break;
7543+
case States.NodeModules:
7544+
case States.Scope:
7545+
if (state === States.NodeModules && fullPath.charAt(partStart + 1) === "@") {
7546+
state = States.Scope;
7547+
}
7548+
else {
7549+
packageRootIndex = partEnd;
7550+
state = States.PackageContent;
7551+
}
7552+
break;
7553+
case States.PackageContent:
7554+
if (fullPath.indexOf(nodeModulesPathPart, partStart) === partStart) {
7555+
state = States.NodeModules;
7556+
}
7557+
else {
7558+
state = States.PackageContent;
7559+
}
7560+
break;
7561+
}
7562+
}
7563+
7564+
fileNameIndex = partStart;
7565+
7566+
return state > States.NodeModules ? { topLevelNodeModulesIndex, topLevelPackageNameIndex, packageRootIndex, fileNameIndex } : undefined;
7567+
}
75047568
}

Diff for: ‎src/services/exportInfoMap.ts

+56-11
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ namespace ts {
3232
symbolTableKey: __String;
3333
moduleName: string;
3434
moduleFile: SourceFile | undefined;
35+
packageName: string | undefined;
3536

3637
// SymbolExportInfo, but optional symbols
3738
readonly symbol: Symbol | undefined;
@@ -63,6 +64,17 @@ namespace ts {
6364
let exportInfoId = 1;
6465
const exportInfo = createMultiMap<string, CachedSymbolExportInfo>();
6566
const symbols = new Map<number, [symbol: Symbol, moduleSymbol: Symbol]>();
67+
/**
68+
* Key: node_modules package name (no @types).
69+
* Value: path to deepest node_modules folder seen that is
70+
* both visible to `usableByFileName` and contains the package.
71+
*
72+
* Later, we can see if a given SymbolExportInfo is shadowed by
73+
* a another installation of the same package in a deeper
74+
* node_modules folder by seeing if its path starts with the
75+
* value stored here.
76+
*/
77+
const packages = new Map<string, string>();
6678
let usableByFileName: Path | undefined;
6779
const cache: ExportInfoMap = {
6880
isUsableByFile: importingFile => importingFile === usableByFileName,
@@ -77,6 +89,29 @@ namespace ts {
7789
cache.clear();
7890
usableByFileName = importingFile;
7991
}
92+
93+
let packageName;
94+
if (moduleFile) {
95+
const nodeModulesPathParts = getNodeModulePathParts(moduleFile.fileName);
96+
if (nodeModulesPathParts) {
97+
const { topLevelNodeModulesIndex, topLevelPackageNameIndex, packageRootIndex } = nodeModulesPathParts;
98+
packageName = unmangleScopedPackageName(getPackageNameFromTypesPackageName(moduleFile.fileName.substring(topLevelPackageNameIndex + 1, packageRootIndex)));
99+
if (startsWith(importingFile, moduleFile.path.substring(0, topLevelNodeModulesIndex))) {
100+
const prevDeepestNodeModulesPath = packages.get(packageName);
101+
const nodeModulesPath = moduleFile.fileName.substring(0, topLevelPackageNameIndex);
102+
if (prevDeepestNodeModulesPath) {
103+
const prevDeepestNodeModulesIndex = prevDeepestNodeModulesPath.indexOf(nodeModulesPathPart);
104+
if (topLevelNodeModulesIndex > prevDeepestNodeModulesIndex) {
105+
packages.set(packageName, nodeModulesPath);
106+
}
107+
}
108+
else {
109+
packages.set(packageName, nodeModulesPath);
110+
}
111+
}
112+
}
113+
}
114+
80115
const isDefault = exportKind === ExportKind.Default;
81116
const namedSymbol = isDefault && getLocalSymbolForExportDefault(symbol) || symbol;
82117
// 1. A named export must be imported by its key in `moduleSymbol.exports` or `moduleSymbol.members`.
@@ -104,6 +139,7 @@ namespace ts {
104139
moduleName,
105140
moduleFile,
106141
moduleFileName: moduleFile?.fileName,
142+
packageName,
107143
exportKind,
108144
targetFlags: target.flags,
109145
isFromPackageJson,
@@ -121,17 +157,20 @@ namespace ts {
121157
exportInfo.forEach((info, key) => {
122158
const { symbolName, ambientModuleName } = parseKey(key);
123159
const rehydrated = info.map(rehydrateCachedInfo);
124-
action(
125-
rehydrated,
126-
preferCapitalized => {
127-
const { symbol, exportKind } = rehydrated[0];
128-
const namedSymbol = exportKind === ExportKind.Default && getLocalSymbolForExportDefault(symbol) || symbol;
129-
return preferCapitalized
130-
? getNameForExportedSymbol(namedSymbol, /*scriptTarget*/ undefined, /*preferCapitalized*/ true)
131-
: symbolName;
132-
},
133-
!!ambientModuleName,
134-
key);
160+
const filtered = rehydrated.filter((r, i) => isNotShadowedByDeeperNodeModulesPackage(r, info[i].packageName));
161+
if (filtered.length) {
162+
action(
163+
filtered,
164+
preferCapitalized => {
165+
const { symbol, exportKind } = rehydrated[0];
166+
const namedSymbol = exportKind === ExportKind.Default && getLocalSymbolForExportDefault(symbol) || symbol;
167+
return preferCapitalized
168+
? getNameForExportedSymbol(namedSymbol, /*scriptTarget*/ undefined, /*preferCapitalized*/ true)
169+
: symbolName;
170+
},
171+
!!ambientModuleName,
172+
key);
173+
}
135174
});
136175
},
137176
releaseSymbols: () => {
@@ -232,6 +271,12 @@ namespace ts {
232271
}
233272
return true;
234273
}
274+
275+
function isNotShadowedByDeeperNodeModulesPackage(info: SymbolExportInfo, packageName: string | undefined) {
276+
if (!packageName || !info.moduleFileName) return true;
277+
const packageDeepestNodeModulesPath = packages.get(packageName);
278+
return !packageDeepestNodeModulesPath || startsWith(info.moduleFileName, packageDeepestNodeModulesPath);
279+
}
235280
}
236281

237282
export function isImportableFile(

Diff for: ‎tests/cases/fourslash/completionsImport_duplicatePackages_scoped.ts

+56
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
/// <reference path="fourslash.ts" />
2+
3+
// @module: commonjs
4+
// @esModuleInterop: true
5+
6+
// @Filename: /node_modules/@scope/react-dom/package.json
7+
//// { "name": "react-dom", "version": "1.0.0", "types": "./index.d.ts" }
8+
9+
// @Filename: /node_modules/@scope/react-dom/index.d.ts
10+
//// import * as React from "react";
11+
//// export function render(): void;
12+
13+
// @Filename: /node_modules/@scope/react/package.json
14+
//// { "name": "react", "version": "1.0.0", "types": "./index.d.ts" }
15+
16+
// @Filename: /node_modules/@scope/react/index.d.ts
17+
//// import "./other";
18+
//// export declare function useState(): void;
19+
20+
// @Filename: /node_modules/@scope/react/other.d.ts
21+
//// export declare function useRef(): void;
22+
23+
// @Filename: /packages/a/node_modules/@scope/react/package.json
24+
//// { "name": "react", "version": "1.0.1", "types": "./index.d.ts" }
25+
26+
// @Filename: /packages/a/node_modules/@scope/react/index.d.ts
27+
//// export declare function useState(): void;
28+
29+
// @Filename: /packages/a/index.ts
30+
//// import "@scope/react-dom";
31+
//// import "@scope/react";
32+
33+
// @Filename: /packages/a/foo.ts
34+
//// /**/
35+
36+
goTo.marker("");
37+
verify.completions({
38+
marker: "",
39+
exact: completion.globalsPlus([{
40+
name: "render",
41+
source: "@scope/react-dom",
42+
sourceDisplay: "@scope/react-dom",
43+
hasAction: true,
44+
sortText: completion.SortText.AutoImportSuggestions,
45+
}, {
46+
name: "useState",
47+
source: "@scope/react",
48+
sourceDisplay: "@scope/react",
49+
hasAction: true,
50+
sortText: completion.SortText.AutoImportSuggestions,
51+
}]),
52+
preferences: {
53+
includeCompletionsForModuleExports: true,
54+
allowIncompleteCompletions: true,
55+
},
56+
});

Diff for: ‎tests/cases/fourslash/completionsImport_duplicatePackages_scopedTypes.ts

+56
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
/// <reference path="fourslash.ts" />
2+
3+
// @module: commonjs
4+
// @esModuleInterop: true
5+
6+
// @Filename: /node_modules/@types/scope__react-dom/package.json
7+
//// { "name": "react-dom", "version": "1.0.0", "types": "./index.d.ts" }
8+
9+
// @Filename: /node_modules/@types/scope__react-dom/index.d.ts
10+
//// import * as React from "react";
11+
//// export function render(): void;
12+
13+
// @Filename: /node_modules/@types/scope__react/package.json
14+
//// { "name": "react", "version": "1.0.0", "types": "./index.d.ts" }
15+
16+
// @Filename: /node_modules/@types/scope__react/index.d.ts
17+
//// import "./other";
18+
//// export declare function useState(): void;
19+
20+
// @Filename: /node_modules/@types/scope__react/other.d.ts
21+
//// export declare function useRef(): void;
22+
23+
// @Filename: /packages/a/node_modules/@types/scope__react/package.json
24+
//// { "name": "react", "version": "1.0.1", "types": "./index.d.ts" }
25+
26+
// @Filename: /packages/a/node_modules/@types/scope__react/index.d.ts
27+
//// export declare function useState(): void;
28+
29+
// @Filename: /packages/a/index.ts
30+
//// import "@scope/react-dom";
31+
//// import "@scope/react";
32+
33+
// @Filename: /packages/a/foo.ts
34+
//// /**/
35+
36+
goTo.marker("");
37+
verify.completions({
38+
marker: "",
39+
exact: completion.globalsPlus([{
40+
name: "render",
41+
source: "@scope/react-dom",
42+
sourceDisplay: "@scope/react-dom",
43+
hasAction: true,
44+
sortText: completion.SortText.AutoImportSuggestions,
45+
}, {
46+
name: "useState",
47+
source: "@scope/react",
48+
sourceDisplay: "@scope/react",
49+
hasAction: true,
50+
sortText: completion.SortText.AutoImportSuggestions,
51+
}]),
52+
preferences: {
53+
includeCompletionsForModuleExports: true,
54+
allowIncompleteCompletions: true,
55+
},
56+
});

0 commit comments

Comments
 (0)