getEditsForFileRename: Avoid changing import specifier ending #26177
Conversation
ghost
force-pushed
the
getEditsForFileRename_preservePathEnding
branch
2 times, most recently
from
22f77c9 to
d40df30
Compare
| @@ -1,22 +0,0 @@ | |||
| /// <reference path="fourslash.ts" /> | |||
There was a problem hiding this comment.
typeRoots doesn't affect module resolution. It affects what directories are searched for automatically adding global definitions to the project. So it doesn't make sense to synthesize a specifier based on typeRoots because that will then be a compile error. paths should be used instead (which we already have a test for in importNameCodeFix_fromPathMapping.ts).
(Ref: microsoft/TypeScript-Handbook#692)
There was a problem hiding this comment.
I dont agree with this, many times the typeRoots will add ambient module definitions to program and getting import module fix would be good. Adding @DanielRosenwasser please comment on this. (look at vscode code i believe it adds typeRoots to add additional typings)
In anycase instead of deleting this test case (where we are using typeroots, add negative test to help catch any future regressions)
There was a problem hiding this comment.
If there is an ambient module definition we should already be handling that without typeRoots. See importNameCodeFixNewImportAmbient0.ts.
| return removeFileExtension(relativePath); | ||
| } | ||
|
|
||
| function tryGetModuleNameFromTypeRoots( |
There was a problem hiding this comment.
See below comment about typeRoots. node_modules/@types is always used for module resolution even if it's not in typeRoots; that's now handled in tryGetModuleNameAsNodeModule.
ghost
force-pushed
the
getEditsForFileRename_preservePathEnding
branch
from
d40df30 to
502fdbe
Compare
src/compiler/moduleSpecifiers.ts
Outdated
| return { | ||
| relativePreference: isExternalModuleNameRelative(oldImportSpecifier) ? RelativePreference.Relative : RelativePreference.NonRelative, | ||
| ending: fileExtensionIs(oldImportSpecifier, Extension.Js) ? Ending.JsExtension | ||
| : getEmitModuleResolutionKind(compilerOptions) !== ModuleResolutionKind.NodeJs || endsWith(oldImportSpecifier, "index") || endsWith(oldImportSpecifier, "index.js") ? Ending.Index : Ending.Minimal, |
There was a problem hiding this comment.
Shpuldnt this case be always false here since you already checked for extendsion to be js
endsWith(oldImportSpecifier, "index.js")
src/compiler/moduleSpecifiers.ts
Outdated
| function getPreferencesForUpdate(_: ModuleSpecifierPreferences, compilerOptions: CompilerOptions, oldImportSpecifier: string): Preferences { | ||
| return { | ||
| relativePreference: isExternalModuleNameRelative(oldImportSpecifier) ? RelativePreference.Relative : RelativePreference.NonRelative, | ||
| ending: fileExtensionIs(oldImportSpecifier, Extension.Js) ? Ending.JsExtension |
There was a problem hiding this comment.
Need to handle js and jsx both since that's what tryResolve(Extensions.Javascript) does.
src/compiler/moduleSpecifiers.ts
Outdated
| function getPreferences({ importModuleSpecifierPreference }: ModuleSpecifierPreferences, compilerOptions: CompilerOptions, importingSourceFile: SourceFile): Preferences { | ||
| return { | ||
| relativePreference: importModuleSpecifierPreference === "relative" ? RelativePreference.Relative : importModuleSpecifierPreference === "non-relative" ? RelativePreference.NonRelative : RelativePreference.Auto, | ||
| ending: usesJsExtensionOnImports(importingSourceFile) ? Ending.JsExtension |
There was a problem hiding this comment.
I haven't looked further to see use but don't you need to handle Json extension as well?
| @@ -1,22 +0,0 @@ | |||
| /// <reference path="fourslash.ts" /> | |||
There was a problem hiding this comment.
I dont agree with this, many times the typeRoots will add ambient module definitions to program and getting import module fix would be good. Adding @DanielRosenwasser please comment on this. (look at vscode code i believe it adds typeRoots to add additional typings)
In anycase instead of deleting this test case (where we are using typeroots, add negative test to help catch any future regressions)
|
@sheetalkamat Please re-review |
1 participant
Fixes #26028