Fix declaration emit for JS default re-exports that resolve to modules through synthesized default exports #56340
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
There was a problem hiding this comment.
You're definitely right that this feels a little bespoke; our ability to identify synthetic default symbols is a bit rough, since the module symbol is just returned for the default member. One day we should try swapping that to a generated Alias symbol, or at least making a bespoke symbol for the export, rather than just returning the module symbol directly.
Still, for now an approach like this one is probably fine.
| @@ -9707,6 +9707,9 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| // does not use localName because the symbol name in this case refers to the name in the exports table, | |||
| // which we must exactly preserve | |||
| const specifier = (node.parent.parent as ExportDeclaration).moduleSpecifier; | |||
| if (specifier && target?.flags & SymbolFlags.ValueModule && (node as ExportSpecifier).propertyName?.escapedText === InternalSymbolName.Default) { | |||
There was a problem hiding this comment.
target?.flags & SymbolFlags.ValueModule is also set for namespaces (via bindModuleDeclaration), which we don't want to trigger this - honestly, I think the reliable indicators would be either the symbol declarations having a source file or ambient module declaration in them.
There was a problem hiding this comment.
The thing that feels weird about the line of code I wrote here is that it’s checking both the symbol structure and the original syntax. It seems like when the JS contains ESM syntax like this, the desired outcome is just to copy the syntax verbatim, but maybe there are edge cases where that’s not true? On one hand, it feels possible to do only a check of the symbol like you’re suggesting and assert that the export specifier was default, but on the other hand, I think I could also not check the symbol at all and just set the target name to default every time the original syntax did a default re-export.
This would make things much easier to reason about. I wonder how much memory penalty it would incur. |
|
@weswigham can you take a look at my latest commit? |
There was a problem hiding this comment.
This is... probably fine? I can't think of a case where there'd be an export declaration named default available but we shouldn't use the name default when serializing it. Reverse-engineering the correct symbol name when the symbol's been repurposed from another location and its actual .name isn't reliable is definitely annoying. 100% on the backlog of things-to-make-better from an API usability point of view.
Fixes #55082
I think this is too narrowly targeted—@weswigham is this on the right track?