Fix type-only imports in interface 'extends' and import=/export=#36496
Fix type-only imports in interface 'extends' and import=/export=#36496andrewbranch merged 15 commits intomicrosoft:masterfrom
Conversation
This reverts commit 7444b0b.
sandersn
left a comment
sandersn
left a comment
There was a problem hiding this comment.
Some initial comments. I'm still reading the checker.ts changes.
src/compiler/checker.ts
Outdated
| if (links.typeOnlyDeclaration === undefined) { | ||
| const typeOnly = find(resolvesToSymbol.declarations, isTypeOnlyImportOrExportDeclaration); | ||
| if (links.typeOnlyDeclaration === undefined || overwriteEmpty && links.typeOnlyDeclaration === false) { | ||
| resolvesToSymbol = resolvesToSymbol.exports?.get(InternalSymbolName.ExportEquals) ?? resolvesToSymbol; |
There was a problem hiding this comment.
I would declare a new variable just because I hate mutation so much:
| resolvesToSymbol = resolvesToSymbol.exports?.get(InternalSymbolName.ExportEquals) ?? resolvesToSymbol; | |
| const resolved = resolvesToSymbol.exports?.get(InternalSymbolName.ExportEquals) ?? resolvesToSymbol; |
Not sure about the name.
There was a problem hiding this comment.
Are there other kinds of aliases we might have to manually follow the same way as import =? This seems like a slightly odd place to manually get ExportEquals out of the exports, although I could be wrong.
There was a problem hiding this comment.
Are there other kinds of aliases we might have to manually follow the same way as
import =?
The answer is almost definitely “no,” but I’ve been struggling to concisely explain why for too long at this point, which makes me realize that the reason I’m confident that I’m right is mostly because of thorough test coverage.
sandersn
left a comment
sandersn
left a comment
There was a problem hiding this comment.
Lots of questions and comments. I can't tell how multi-part chains are getting followed from reading the code.
src/compiler/checker.ts
Outdated
| markSymbolOfAliasDeclarationIfResolvesToTypeOnly(specifier, exportSymbol); | ||
| return resolveSymbol(exportSymbol, dontResolveAlias); | ||
| const resolved = resolveSymbol(exportSymbol, dontResolveAlias); | ||
| if (!markSymbolOfAliasDeclarationIfResolvesToTypeOnly(specifier, exportSymbol)) { |
There was a problem hiding this comment.
this call pair happens quite a few times. I think it should be extracted, perhaps into markSymbolOfAliasDeclarationIfResolvesToTypeOnly itself. I can't tell if it's possible and easy to regularize the other calls to accommodate the 1-export 2-resolved pattern.
There was a problem hiding this comment.
I had the same thought when reviewing my own PR this morning, but stopped when I couldn’t come up with a satisfying name to add more logic to a function that already has a monstrous name. Any suggestions?
There was a problem hiding this comment.
Combined all three type-only checks (1. declaration is marked type-only; 2. immediate alias is marked type-only; 3. final alias is marked type-only) into one markSymbolOfAliasDeclarationIfTypeOnly call
weswigham
left a comment
weswigham
left a comment
There was a problem hiding this comment.
export default <expr> behaves a lot like export = <expr> and probably also needs a test.
|
@weswigham like this one? The large majority of type-only tests already existed. There are also some new |
|
I was thinking like this one but with |
|
I just commented on #36478 as I don't think this fixes the issue when the type is another level deep because it appears I can do this: import type * as monaco from "monaco-editor";
interface MyViewZone extends monaco.IRange {
marginDomNode: HTMLElement;
}but not this: import type * as monaco from "monaco-editor";
interface MyViewZone extends monaco.editor.IViewZone {
marginDomNode: HTMLElement;
} |
|
Ah, though as a workaround I can do: import type * as monaco from "monaco-editor";
type IViewZone = monaco.editor.IViewZone;
interface MyViewZone extends IViewZone {
marginDomNode: HTMLElement;
} |
6 participants
tsc)—tested in the added unit test.