- Notifications
You must be signed in to change notification settings - Fork13.2k
Triple-slash reference type directives can override the import mode used for their resolution#47732
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…sed for their resolutionThey now use the file's default mode by default, rather than always using commonjs. The new arguments to thereference directive look like:```ts///<reference types="pkg" resolution-mode="require" />```or```ts///<reference types="pkg" resolution-mode="import" />```
andrewbranch left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Re emit: I’m in favor of what you have now, not changing what the user wrote in the directive. Looks good to me at a glance, will take a closer look tomorrow.
| processTypeReferenceDirective(fileName,resolvedTypeReferenceDirective,{kind:FileIncludeKind.TypeReferenceDirective,file:file.path, index,}); | ||
| constmode=ref.resolutionMode||file.impliedNodeFormat; | ||
| if(mode&&getEmitModuleResolutionKind(options)!==ModuleResolutionKind.Node12&&getEmitModuleResolutionKind(options)!==ModuleResolutionKind.NodeNext){ | ||
| programDiagnostics.add(createDiagnosticForRange(file,ref,Diagnostics.Resolution_modes_are_only_supported_when_moduleResolution_is_node12_or_nodenext)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hm, is this going to be problematic (though silenceable with--skipLibCheck) for library code? I guess all library code that’s written with nodenext can potentially present problems for consumers in a different resolution mode, so maybe this is nothing new.
weswigham commentedFeb 4, 2022
As-is, thisalways emits the appropriate |
andrewbranch commentedFeb 4, 2022
Oh, I misunderstood. Hm, I’ll have to think about that. I think it complicates the library code problem more. If you happen to compile your library in nodenext but resolution would work for consumers under commonjs, it would be a shame to have errors on those references just for including superfluous syntax. |
andrewbranch commentedFeb 4, 2022
I think on principle I lean toward leaving code the user wrote as-is unless there’s a good reason it needs to be changed. |
weswigham commentedFeb 9, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I now omit the |
weswigham commentedFeb 9, 2022
I've mentioned it in some linked issues, but the cache changes in this PR also happen to fix some language service issues to do with format detection caching, and as such this now includes a test to that effect. ♥ |
andrewbranch left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Would be good to get another set of eyes since it’s a big changeset, but looks good to me (for merging after 4.6 RC, I assume). Do you know where the bug in the caching code was that you accidentally fixed?
weswigham commentedFeb 9, 2022
In the |
weswigham commentedFeb 9, 2022
Specifically, it wasn't being propagatedhere. |
sandersn left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Some comments from looking over the code. I'll look at the tests next.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| tracing?.push(tracing.Phase.Program,"resolveTypeReferenceDirectiveNamesWorker",{ containingFileName}); | ||
| performance.mark("beforeResolveTypeReference"); | ||
| constresult=actualResolveTypeReferenceDirectiveNamesWorker(typeDirectiveNames,containingFileName,redirectedReference); | ||
| constresult=actualResolveTypeReferenceDirectiveNamesWorker(typeDirectiveNames,containingFileName,redirectedReference,containingFileMode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
resolveTypeReferenceDirectiveNamesWorker -> actualResolveTypeReferenceDirectiveNamesWorker
I think our layers of indirection have gotten out of control
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
sandersn left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think I basically get what's going on, but I don't think I can predict many of the ramifications. I'd vote that we merge this early enough for people to try it out.
Uh oh!
There was an error while loading.Please reload this page.
tests/baselines/reference/nodeModulesTripleSlashReferenceModeDeclarationEmit5(module=node12).jsShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
tests/cases/conformance/node/nodeModulesTripleSlashReferenceModeOverride3.ts OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
…deOverride3.tsCo-authored-by: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com>
079515f tof919c2aCompare
Uh oh!
There was an error while loading.Please reload this page.
They now use the file's default mode by default, rather than always using commonjs. The new arguments to the
reference directive look like:
///<reference types="pkg" resolution-mode="require" />or
///<reference types="pkg" resolution-mode="import" />We consider it an error to see these kinds of references outside of
node12ornodenextresolution. This change is separate from the change adding similar configurability for type-only imports (and import types), as the plumbing work to support multi-modal type reference directives is much more extensive (since the API changes to support modality for them was not yet in place), so is probably best reviewed on its own.Fixes#47795
Fixes#47806