Preserve native destructuring for CommonJS exports to keep iterator semantics

[Copilot]

• Visit v.Name() once and reuse the visited node for both the preserved declaration and the generated exports.X = X assignments, so any visitor rewrites stay consistent.
• Add EFNoSourceMap to the synthetic propertyAccess so it actually matches the stated intent of suppressing comments and source-map ranges on the synthetic export reference.
• Extend exportDestructuringIterator.ts with: an array pattern with omitted elements ([, e, , f]), a nested array-of-array/object pattern ([[g, h], { i, j: k }]), and a nested object-with-nested-pattern ({ m: [n, o], p: { q } }) to cover the recursion and omitted-element paths of appendExportAssignmentsForBindingPattern.
• Address review feedback: rename name parameter to pattern in appendExportAssignmentsForBindingPattern
• Address review feedback: fix misleading comment about alias exports
• Add new .diff files to submoduleAccepted.txt with link to When using module=commonjs, Array destructuring is transpiled with wrong (non-iterator) behavior #3701
• Simplify: use original variable declaration directly via pushVariable(variable) instead of reconstructing with NewVariableDeclaration
• Build, full test suite, lint, format all clean.

[jakebailey]

This does what it says on the tin, but, I'm not sure what affect this has on cjs-module-lexer's scanning.

@blickly Is this the change you were hoping to get?

[blickly]

Honestly, I was expecting something more like:

const [A, V] = foo();
exports.A = A;
exports.V = V;

but this version is technically not wrong, and is definitely and improvement over the old behavior.

[jakebailey]

Yeah. I think what you've suggested would be better just because it avoids the lexer problem.

@copilot please try

[Copilot]

Yeah. I think what you've suggested would be better just because it avoids the lexer problem.

@copilot please try

Reworked in 4a6c647 to emit the lexer-friendly form. For export const [A, V] = foo() the output is now:

const [A, V] = foo();
exports.A = A, exports.V = V;

The variable declaration is preserved as-is (so array destructuring keeps native iterator semantics), and exports.X = X is emitted for each declared identifier — so cjs-module-lexer can pick them up.

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Updates CommonJS module emit to preserve native destructuring for exported binding patterns (especially arrays) so iterator semantics aren’t lost, while still emitting explicit exports.X = X assignments for tooling compatibility.

Changes:

• Preserve exported binding-pattern variable declarations (instead of flattening to index/property access) and emit per-identifier exports.X = X assignments.
• Add a new helper to walk binding patterns (nesting/rest/defaults/omissions) and generate the export assignments with suppressed synthetic comment/source-map metadata.
• Add a regression test and update multiple compiler baselines to reflect the new emit shape.

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
internal/transformers/moduletransforms/commonjsmodule.go	Preserves exported destructuring declarations and adds helper to emit exports.X = X for each bound identifier.
testdata/tests/cases/compiler/exportDestructuringIterator.ts	New test case covering exported destructuring patterns under module=commonjs.
testdata/baselines/reference/compiler/exportDestructuringIterator.js	New baseline verifying native destructuring + explicit export assignments.
testdata/baselines/reference/compiler/exportDestructuringIterator.types	New type baseline for the added test.
testdata/baselines/reference/compiler/exportDestructuringIterator.symbols	New symbol baseline for the added test.
testdata/baselines/reference/compiler/exportDestructuring.js	Baseline update reflecting the new emit for exported array destructuring.
testdata/baselines/reference/submodule/compiler/exportObjectRest(module=commonjs,target=esnext).js	Baseline update to preserve native object rest destructuring + explicit export assignments.
testdata/baselines/reference/submodule/compiler/exportObjectRest(module=commonjs,target=esnext).js.diff	Diff baseline capturing the change in output shape.
testdata/baselines/reference/submodule/compiler/exportEmptyObjectBindingPattern(module=commonjs,target=esnext).js	Baseline update to emit native empty object destructuring.
testdata/baselines/reference/submodule/compiler/exportEmptyObjectBindingPattern(module=commonjs,target=esnext).js.diff	Diff baseline capturing the change in output shape.
testdata/baselines/reference/submodule/compiler/exportEmptyArrayBindingPattern(module=commonjs,target=esnext).js	Baseline update to emit native empty array destructuring.
testdata/baselines/reference/submodule/compiler/exportEmptyArrayBindingPattern(module=commonjs,target=esnext).js.diff	Diff baseline capturing the change in output shape.
testdata/baselines/reference/submodule/compiler/downlevelLetConst13(target=es2015).js	Baseline update for exported destructuring emit now producing locals + export assignments.
testdata/baselines/reference/submodule/compiler/downlevelLetConst13(target=es2015).js.diff	Diff baseline capturing the change in output shape.
testdata/baselines/reference/submodule/compiler/destructuringInVariableDeclarations1.js	Baseline update for exported destructuring emit now producing locals + export assignment.
testdata/baselines/reference/submodule/compiler/destructuringInVariableDeclarations1.js.diff	Diff baseline capturing the change in output shape.
testdata/baselines/reference/submodule/compiler/declarationEmitRetainsJsdocyComments.js	Baseline update reflecting preserved binding-pattern declaration with JSDoc-like comment placement.
testdata/baselines/reference/submodule/compiler/declarationEmitRetainsJsdocyComments.js.diff	Diff baseline capturing the change in output shape.
testdata/baselines/reference/submodule/compiler/bindingPatternOmittedExpressionNesting.js	Baseline update reflecting preserved native destructuring for a pattern consisting only of omissions/nesting.
testdata/baselines/reference/submodule/compiler/bindingPatternOmittedExpressionNesting.js.diff	Diff baseline capturing the change in output shape.

[jakebailey]

@copilot please consider the review comments

[Copilot]

@copilot please consider the review comments

Addressed all three review comments in de75861:

• Visit v.Name() once and reuse the visited node for both the preserved declaration and the generated exports.X = X assignments.
• Added EFNoSourceMap to the synthetic propertyAccess so it matches the comment's stated intent.
• Expanded exportDestructuringIterator.ts with omitted-element ([, e, , f]), nested array-of-array/object ([[g, h], { i, j: k }]), and nested object-with-nested-pattern ({ m: [n, o], p: { q } }) cases.

[jakebailey]

@copilot isn't it a bad idea to reuse the same node twice in the tree? are you sure you've done this correctly?

[Copilot]

@copilot isn't it a bad idea to reuse the same node twice in the tree? are you sure you've done this correctly?

Good catch to double-check — the binding pattern node itself is only placed at one spot in the emitted tree (as the Name of the new VariableDeclaration at commonjsmodule.go:1091-1096). appendExportAssignmentsForBindingPattern only walks the visited pattern to enumerate leaf identifiers and then clones each one via bindingName.Clone(tx.Factory()) before putting it into the synthesized exports.X = X assignment (commonjsmodule.go:1166 and 1175). So the cloned identifiers — not the original — are what end up in the export assignments. This mirrors the existing pattern in the same file for direct exports (commonjsmodule.go:1525-1534 and 1547-1557), which also name.Clone(...) before reusing as the property-access name.

The reason for sharing visitedName between the declaration and the walk is just so the helper enumerates the visited identifiers (in case the visitor renamed any), not so the visited node is inserted twice.

[Copilot]

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.

[jakebailey]

@copilot Address feedback. Also, any new .diff files this PR introduces should be put into submoduleAccepted, with a link back to #3701

[Copilot]

@copilot Address feedback. Also, any new .diff files this PR introduces should be put into submoduleAccepted, with a link back to #3701

Addressed in 81c74ea:

• Renamed name parameter to pattern in appendExportAssignmentsForBindingPattern for clarity.
• Fixed the misleading comment about alias exports — now says they're emitted "in addition" rather than being "left to" appendExportsOfVariableStatement.
• Moved all 7 new .diff files to submoduleAccepted/ and added entries to submoduleAccepted.txt with a link to When using module=commonjs, Array destructuring is transpiled with wrong (non-iterator) behavior #3701.

(The trailing-whitespace baseline comment from the reviewer is a pre-existing printer behavior, not something introduced by this PR.)

[weswigham]

I'm like 80% sure this should be Update instead of New - actually, hold on, nothing changes from the transform here (quite literally we're using the variable declaration verbatim now), so all this peeling apart and putting back together is extraneous. It should just be pushVariable(v) followed by a tx.appendExportAssignmentsForBindingPattern(v.Name(), pushExpression), no?

[jakebailey]

Yeah, it could (the PR evolved so this I think is leftover).

@copilot please address :)

[Copilot]

Simplified in 6134812 — now just pushVariable(variable) followed by tx.appendExportAssignmentsForBindingPattern(v.Name(), pushExpression). The reconstruction with NewVariableDeclaration + VisitNode is removed.

[weswigham]

I mentioned this online, but I'll include it here for posterity - transforming export const [A, B] = whatever to

const [A, B] = whatever
exports.A = A, exports.B = B

does look nicer, but it breaks export visibility timing compared to the current (array-assuming) emit. So if B is fetched by a getter, that getter should be able to see exports.A already set - but that's not the case with this emit. Here's a whole test case:

import * as self from "./test.js"
const obj = { [Symbol.iterator]: function* () { yield "ok"; yield self.A; } }

export const [A, B] = obj

both A and B have value "ok" at runtime.

I don't know immediately how to transform it both to respect the iterator and the export timing without being very verbose with converting the array destructuring to an iterator you manually next.

[blickly]

If the goal is to follow both the iterator protocol and export ordering, the original [exports.A, exports.B] = foo() that Copilot started with actually works fine.

That said, I question if we need to hold to that constraint here.

No argument from me that the suggested emit isn't fully ES spec-compliant, but we don't need to fix that here. Getting TSGo to parity with TS6 is the goal, and TS6 already has the exact same timing caveats when run with downlevelIteration. (Compare with this Playground link).

Also, I'm not sure I followed exactly what you meant by "compared to the current (array-assuming) emit." The array-assuming version seems even more broken, since it doesn't support iterators at all, much less iterators that depend on interleaving iteration with export visibility.

[jakebailey]

I think another option is to do:

[exports.A, exports.B] = foo()
0 && exports.A = void 0;
0 && exports.B = void 0;

Or something, which is somewhat similar to what esbuild does to "declare" exports: https://jakebailey.dev/esbuild-playground/#v3=PTAEAEDMEsBsFMB2BDAtvAXKAxge0TAOYB0AVgM74BQA3laKAERIAuATgJ4AKu0iL5RlgDajYHwAm8AB7EBjALoAaek0i42qZCyFNsFRioaMADrG3rNuxolxTGVAL5UqICDAQp0WSTLnkqGRMNFlBIAFdEbBZofFBYXEIACgBKUDoGPERKBGIE5MYAd1xcRhSnKiA

[jakebailey]

Wait, we don't need any of this, we already emit a big exports.A = exports.B = void 0 at the top of the module, so they are all declared.

I think we can just do a simple destructuring like [exports.A, exports.B] = ..., that is, the original change, oops!