From 15d294d3508918f0f42c977742ba31aaf576f96e Mon Sep 17 00:00:00 2001 From: Mine Starks Date: Wed, 19 Jul 2017 11:02:49 -0700 Subject: [PATCH 1/3] 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 #16963 --- src/services/codefixes/importFixes.ts | 66 ++++++++++++------- .../importNameCodeFixNewImportNodeModules4.ts | 25 +++++++ .../importNameCodeFixNewImportNodeModules5.ts | 25 +++++++ .../importNameCodeFixNewImportNodeModules6.ts | 25 +++++++ 4 files changed, 117 insertions(+), 24 deletions(-) create mode 100644 tests/cases/fourslash/importNameCodeFixNewImportNodeModules4.ts create mode 100644 tests/cases/fourslash/importNameCodeFixNewImportNodeModules5.ts create mode 100644 tests/cases/fourslash/importNameCodeFixNewImportNodeModules6.ts diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index 0fa0b72162b..4efa6019753 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -504,42 +504,60 @@ namespace ts.codefix { return undefined; } - const indexOfNodeModules = moduleFileName.indexOf("node_modules"); - if (indexOfNodeModules < 0) { + const indexOfTopNodeModules = moduleFileName.indexOf("node_modules"); + if (indexOfTopNodeModules < 0) { return undefined; } - let relativeFileName: string; - if (sourceDirectory.indexOf(moduleFileName.substring(0, indexOfNodeModules - 1)) === 0) { - // if node_modules folder is in this folder or any of its parent folder, no need to keep it. - relativeFileName = moduleFileName.substring(indexOfNodeModules + 13 /* "node_modules\".length */); - } - else { - relativeFileName = getRelativePath(moduleFileName, sourceDirectory); - } + // Simplify the full file path to something that can be resolved by Node. + // First remove the extension + let moduleSpecifier = removeFileExtension(moduleFileName); + // If the module could be imported by a directory name, use that directory's name + moduleSpecifier = getDirectoryOrFileName(moduleSpecifier); + // Get a path that's relative to node_modules or the importing file's path + moduleSpecifier = getNodeResolvablePath(moduleSpecifier); + // If the module was found in @types, get the actual node package name + return getPackageNameFromAtTypesDirectory(moduleSpecifier); - relativeFileName = removeFileExtension(relativeFileName); - if (endsWith(relativeFileName, "/index")) { - relativeFileName = getDirectoryPath(relativeFileName); - } - else { - try { - const moduleDirectory = getDirectoryPath(moduleFileName); - const packageJsonContent = JSON.parse(context.host.readFile(combinePaths(moduleDirectory, "package.json"))); + function getDirectoryOrFileName(fullModulePathWithoutExtension: string): string { + // If the file is the main module, it can be imported by the package name + const indexOfLastNodeModules = moduleFileName.lastIndexOf("node_modules"); + const indexOfSlashAtPackageRoot = moduleFileName.indexOf("/", indexOfLastNodeModules + 13 /* "node_modules\".length */); + const packageRootPath = moduleFileName.substring(0, indexOfSlashAtPackageRoot); + const packageJsonPath = combinePaths(packageRootPath, "package.json"); + if (context.host.fileExists(packageJsonPath)) { + const packageJsonContent = JSON.parse(context.host.readFile(packageJsonPath)); if (packageJsonContent) { - const mainFile = packageJsonContent.main || packageJsonContent.typings; - if (mainFile) { - const mainExportFile = toPath(mainFile, moduleDirectory, getCanonicalFileName); + const mainFileRelative = packageJsonContent.typings || packageJsonContent.types || packageJsonContent.main; + if (mainFileRelative) { + const mainExportFile = toPath(mainFileRelative, packageRootPath, getCanonicalFileName); if (removeFileExtension(mainExportFile) === removeFileExtension(moduleFileName)) { - relativeFileName = getDirectoryPath(relativeFileName); + return packageRootPath; } } } } - catch (e) { } + + // If the file is index.js, it can be imported by its directory name + if (endsWith(fullModulePathWithoutExtension, "/index")) { + return getDirectoryPath(fullModulePathWithoutExtension); + } + + return fullModulePathWithoutExtension; } - return getPackageNameFromAtTypesDirectory(relativeFileName); + function getNodeResolvablePath(path: string): string { + const fullPathUptoNodeModules = moduleFileName.substring(0, indexOfTopNodeModules - 1); + if (sourceDirectory.indexOf(fullPathUptoNodeModules) === 0) { + const indexOfTopPackageName = indexOfTopNodeModules + 13 /* "node_modules\".length */; + // if node_modules folder is in this folder or any of its parent folders, no need to keep it. + const relativeToTopNodeModules = path.substring(indexOfTopPackageName); + return relativeToTopNodeModules; + } + else { + return getRelativePath(path, sourceDirectory); + } + } } } diff --git a/tests/cases/fourslash/importNameCodeFixNewImportNodeModules4.ts b/tests/cases/fourslash/importNameCodeFixNewImportNodeModules4.ts new file mode 100644 index 00000000000..9969bdcf46a --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixNewImportNodeModules4.ts @@ -0,0 +1,25 @@ +/// + +//// [|f1/*0*/('');|] + +// @Filename: package.json +//// { "dependencies": { "package-name": "latest" } } + +// @Filename: node_modules/package-name/bin/lib/libfile.d.ts +//// export function f1(text: string): string; + +// @Filename: node_modules/package-name/bin/lib/libfile.js +//// function f1(text) { } +//// exports.f1 = f1; + +// @Filename: node_modules/package-name/package.json +//// { +//// "main": "bin/lib/libfile.js", +//// "types": "bin/lib/libfile.d.ts" +//// } + +verify.importFixAtPosition([ +`import { f1 } from "package-name"; + +f1('');` +]); diff --git a/tests/cases/fourslash/importNameCodeFixNewImportNodeModules5.ts b/tests/cases/fourslash/importNameCodeFixNewImportNodeModules5.ts new file mode 100644 index 00000000000..24100362994 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixNewImportNodeModules5.ts @@ -0,0 +1,25 @@ +/// + +//// [|f1/*0*/('');|] + +// @Filename: package.json +//// { "dependencies": { "package-name": "latest" } } + +// @Filename: node_modules/package-name/node_modules/package-name2/bin/lib/libfile.d.ts +//// export function f1(text: string): string; + +// @Filename: node_modules/package-name/node_modules/package-name2/bin/lib/libfile.js +//// function f1(text) { } +//// exports.f1 = f1; + +// @Filename: node_modules/package-name/node_modules/package-name2/package.json +//// { +//// "main": "bin/lib/libfile.js", +//// "types": "bin/lib/libfile.d.ts" +//// } + +verify.importFixAtPosition([ +`import { f1 } from "package-name/node_modules/package-name2"; + +f1('');` +]); diff --git a/tests/cases/fourslash/importNameCodeFixNewImportNodeModules6.ts b/tests/cases/fourslash/importNameCodeFixNewImportNodeModules6.ts new file mode 100644 index 00000000000..7a52e3f67fe --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixNewImportNodeModules6.ts @@ -0,0 +1,25 @@ +/// + +//// [|f1/*0*/('');|] + +// @Filename: package.json +//// { "dependencies": { "package-name": "latest" } } + +// @Filename: node_modules/package-name/bin/lib/index.d.ts +//// export function f1(text: string): string; + +// @Filename: node_modules/package-name/bin/lib/index.js +//// function f1(text) { } +//// exports.f1 = f1; + +// @Filename: node_modules/package-name/package.json +//// { +//// "main": "bin/lib/index.js", +//// "types": "bin/lib/index.d.ts" +//// } + +verify.importFixAtPosition([ +`import { f1 } from "package-name"; + +f1('');` +]); From 98b14e34cae02787cd78e5513061ef8b00e9efbd Mon Sep 17 00:00:00 2001 From: Mine Starks Date: Thu, 20 Jul 2017 15:10:29 -0700 Subject: [PATCH 2/3] Fix quote styles to match --- src/harness/fourslash.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index e5dc3b0023a..4d1e5d40f1e 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -2399,7 +2399,7 @@ namespace FourSlash { const sortedActualArray = actualTextArray.sort(); if (!ts.arrayIsEqualTo(sortedExpectedArray, sortedActualArray)) { this.raiseError( - `Actual text array doesn't match expected text array. \nActual: \n"${sortedActualArray.join("\n\n")}"\n---\nExpected: \n'${sortedExpectedArray.join("\n\n")}'`); + `Actual text array doesn't match expected text array. \nActual: \n'${sortedActualArray.join("\n\n")}'\n---\nExpected: \n'${sortedExpectedArray.join("\n\n")}'`); } } From 9f6ec635a4de2152b507cd54f0d68698b86f1ece Mon Sep 17 00:00:00 2001 From: Mine Starks Date: Thu, 20 Jul 2017 15:10:59 -0700 Subject: [PATCH 3/3] Cleaner path splitting, refine file extension and case sensitivity handling --- src/services/codefixes/importFixes.ts | 88 +++++++++++++++---- .../importNameCodeFixNewImportNodeModules7.ts | 29 ++++++ 2 files changed, 98 insertions(+), 19 deletions(-) create mode 100644 tests/cases/fourslash/importNameCodeFixNewImportNodeModules7.ts diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index 4efa6019753..142107347dd 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -504,26 +504,24 @@ namespace ts.codefix { return undefined; } - const indexOfTopNodeModules = moduleFileName.indexOf("node_modules"); - if (indexOfTopNodeModules < 0) { + const parts = getNodeModulePathParts(moduleFileName); + + if (!parts) { return undefined; } // Simplify the full file path to something that can be resolved by Node. - // First remove the extension - let moduleSpecifier = removeFileExtension(moduleFileName); + // If the module could be imported by a directory name, use that directory's name - moduleSpecifier = getDirectoryOrFileName(moduleSpecifier); + let moduleSpecifier = getDirectoryOrExtensionlessFileName(moduleFileName); // Get a path that's relative to node_modules or the importing file's path moduleSpecifier = getNodeResolvablePath(moduleSpecifier); - // If the module was found in @types, get the actual node package name + // If the module was found in @types, get the actual Node package name return getPackageNameFromAtTypesDirectory(moduleSpecifier); - function getDirectoryOrFileName(fullModulePathWithoutExtension: string): string { + function getDirectoryOrExtensionlessFileName(path: string): string { // If the file is the main module, it can be imported by the package name - const indexOfLastNodeModules = moduleFileName.lastIndexOf("node_modules"); - const indexOfSlashAtPackageRoot = moduleFileName.indexOf("/", indexOfLastNodeModules + 13 /* "node_modules\".length */); - const packageRootPath = moduleFileName.substring(0, indexOfSlashAtPackageRoot); + const packageRootPath = path.substring(0, parts.packageRootIndex); const packageJsonPath = combinePaths(packageRootPath, "package.json"); if (context.host.fileExists(packageJsonPath)) { const packageJsonContent = JSON.parse(context.host.readFile(packageJsonPath)); @@ -531,28 +529,29 @@ namespace ts.codefix { const mainFileRelative = packageJsonContent.typings || packageJsonContent.types || packageJsonContent.main; if (mainFileRelative) { const mainExportFile = toPath(mainFileRelative, packageRootPath, getCanonicalFileName); - if (removeFileExtension(mainExportFile) === removeFileExtension(moduleFileName)) { + if (mainExportFile === getCanonicalFileName(path)) { return packageRootPath; } } } } - // If the file is index.js, it can be imported by its directory name - if (endsWith(fullModulePathWithoutExtension, "/index")) { - return getDirectoryPath(fullModulePathWithoutExtension); + // We still have a file name - remove the extension + const fullModulePathWithoutExtension = removeFileExtension(path); + + // If the file is /index, it can be imported by its directory name + if (getCanonicalFileName(fullModulePathWithoutExtension.substring(parts.fileNameIndex)) === "/index") { + return fullModulePathWithoutExtension.substring(0, parts.fileNameIndex); } return fullModulePathWithoutExtension; } function getNodeResolvablePath(path: string): string { - const fullPathUptoNodeModules = moduleFileName.substring(0, indexOfTopNodeModules - 1); - if (sourceDirectory.indexOf(fullPathUptoNodeModules) === 0) { - const indexOfTopPackageName = indexOfTopNodeModules + 13 /* "node_modules\".length */; + const basePath = path.substring(0, parts.topLevelNodeModulesIndex); + if (sourceDirectory.indexOf(basePath) === 0) { // if node_modules folder is in this folder or any of its parent folders, no need to keep it. - const relativeToTopNodeModules = path.substring(indexOfTopPackageName); - return relativeToTopNodeModules; + return path.substring(parts.topLevelPackageNameIndex + 1); } else { return getRelativePath(path, sourceDirectory); @@ -561,6 +560,57 @@ namespace ts.codefix { } } + function getNodeModulePathParts(fullPath: string) { + // If fullPath can't be valid module file within node_modules, returns undefined. + // Example of expected pattern: /base/path/node_modules/[otherpackage/node_modules/]package/[subdirectory/]file.js + // Returns indices: ^ ^ ^ ^ + + let topLevelNodeModulesIndex = 0; + let topLevelPackageNameIndex = 0; + let packageRootIndex = 0; + let fileNameIndex = 0; + + const enum States { + BeforeNodeModules, + NodeModules, + PackageContent + } + + let partStart = 0; + let partEnd = 0; + let state = States.BeforeNodeModules; + + while (partEnd >= 0) { + partStart = partEnd; + partEnd = fullPath.indexOf("/", partStart + 1); + switch (state) { + case States.BeforeNodeModules: + if (fullPath.indexOf("/node_modules/", partStart) === partStart) { + topLevelNodeModulesIndex = partStart; + topLevelPackageNameIndex = partEnd; + state = States.NodeModules; + } + break; + case States.NodeModules: + packageRootIndex = partEnd; + state = States.PackageContent; + break; + case States.PackageContent: + if (fullPath.indexOf("/node_modules/", partStart) === partStart) { + state = States.NodeModules; + } + else { + state = States.PackageContent; + } + break; + } + } + + fileNameIndex = partStart; + + return state > States.NodeModules ? { topLevelNodeModulesIndex, topLevelPackageNameIndex, packageRootIndex, fileNameIndex } : undefined; + } + function getPathRelativeToRootDirs(path: string, rootDirs: string[]) { for (const rootDir of rootDirs) { const relativeName = getRelativePathIfInDirectory(path, rootDir); diff --git a/tests/cases/fourslash/importNameCodeFixNewImportNodeModules7.ts b/tests/cases/fourslash/importNameCodeFixNewImportNodeModules7.ts new file mode 100644 index 00000000000..9032018a7f5 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixNewImportNodeModules7.ts @@ -0,0 +1,29 @@ +/// + +//// [|f1/*0*/('');|] + +// @Filename: package.json +//// { "dependencies": { "package-name": "0.0.1" } } + +// @Filename: node_modules/package-name/bin/lib/libfile.d.ts +//// export declare function f1(text: string): string; + +// @Filename: node_modules/package-name/bin/lib/libfile.js +//// function f1(text) {} +//// exports.f1 = f1; + +// @Filename: node_modules/package-name/package.json +//// { "main": "bin/lib/libfile.js" } + + +// In this case, importing the module by its package name: +// import { f1 } from 'package-name' +// could in theory work, however the resulting code compiles with a module resolution error +// since bin/lib/libfile.d.ts isn't declared under "typings" in package.json +// Therefore just import the module by its qualified path + +verify.importFixAtPosition([ +`import { f1 } from "package-name/bin/lib/libfile"; + +f1('');` +]); \ No newline at end of file