Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Triple-slash reference type directives can override the import mode used for their resolution #47732

Merged
merged 6 commits into from Feb 15, 2022

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Feb 3, 2022

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 node12 or nodenext 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

…sed for their resolution

They now use the file's default mode by default, rather than always using commonjs. The new arguments to the
reference directive look like:

```ts
///<reference types="pkg" resolution-mode="require" />
```

or

```ts
///<reference types="pkg" resolution-mode="import" />
```
@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Feb 3, 2022
Copy link
Member

@andrewbranch andrewbranch 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, });
const mode = 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
Member Author

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

As-is, this always emits the appropriate resolution-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-bot typescript-bot added For Milestone Bug PRs that fix a bug with a specific milestone and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Feb 9, 2022
@weswigham
Copy link
Member Author

weswigham commented Feb 9, 2022

I now omit the resolution-mode override if it's unneeded in the resulting file, so they're not usually 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 complicate FileReferences a bit, since they'd need to somehow inherit their original file's applied resolution mode.

@weswigham
Copy link
Member Author

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

@andrewbranch andrewbranch 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
Member Author

In the resolutionCache, the mode 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.

@weswigham
Copy link
Member Author

Specifically, it wasn't being propagated here.

Copy link
Member

@sandersn sandersn 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.

src/compiler/emitter.ts Outdated Show resolved Hide resolved
src/compiler/emitter.ts Outdated Show resolved Hide resolved
src/compiler/moduleNameResolver.ts Outdated Show resolved Hide resolved
tracing?.push(tracing.Phase.Program, "resolveTypeReferenceDirectiveNamesWorker", { containingFileName });
performance.mark("beforeResolveTypeReference");
const result = actualResolveTypeReferenceDirectiveNamesWorker(typeDirectiveNames, containingFileName, redirectedReference);
const result = 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

src/compiler/program.ts Outdated Show resolved Hide resolved
src/compiler/resolutionCache.ts Outdated Show resolved Hide resolved
Copy link
Member

@sandersn sandersn 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.ts

Co-authored-by: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Relates to the public API for TypeScript Author: Team Breaking Change Would introduce errors in existing code For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
5 participants