Skip to content

Commit 441daa4

Browse files
minestarksminestarks
authored
Merge pull request microsoft#17302 from minestarks/removeimportfix
Bugs in missing import codefix
2 parents f0bd91c + 9f6ec63 commit 441daa4

6 files changed

Lines changed: 199 additions & 27 deletions

File tree

‎src/harness/fourslash.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2405,7 +2405,7 @@ namespace FourSlash {
24052405
const sortedActualArray = actualTextArray.sort();
24062406
if (!ts.arrayIsEqualTo(sortedExpectedArray, sortedActualArray)) {
24072407
this.raiseError(
2408-
`Actual text array doesn't match expected text array. \nActual: \n"${sortedActualArray.join("\n\n")}"\n---\nExpected: \n'${sortedExpectedArray.join("\n\n")}'`);
2408+
`Actual text array doesn't match expected text array. \nActual: \n'${sortedActualArray.join("\n\n")}'\n---\nExpected: \n'${sortedExpectedArray.join("\n\n")}'`);
24092409
}
24102410
}
24112411

‎src/services/codefixes/importFixes.ts‎

Lines changed: 94 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -501,45 +501,113 @@ namespace ts.codefix {
501501
return undefined;
502502
}
503503

504-
const indexOfNodeModules = moduleFileName.indexOf("node_modules");
505-
if (indexOfNodeModules < 0) {
506-
return undefined;
507-
}
504+
const parts = getNodeModulePathParts(moduleFileName);
508505

509-
let relativeFileName: string;
510-
if (sourceDirectory.indexOf(moduleFileName.substring(0, indexOfNodeModules - 1)) === 0) {
511-
// if node_modules folder is in this folder or any of its parent folder, no need to keep it.
512-
relativeFileName = moduleFileName.substring(indexOfNodeModules + 13 /* "node_modules\".length */);
513-
}
514-
else {
515-
relativeFileName = getRelativePath(moduleFileName, sourceDirectory);
506+
if (!parts) {
507+
return undefined;
516508
}
517509

518-
relativeFileName = removeFileExtension(relativeFileName);
519-
if (endsWith(relativeFileName, "/index")) {
520-
relativeFileName = getDirectoryPath(relativeFileName);
521-
}
522-
else {
523-
try {
524-
const moduleDirectory = getDirectoryPath(moduleFileName);
525-
const packageJsonContent = JSON.parse(context.host.readFile(combinePaths(moduleDirectory, "package.json")));
510+
// Simplify the full file path to something that can be resolved by Node.
511+
512+
// If the module could be imported by a directory name, use that directory's name
513+
let moduleSpecifier = getDirectoryOrExtensionlessFileName(moduleFileName);
514+
// Get a path that's relative to node_modules or the importing file's path
515+
moduleSpecifier = getNodeResolvablePath(moduleSpecifier);
516+
// If the module was found in @types, get the actual Node package name
517+
return getPackageNameFromAtTypesDirectory(moduleSpecifier);
518+
519+
function getDirectoryOrExtensionlessFileName(path: string): string {
520+
// If the file is the main module, it can be imported by the package name
521+
const packageRootPath = path.substring(0, parts.packageRootIndex);
522+
const packageJsonPath = combinePaths(packageRootPath, "package.json");
523+
if (context.host.fileExists(packageJsonPath)) {
524+
const packageJsonContent = JSON.parse(context.host.readFile(packageJsonPath));
526525
if (packageJsonContent) {
527-
const mainFile = packageJsonContent.main || packageJsonContent.typings;
528-
if (mainFile) {
529-
const mainExportFile = toPath(mainFile, moduleDirectory, getCanonicalFileName);
530-
if (removeFileExtension(mainExportFile) === removeFileExtension(moduleFileName)) {
531-
relativeFileName = getDirectoryPath(relativeFileName);
526+
const mainFileRelative = packageJsonContent.typings || packageJsonContent.types || packageJsonContent.main;
527+
if (mainFileRelative) {
528+
const mainExportFile = toPath(mainFileRelative, packageRootPath, getCanonicalFileName);
529+
if (mainExportFile === getCanonicalFileName(path)) {
530+
return packageRootPath;
532531
}
533532
}
534533
}
535534
}
536-
catch (e) { }
535+
536+
// We still have a file name - remove the extension
537+
const fullModulePathWithoutExtension = removeFileExtension(path);
538+
539+
// If the file is /index, it can be imported by its directory name
540+
if (getCanonicalFileName(fullModulePathWithoutExtension.substring(parts.fileNameIndex)) === "/index") {
541+
return fullModulePathWithoutExtension.substring(0, parts.fileNameIndex);
542+
}
543+
544+
return fullModulePathWithoutExtension;
537545
}
538546

539-
return getPackageNameFromAtTypesDirectory(relativeFileName);
547+
function getNodeResolvablePath(path: string): string {
548+
const basePath = path.substring(0, parts.topLevelNodeModulesIndex);
549+
if (sourceDirectory.indexOf(basePath) === 0) {
550+
// if node_modules folder is in this folder or any of its parent folders, no need to keep it.
551+
return path.substring(parts.topLevelPackageNameIndex + 1);
552+
}
553+
else {
554+
return getRelativePath(path, sourceDirectory);
555+
}
556+
}
540557
}
541558
}
542559

560+
function getNodeModulePathParts(fullPath: string) {
561+
// If fullPath can't be valid module file within node_modules, returns undefined.
562+
// Example of expected pattern: /base/path/node_modules/[otherpackage/node_modules/]package/[subdirectory/]file.js
563+
// Returns indices: ^ ^ ^ ^
564+
565+
let topLevelNodeModulesIndex = 0;
566+
let topLevelPackageNameIndex = 0;
567+
let packageRootIndex = 0;
568+
let fileNameIndex = 0;
569+
570+
const enum States {
571+
BeforeNodeModules,
572+
NodeModules,
573+
PackageContent
574+
}
575+
576+
let partStart = 0;
577+
let partEnd = 0;
578+
let state = States.BeforeNodeModules;
579+
580+
while (partEnd >= 0) {
581+
partStart = partEnd;
582+
partEnd = fullPath.indexOf("/", partStart + 1);
583+
switch (state) {
584+
case States.BeforeNodeModules:
585+
if (fullPath.indexOf("/node_modules/", partStart) === partStart) {
586+
topLevelNodeModulesIndex = partStart;
587+
topLevelPackageNameIndex = partEnd;
588+
state = States.NodeModules;
589+
}
590+
break;
591+
case States.NodeModules:
592+
packageRootIndex = partEnd;
593+
state = States.PackageContent;
594+
break;
595+
case States.PackageContent:
596+
if (fullPath.indexOf("/node_modules/", partStart) === partStart) {
597+
state = States.NodeModules;
598+
}
599+
else {
600+
state = States.PackageContent;
601+
}
602+
break;
603+
}
604+
}
605+
606+
fileNameIndex = partStart;
607+
608+
return state > States.NodeModules ? { topLevelNodeModulesIndex, topLevelPackageNameIndex, packageRootIndex, fileNameIndex } : undefined;
609+
}
610+
543611
function getPathRelativeToRootDirs(path: string, rootDirs: string[]) {
544612
for (const rootDir of rootDirs) {
545613
const relativeName = getRelativePathIfInDirectory(path, rootDir);

‎tests/cases/fourslash/importNameCodeFixNewImportNodeModules4.ts‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
/// <reference path="fourslash.ts" />
2+
3+
//// [|f1/*0*/('');|]
4+
5+
// @Filename: package.json
6+
//// { "dependencies": { "package-name": "latest" } }
7+
8+
// @Filename: node_modules/package-name/bin/lib/libfile.d.ts
9+
//// export function f1(text: string): string;
10+
11+
// @Filename: node_modules/package-name/bin/lib/libfile.js
12+
//// function f1(text) { }
13+
//// exports.f1 = f1;
14+
15+
// @Filename: node_modules/package-name/package.json
16+
//// {
17+
//// "main": "bin/lib/libfile.js",
18+
//// "types": "bin/lib/libfile.d.ts"
19+
//// }
20+
21+
verify.importFixAtPosition([
22+
`import { f1 } from "package-name";
23+
24+
f1('');`
25+
]);

‎tests/cases/fourslash/importNameCodeFixNewImportNodeModules5.ts‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
/// <reference path="fourslash.ts" />
2+
3+
//// [|f1/*0*/('');|]
4+
5+
// @Filename: package.json
6+
//// { "dependencies": { "package-name": "latest" } }
7+
8+
// @Filename: node_modules/package-name/node_modules/package-name2/bin/lib/libfile.d.ts
9+
//// export function f1(text: string): string;
10+
11+
// @Filename: node_modules/package-name/node_modules/package-name2/bin/lib/libfile.js
12+
//// function f1(text) { }
13+
//// exports.f1 = f1;
14+
15+
// @Filename: node_modules/package-name/node_modules/package-name2/package.json
16+
//// {
17+
//// "main": "bin/lib/libfile.js",
18+
//// "types": "bin/lib/libfile.d.ts"
19+
//// }
20+
21+
verify.importFixAtPosition([
22+
`import { f1 } from "package-name/node_modules/package-name2";
23+
24+
f1('');`
25+
]);

‎tests/cases/fourslash/importNameCodeFixNewImportNodeModules6.ts‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
/// <reference path="fourslash.ts" />
2+
3+
//// [|f1/*0*/('');|]
4+
5+
// @Filename: package.json
6+
//// { "dependencies": { "package-name": "latest" } }
7+
8+
// @Filename: node_modules/package-name/bin/lib/index.d.ts
9+
//// export function f1(text: string): string;
10+
11+
// @Filename: node_modules/package-name/bin/lib/index.js
12+
//// function f1(text) { }
13+
//// exports.f1 = f1;
14+
15+
// @Filename: node_modules/package-name/package.json
16+
//// {
17+
//// "main": "bin/lib/index.js",
18+
//// "types": "bin/lib/index.d.ts"
19+
//// }
20+
21+
verify.importFixAtPosition([
22+
`import { f1 } from "package-name";
23+
24+
f1('');`
25+
]);

‎tests/cases/fourslash/importNameCodeFixNewImportNodeModules7.ts‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
/// <reference path="fourslash.ts" />
2+
3+
//// [|f1/*0*/('');|]
4+
5+
// @Filename: package.json
6+
//// { "dependencies": { "package-name": "0.0.1" } }
7+
8+
// @Filename: node_modules/package-name/bin/lib/libfile.d.ts
9+
//// export declare function f1(text: string): string;
10+
11+
// @Filename: node_modules/package-name/bin/lib/libfile.js
12+
//// function f1(text) {}
13+
//// exports.f1 = f1;
14+
15+
// @Filename: node_modules/package-name/package.json
16+
//// { "main": "bin/lib/libfile.js" }
17+
18+
19+
// In this case, importing the module by its package name:
20+
// import { f1 } from 'package-name'
21+
// could in theory work, however the resulting code compiles with a module resolution error
22+
// since bin/lib/libfile.d.ts isn't declared under "typings" in package.json
23+
// Therefore just import the module by its qualified path
24+
25+
verify.importFixAtPosition([
26+
`import { f1 } from "package-name/bin/lib/libfile";
27+
28+
f1('');`
29+
]);

0 commit comments

Comments
 (0)