Follow paths in triple-slash types references
#48386
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
| @@ -354,7 +354,10 @@ namespace ts { | |||
| } | |||
| const conditions = features & NodeResolutionFeatures.Exports ? features & NodeResolutionFeatures.EsmMode ? ["node", "import", "types"] : ["node", "require", "types"] : []; | |||
| const moduleResolutionState: ModuleResolutionState = { compilerOptions: options, host, traceEnabled, failedLookupLocations, packageJsonInfoCache: cache, features, conditions }; | |||
| let resolved = primaryLookup(); | |||
| const loader: ResolutionKindSpecificLoader = (extensions, candidate, onlyRecordFailures, state) => nodeLoadModuleByRelativeName(extensions, candidate, onlyRecordFailures, state, /*considerPackageJson*/ true); | |||
| const optional = tryLoadModuleUsingOptionalResolutionSettings(Extensions.DtsOnly, typeReferenceDirectiveName, containingDirectory || "", loader, moduleResolutionState); | |||
There was a problem hiding this comment.
Well paths uses baseUrl.. And then tryLoadModuleUsingOptionalResolutionSettings has some baseUrl and rootDirs logic as well.. wouldnt we want typeRoots instead of baseUrl and probably rootDirs as well for type reference directive
There was a problem hiding this comment.
I don't think that's the case? Generally, the model we all have is that ///<reference types= resolves exactly like an import statement does. An import statement uses the full tryLoadModuleUsingOptionalResolutionSettings function, so I think we also should use it here.
(Plus, typeRoots is already explicitly handled when looking up @types names inside primaryLookup)
There was a problem hiding this comment.
I think the only 🤷♀️❓🤷♀️ kinda thing is weather a typeRoots resolution should have priority over a paths/baseUrl one or not. I'd argue that paths and baseUrl are more specific compiler options, so should have priority (hence this placement).
There was a problem hiding this comment.
But typeRoots also is in compilerOption and i think is set when user prefers to resolve type reference directive from certain location.. When not set i agree that it baseUrl makes sense
There was a problem hiding this comment.
Yeah, but typeRoots a list of potential directories, while paths is exact matches and baseUrl is a single directory. That's why paths > baseUrl > typeRoots makes sense to me. (typeRoots will never conflict with rootDirs as it were because typeRoots are only relevant for non-relative names, while rootDirs are only relevant for relative names)
|
Also relevant for mapping of older versions of |
| @@ -354,7 +354,10 @@ namespace ts { | |||
| } | |||
| const conditions = features & NodeResolutionFeatures.Exports ? features & NodeResolutionFeatures.EsmMode ? ["node", "import", "types"] : ["node", "require", "types"] : []; | |||
| const moduleResolutionState: ModuleResolutionState = { compilerOptions: options, host, traceEnabled, failedLookupLocations, packageJsonInfoCache: cache, features, conditions }; | |||
| let resolved = primaryLookup(); | |||
| const loader: ResolutionKindSpecificLoader = (extensions, candidate, onlyRecordFailures, state) => nodeLoadModuleByRelativeName(extensions, candidate, onlyRecordFailures, state, /*considerPackageJson*/ true); | |||
| const optional = tryLoadModuleUsingOptionalResolutionSettings(Extensions.DtsOnly, typeReferenceDirectiveName, containingDirectory || "", loader, moduleResolutionState); | |||
There was a problem hiding this comment.
Is containingDirectory || host.getCurrentDirectory() better than containingDirectory || "" ?
There was a problem hiding this comment.
Hmm, maybe... nah, if anything you'd want the project root, I think. currentDirectory is never actually undefined in practice, however. The resolveTypeReferenceDirectiveNamesWorker it's always wrapped up into (in our uses, anyway) always passes a non-undefined containingFile, so I think it's mostly a quirk of the public signature.
| const loader: ResolutionKindSpecificLoader = (extensions, candidate, onlyRecordFailures, state) => nodeLoadModuleByRelativeName(extensions, candidate, onlyRecordFailures, state, /*considerPackageJson*/ true); | ||
| const optional = tryLoadModuleUsingOptionalResolutionSettings(Extensions.DtsOnly, typeReferenceDirectiveName, containingDirectory || "", loader, moduleResolutionState); | ||
| // tryLoadModuleUsingOptionalResolutionSettings will happily return what a user wrote in `paths` varbatim - this means if a user writes `.js`, it'll return a `.js` file, even if we're looking for TS files | ||
| let resolved = optional && hasTSFileExtension(optional.path) && { fileName: optional.path, packageId: optional.packageId } || primaryLookup(); |
There was a problem hiding this comment.
Does this mean we need to change auto addition of type reference directive during declaration emit
There was a problem hiding this comment.
AFAIK, those reuse resolutions found during program construction - so long as we initially make the non-relative name here, we should reuse it in declaration emit.
|
@weswigham is this PR worth keeping?
|
github-project-automation
bot
moved this from Waiting on reviewers
to Done
in PR Backlog
Everyone I've spoken to seems to agree that
///<reference typescomments not following thepaths(andbaseUrl) compiler options seems to be an oversight on our part.Fixes an issue I brought up on our teams chat while doing work on #48278.