Bugs in missing import codefix#17302
Conversation
- 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
| function getNodeResolvablePath(path: string): string { | ||
| const fullPathUptoNodeModules = moduleFileName.substring(0, indexOfTopNodeModules - 1); | ||
| if (sourceDirectory.indexOf(fullPathUptoNodeModules) === 0) { | ||
| const indexOfTopPackageName = indexOfTopNodeModules + 13 /* "node_modules\".length */; |
There was a problem hiding this comment.
If I change it to a forward slash it may become clearer.
It's trying to say that the constant 13 corresponds to the length of node_modules/, i.e.
"node_modules/".length
There was a problem hiding this comment.
Oh, it's a path separator? Forward slash might be more consistent with the code.
| catch (e) { } | ||
|
|
||
| // If the file is index.js, it can be imported by its directory name | ||
| if (endsWith(fullModulePathWithoutExtension, "/index")) { |
There was a problem hiding this comment.
Has the path already been normalized? If not, do you need to check for backslash as well?
There was a problem hiding this comment.
I should check on this. Since /index was existing code, I relied on the assumption that it was normalized -- but wouldn't hurt to verify.
|
|
||
| const indexOfNodeModules = moduleFileName.indexOf("node_modules"); | ||
| if (indexOfNodeModules < 0) { | ||
| const indexOfTopNodeModules = moduleFileName.indexOf("node_modules"); |
There was a problem hiding this comment.
Seems like this could produce false positives (e.g. "node_modules2").
There was a problem hiding this comment.
Would implementing a path splitter simplify the logic in this function?
| const mainFileRelative = packageJsonContent.typings || packageJsonContent.types || packageJsonContent.main; | ||
| if (mainFileRelative) { | ||
| const mainExportFile = toPath(mainFileRelative, packageRootPath, getCanonicalFileName); | ||
| if (removeFileExtension(mainExportFile) === removeFileExtension(moduleFileName)) { |
There was a problem hiding this comment.
Does this mean they're allowed to have different file extensions? Also, does case matter?
There was a problem hiding this comment.
Also, use fullModulePathWithoutExtension?
There was a problem hiding this comment.
Alright, I had to come up with a really contrived case to check the answers to these. Found that a) case doesn't matter but file extensions do. Will update the code.
There was a problem hiding this comment.
Or rather, case sensitivity is based on the file system's case sensitivity
f9247bf to
ddfcd49
Compare
| // Example of expected pattern: /base/path/node_modules/[otherpackage/node_modules/]package/[subdirectory/]file.js | ||
| // Returns indices: ^ ^ ^ ^ | ||
|
|
||
| let nodeModulesIndex = 0; |
| let packageRootIndex = 0; | ||
| let fileNameIndex = 0; | ||
|
|
||
| enum States { |
| while (partEnd >= 0) { | ||
| partStart = partEnd; | ||
| partEnd = fullPath.indexOf("/", partStart + 1); | ||
| const part = fullPath.substring(partStart, partEnd); |
There was a problem hiding this comment.
If the goal of doing everything with indices was to avoid creating temporary strings, this should probably be created on demand (e.g. never in States.NodeModules) or, better still, "/node_modules" could be detected in-place.
ddfcd49 to
7b15392
Compare
7b15392 to
9f6ec63
Compare
package.jsoncorrectly in cases where the module to be imported is in a subdirectory of the packagetypeselement in package.json (justtypings)/index.jsfrom the path if the main module was in a subdirectoryFixes #16963