Have module resolution be independend of --allowJs and --jsx flags #11704
Conversation
…moduleNameResolver responsible for omitting files based on compiler options
| * - be a .d.ts file | ||
| * - use top level imports\exports | ||
| * - don't use tripleslash references | ||
| */ | ||
| isExternalLibraryImport?: boolean; | ||
| isExternalLibraryImport: boolean; |
There was a problem hiding this comment.
this should stay optional. otherwise we break other API
| * in which case they will be inferred from the file extension. | ||
| * Prefer to return a full ResolvedModule. | ||
| */ | ||
| export type ResolvedModuleFromHost = { resolvedFileName: string; isExternalLibraryImport: boolean } | ResolvedModule; |
There was a problem hiding this comment.
i do not understand why we need this. the two new properties are optional, and we have logic to handle them being missing. so do not see the value here.
| return undefined; | ||
| } | ||
| // `resolvedTsFileName` and `resolvedJsFileName` should be present as properties even if undefined. | ||
| else if ("resolvedTsFileName" in resolved) { |
There was a problem hiding this comment.
mm. do you really need that check? can we check when we create them instead?
| * Extracts the file name from a ResolvedModule, or returns a DiagnosticMessage if we are not set up to use that kind of file. | ||
| * The DiagnosticMessage's parameters are the imported module name, and the filename it resolved to. | ||
| */ | ||
| export function getResolutionOrDiagnostic(options: CompilerOptions, { resolvedTsFileName: ts, resolvedJsFileName: js }: ResolvedModule): string | { file: string, diag: DiagnosticMessage } { |
There was a problem hiding this comment.
nit. i would rather we just use module.resolvedTsFileName || module.resolvedJsFileName for the module, and have this function just to report errors. this avoids the complexity of checking if the return type is string or not.
|
@vladima can you take a look. |
… avoid using resolution.resolvedFileName if the diagnostic is defined.
…e` and `resolvedJsFileName` optional properties (but still automatically infer one of them to supply if the host supplied neither)
| /* @internal */ | ||
| export function trace(host: ModuleResolutionHost, message: DiagnosticMessage, ...args: any[]): void; | ||
| export function trace(host: ModuleResolutionHost, message: DiagnosticMessage): void { | ||
| function trace(host: ModuleResolutionHost, message: DiagnosticMessage, ...args: any[]): void; |
|
|
||
| /** LsHost uses a global cache of automatically-installed typings to help it resolve modules. */ | ||
| /* @internal */ | ||
| export function resolveModuleNameForLsHost(moduleName: string, containingFile: string, compilerOptions: CompilerOptions, host: ModuleResolutionHost, globalCache: string | undefined, projectName: string): ResolvedModuleWithFailedLookupLocations { |
There was a problem hiding this comment.
I don' think this function should be here given that this file is part of core compiler bits and LSHost is specific to server
…solvedFileName` and `extension`. Also, change search order to do all TS searching before searching for JS.
| resolveModuleNamesWorker = (moduleNames, containingFile) => host.resolveModuleNames(moduleNames, containingFile); | ||
| resolveModuleNamesWorker = (moduleNames, containingFile) => host.resolveModuleNames(moduleNames, containingFile).map(resolved => { | ||
| // An older host may have omitted extension, in which case we should infer it from the file extension of resolvedFileName. | ||
| if (!resolved || resolved.extension) { |
There was a problem hiding this comment.
Extenstion.TS is Zero so this will fail. probably want to check for undefined here.
| /* | ||
| * Denotes if 'resolvedFileName' is isExternalLibraryImport and thus should be proper external module: | ||
| /** Extension of resolvedFileName. This must match what's at the end of resolvedFileName. */ | ||
| extension: Extension; |
There was a problem hiding this comment.
If this is optional we will need fallback logic on every access. I thought it was easier to just perform the conversion when getting it from an external source. This would mean that older clients won't compile against the new compiler, but will still run.
We might choose to not have this property as it is techincally redundant, but it's neater to have this property and use switch statements than to use if (fileExtensionIs(resolved.resolvedFileName, ".ts")) { ... } else if (fileExtensionIs(resolved.resolvedFileName, ".tsx")) { ... } all over.
ghost
merged commit 
ghost
changed the title
…and don't have moduleNameResolver responsible for omitting files based on compiler options.
This PR has relatively little observable effect, but gives some better error messages if a module would have been resolved to a tsx file but you forgot the "--jsx" flag.
The API should be backwards compatible as the new
ResolvedModuledata structure still contains theresolvedFileNamefield, and we have a methodconvertResolvedModuleFromHostwhich adds the newresolvedTsFileNameandresolvedJsFileNameproperties if necessary.This also technically causes breaking change in the module resolution. Previously we would ignore ".tsx" files without "--jsx", but now we will resolve to that module and report an error in the checker.
Also, the secondary lookup in
resolvedTypeReferenceDirectivenow only looks for.d.tsfiles, as the documentation onResolvedTypeReferenceDirectiveseems to indicate that that's all we should be returning. I'm also wondering ifloadModuleFromNodeModulesWorkershould do the same when searching through@types, and same forloadModuleFromNodeModulesAtTypes.I will redo #11446 after this is in.
@vladima brought up the point that we might want to consider getting rid of shims. That won't be necessary for this PR, as I haven't changed the
resolvedModuleNamemethod ofCoreServicesShimObject. This method is public on the class, but the class isn't exported and is only used as an implementation of theCoreServicesShiminterface, which doesn't define that method. Pinging @mhegazy too.