Ignore reparsed nodes when determining external module indicator

[jakebailey]

These fake nodes should not contribute to the module-ness of a file.

[Copilot]

Pull Request Overview

This PR fixes module detection in CommonJS files by preventing reparsed nodes (synthetic nodes created during parsing, such as from JSDoc tags) from being considered as external module indicators. The change adds a guard in isAnExternalModuleIndicatorNode to skip nodes with the NodeFlagsReparsed flag.

Key Changes:

• Added early return in isAnExternalModuleIndicatorNode when NodeFlagsReparsed flag is set
• Numerous baseline updates showing removal of Object.defineProperty(exports, "__esModule", { value: true }); from generated JavaScript
• Several test baseline convergences with TypeScript reference implementation (reduction in .diff files)

Reviewed Changes

Copilot reviewed 76 out of 76 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
internal/ast/parseoptions.go	Added check to filter out reparsed nodes from external module indicator detection
testdata/baselines/reference/submodule/conformance/untypedModuleImport_allowJs.*	Regression: JS module no longer treated as ES module, causing type errors
testdata/baselines/reference/submodule/conformance/moduleExportAliasImported.types*	Improved: Dynamic import now correctly returns wrapped object type
Various *.js baseline files	Removal of Object.defineProperty(exports, "__esModule", ...) preamble
Various *.diff baseline files	Convergence with TypeScript reference implementation

[DanielRosenwasser]

Can you give more context on why this was happening? I don't get what was being injected that was module-like but wouldn't imply that the output should be a module.

[andrewbranch]

I can't immediately figure out what's going on with this diff, but it's the only one that didn't shrink

[jakebailey]

The diff makes it look worse than it is; this is actually a preexisting bug @sandersn also just found in #2047

[jakebailey]

Can you give more context on why this was happening? I don't get what was being injected that was module-like but wouldn't imply that the output should be a module.

Yes, for example @callback can get turned into export type Foo = () => void, and this code would find that node and go "oh, there's an export" and use that to determine if it's a module. Or module.exports = {} turned into export = {} and that gets caught too.

Basically, the thinking is that the old tree in Strada would never have any of these new reparsed nodes, so to match algorithms, we need to ignore them too.

[DanielRosenwasser]

I'm sure this is an okay fix, but I think the more appropriate thing would have been for the @callback to transform differently based on if the containing file was originally a module. Having exported nodes in a global file is very suspicious for any later analysis.

[jakebailey]

That isn't possible; reparsing happens on the fly, whereas module-ness is determined after the fact.

I agree that export keywords should not appear; I suspect you're looking for a fix more like #1999 which I think may have just missed @callback.

[jakebailey]

Filed #2048

[ahejlsberg]

I'm not sure I agree with this PR. Reparsed nodes are intended to replace JSDoc nodes (from a semantic point of view) such that downstream logic doesn't have to examine the JSDoc. But if we ignore them, then we push the responsibility downstream--exactly what we were trying to avoid. Once I get my fix for #2048 up, reparsed JSTypeAliasDeclaration nodes resulting from @callback wont have an export modifier, at which point this PR doesn't serve any purpose.

[ahejlsberg]

I recommend we revert this PR.

[jakebailey]

I can do that if you have the follow up prepared.

[jakebailey]

#2062