Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged

Conversation

@weswigham
Copy link
Member

@weswighamweswigham commentedFeb 3, 2022
edited
Loading

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 ofnode12 ornodenext resolution. 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

jaydenseric and ExE-Boss reacted with eyes emoji
…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" />```
@typescript-bottypescript-bot added Author: Team For Uncommitted BugPR for untriaged, rejected, closed or missing bug labelsFeb 3, 2022
Copy link
Member

@andrewbranchandrewbranch left a 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));
Copy link
Member

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
Copy link
MemberAuthor

Re emit: I’m in favor of what you have now, not changing what the user wrote in the directive.

As-is, thisalways emits the appropriateresolution-mode in the directive, weather it was originally explicitly written or not.

@andrewbranch
Copy link
Member

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
Copy link
Member

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.

@typescript-bottypescript-bot added For Milestone BugPRs that fix a bug with a specific milestone and removed For Uncommitted BugPR for untriaged, rejected, closed or missing bug labelsFeb 9, 2022
@weswigham
Copy link
MemberAuthor

weswigham commentedFeb 9, 2022
edited
Loading

I now omit theresolution-mode override if it's unneeded in the resulting file, so they're notusually present. While preserving the input formatting where possible sounds nice, to do so needs context about where the original comment came from preserved (so we know when we have to change that formatting), which, since it's derived from a comment, is not the case right now (unlike nodes, which retain pointers back to their original source,FileReferences contain no such data and get copied around willy-nilly - we happily pull type references from random other files in your build!). It's probably doable, but'd complicateFileReferences a bit, since they'd need to somehow inherit their original file's applied resolution mode.

@weswigham
Copy link
MemberAuthor

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. ♥

Copy link
Member

@andrewbranchandrewbranch left a 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
Copy link
MemberAuthor

In theresolutionCache, themode wasn't consistently handed in by all callers before, which wasn't obvious prior to this PR because triple-slash references intentionally didn't pass a mode.

andrewbranch reacted with thumbs up emoji

@weswigham
Copy link
MemberAuthor

Specifically, it wasn't being propagatedhere.

Copy link
Member

@sandersnsandersn left a 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.

tracing?.push(tracing.Phase.Program,"resolveTypeReferenceDirectiveNamesWorker",{ containingFileName});
performance.mark("beforeResolveTypeReference");
constresult=actualResolveTypeReferenceDirectiveNamesWorker(typeDirectiveNames,containingFileName,redirectedReference);
constresult=actualResolveTypeReferenceDirectiveNamesWorker(typeDirectiveNames,containingFileName,redirectedReference,containingFileMode);
Copy link
Member

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

Copy link
Member

@sandersnsandersn left a 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.

…deOverride3.tsCo-authored-by: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com>
@DanielRosenwasserDanielRosenwasser added Breaking ChangeWould introduce errors in existing code Domain: APIRelates to the public API for TypeScript labelsApr 5, 2022
sheetalkamat added a commit to microsoft/TypeScript-TmLanguage that referenced this pull requestMay 24, 2022
@microsoftmicrosoft locked asresolvedand limited conversation to collaboratorsOct 22, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@sandersnsandersnsandersn approved these changes

@andrewbranchandrewbranchandrewbranch approved these changes

@amcaseyamcaseyAwaiting requested review from amcasey

Assignees

@weswighamweswigham

Labels

Author: TeamBreaking ChangeWould introduce errors in existing codeDomain: APIRelates to the public API for TypeScriptFor Milestone BugPRs that fix a bug with a specific milestone

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

VS Code/language server doesn't properly detectNodeNext module kind based onpackage.json Node 12 ESM reports different errors between editor and CLI

5 participants

@weswigham@andrewbranch@sandersn@DanielRosenwasser@typescript-bot

[8]ページ先頭

©2009-2025 Movatter.jp