Skip to content

Commit 15d294d

Browse files
minestarksminestarks
committed
Bugs in missing import codefix
- We didn't locate the package.json correctly in cases where the module to be imported is in a subdirectory of the package - We didn't look at the types element in package.json (just typings) - We didn't remove /index.js from the path if the main module was in a subdirectory Fixes microsoft#16963
1 parent abb229e commit 15d294d

4 files changed

Lines changed: 118 additions & 25 deletions

File tree

‎src/services/codefixes/importFixes.ts‎

Lines changed: 43 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -504,42 +504,60 @@ namespace ts.codefix {
504504
return undefined;
505505
}
506506

507-
const indexOfNodeModules = moduleFileName.indexOf("node_modules");
508-
if (indexOfNodeModules < 0) {
507+
const indexOfTopNodeModules = moduleFileName.indexOf("node_modules");
508+
if (indexOfTopNodeModules < 0) {
509509
return undefined;
510510
}
511511

512-
let relativeFileName: string;
513-
if (sourceDirectory.indexOf(moduleFileName.substring(0, indexOfNodeModules - 1)) === 0) {
514-
// if node_modules folder is in this folder or any of its parent folder, no need to keep it.
515-
relativeFileName = moduleFileName.substring(indexOfNodeModules + 13 /* "node_modules\".length */);
516-
}
517-
else {
518-
relativeFileName = getRelativePath(moduleFileName, sourceDirectory);
519-
}
520-
521-
relativeFileName = removeFileExtension(relativeFileName);
522-
if (endsWith(relativeFileName, "/index")) {
523-
relativeFileName = getDirectoryPath(relativeFileName);
524-
}
525-
else {
526-
try {
527-
const moduleDirectory = getDirectoryPath(moduleFileName);
528-
const packageJsonContent = JSON.parse(context.host.readFile(combinePaths(moduleDirectory, "package.json")));
512+
// Simplify the full file path to something that can be resolved by Node.
513+
// First remove the extension
514+
let moduleSpecifier = removeFileExtension(moduleFileName);
515+
// If the module could be imported by a directory name, use that directory's name
516+
moduleSpecifier = getDirectoryOrFileName(moduleSpecifier);
517+
// Get a path that's relative to node_modules or the importing file's path
518+
moduleSpecifier = getNodeResolvablePath(moduleSpecifier);
519+
// If the module was found in @types, get the actual node package name
520+
return getPackageNameFromAtTypesDirectory(moduleSpecifier);
521+
522+
function getDirectoryOrFileName(fullModulePathWithoutExtension: string): string {
523+
// If the file is the main module, it can be imported by the package name
524+
const indexOfLastNodeModules = moduleFileName.lastIndexOf("node_modules");
525+
const indexOfSlashAtPackageRoot = moduleFileName.indexOf("/", indexOfLastNodeModules + 13 /* "node_modules\".length */);
526+
const packageRootPath = moduleFileName.substring(0, indexOfSlashAtPackageRoot);
527+
const packageJsonPath = combinePaths(packageRootPath, "package.json");
528+
if (context.host.fileExists(packageJsonPath)) {
529+
const packageJsonContent = JSON.parse(context.host.readFile(packageJsonPath));
529530
if (packageJsonContent) {
530-
const mainFile = packageJsonContent.main || packageJsonContent.typings;
531-
if (mainFile) {
532-
const mainExportFile = toPath(mainFile, moduleDirectory, getCanonicalFileName);
531+
const mainFileRelative = packageJsonContent.typings || packageJsonContent.types || packageJsonContent.main;
532+
if (mainFileRelative) {
533+
const mainExportFile = toPath(mainFileRelative, packageRootPath, getCanonicalFileName);
533534
if (removeFileExtension(mainExportFile) === removeFileExtension(moduleFileName)) {
534-
relativeFileName = getDirectoryPath(relativeFileName);
535+
return packageRootPath;
535536
}
536537
}
537538
}
538539
}
539-
catch (e) { }
540+
541+
// If the file is index.js, it can be imported by its directory name
542+
if (endsWith(fullModulePathWithoutExtension, "/index")) {
543+
return getDirectoryPath(fullModulePathWithoutExtension);
544+
}
545+
546+
return fullModulePathWithoutExtension;
540547
}
541548

542-
return getPackageNameFromAtTypesDirectory(relativeFileName);
549+
function getNodeResolvablePath(path: string): string {
550+
const fullPathUptoNodeModules = moduleFileName.substring(0, indexOfTopNodeModules - 1);
551+
if (sourceDirectory.indexOf(fullPathUptoNodeModules) === 0) {
552+
const indexOfTopPackageName = indexOfTopNodeModules + 13 /* "node_modules\".length */;
553+
// if node_modules folder is in this folder or any of its parent folders, no need to keep it.
554+
const relativeToTopNodeModules = path.substring(indexOfTopPackageName);
555+
return relativeToTopNodeModules;
556+
}
557+
else {
558+
return getRelativePath(path, sourceDirectory);
559+
}
560+
}
543561
}
544562
}
545563

‎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+
]);

0 commit comments

Comments
 (0)