Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a declarations (.d.ts) emit porting issue in the declaration transformer where a SyntaxList replacement was incorrectly checked as an external-module indicator, instead of checking the list elements.
Changes:
- Correct external module indicator detection for late-painted statement replacements that return a
SyntaxListby checking each child statement node.
| if replacement.Kind == ast.KindSyntaxList { | ||
| if !tx.needsScopeFixMarker || !tx.resultHasExternalModuleIndicator { | ||
| for _, elem := range replacement.AsSyntaxList().Children { | ||
| if needsScopeMarker(elem) { | ||
| tx.needsScopeFixMarker = true | ||
| } | ||
| if ast.IsSourceFile(statement.Parent) && ast.IsExternalModuleIndicator(replacement) { | ||
| if ast.IsSourceFile(statement.Parent) && ast.IsExternalModuleIndicator(elem) { | ||
| tx.resultHasExternalModuleIndicator = true | ||
| } |
There was a problem hiding this comment.
This fixes external-module-indicator detection for SyntaxList replacements, but there’s no regression test covering the SyntaxList path (where a transformed statement expands into multiple statements including an import/export). Please add a minimal compiler baseline test that triggers a late-painted statement replacement returning a SyntaxList with an external module indicator, and asserts we don’t incorrectly inject the empty export {} marker due to resultHasExternalModuleIndicator staying false.
There was a problem hiding this comment.
Yeah, though given this is syntax list expansion, maybe this was just for JS dts emit or something
Noticed this porting bug; the syntax list is not going to be an external module indicator. However, it does not seem to matter.