Skip to content

Improve CommonJS support #2893

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ahejlsberg wants to merge 2 commits into from
Closed

Improve CommonJS support #2893

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b9dc90f
Permit multiple module.exports assignments, merge module into export=
ahejlsberg Feb 24, 2026
8ed3fe8
Accept new baselines
ahejlsberg Feb 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 37 additions & 10 deletions internal/checker/checker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2299,7 +2299,7 @@ func (c *Checker) checkSourceElementWorker(node *ast.Node) {
c.checkImportEqualsDeclaration(node)
case ast.KindExportDeclaration:
c.checkExportDeclaration(node)
case ast.KindExportAssignment, ast.KindJSExportAssignment:
case ast.KindExportAssignment:
c.checkExportAssignment(node)
case ast.KindEmptyStatement:
c.checkGrammarStatementInAmbientContext(node)
Expand Down Expand Up @@ -5098,7 +5098,7 @@ func (c *Checker) checkModuleAugmentationElement(node *ast.Node) {
for _, decl := range node.AsVariableStatement().DeclarationList.AsVariableDeclarationList().Declarations.Nodes {
c.checkModuleAugmentationElement(decl)
}
case ast.KindExportAssignment, ast.KindJSExportAssignment, ast.KindExportDeclaration:
case ast.KindExportAssignment, ast.KindExportDeclaration:
c.grammarErrorOnFirstToken(node, diagnostics.Exports_and_export_assignments_are_not_permitted_in_module_augmentations)
case ast.KindImportEqualsDeclaration:
// import a = e.x; in module augmentation is ok, but not import a = require('fs)
Expand Down Expand Up @@ -15158,13 +15158,37 @@ func (c *Checker) GetAmbientModules() []*ast.Symbol {
}

func (c *Checker) resolveExternalModuleSymbol(moduleSymbol *ast.Symbol, dontResolveAlias bool) *ast.Symbol {
if moduleSymbol != nil {
exportEquals := c.resolveSymbolEx(moduleSymbol.Exports[ast.InternalSymbolNameExportEquals], dontResolveAlias)
if exportEquals != nil {
return c.getMergedSymbol(exportEquals)
if moduleSymbol == nil {
return nil
}
exportEquals := c.resolveSymbolEx(moduleSymbol.Exports[ast.InternalSymbolNameExportEquals], dontResolveAlias)
if exportEquals == nil {
return moduleSymbol
}
if exportEquals == c.unknownSymbol || exportEquals == moduleSymbol || exportEquals.Flags&ast.SymbolFlagsAlias != 0 || len(moduleSymbol.Exports) == 1 {
return c.getMergedSymbol(exportEquals)
}
// We have a module with an export= declaration along with other exported declarations. This combination may occur
// in .js files with module.exports assignments and JSDoc @typedef type declarations. Merge the exported symbols
// of the module into the exports of the export= symbol to make the types accessible.
links := c.moduleSymbolLinks.Get(exportEquals)
if links.mergedExportEquals != nil {
return links.mergedExportEquals
}
merged := c.cloneSymbol(exportEquals)
Copy link
Copy Markdown
Member

@weswigham weswigham Feb 26, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's funny, because this was, hands down, the biggest thing we wanted to avoid by using the reparsing approach for JSDoc. At least in my opinion.

Supporting this in the checker, without a doubt, requires some thought on how to support it in the declaration transformer, which seems absent here thus far. Naively, we'll produce something like

export type Foo = number | string
declare const _exports: number
export = _exports

which, uhhh, not valid TS. You can see that in a bunch of the .js diffs. In strada, what we'd do, symbolically, was see if the export= target was namespace-y, and, if so, make a namespace to merge with that namespace that contained all these merged-in typedefs. The declaration transformer needs logic like that now that this is in place, so it can produce

declare namespace _exports {
    export { Foo };
}
declare const _exports: number;
export = _exports;
type Foo = string | number;

Copy link
Copy Markdown
Member

@weswigham weswigham Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do note, the later can still fail if whatever the export= points at doesn't support normal namespace merges! But that's more rare, at least.

Copy link
Copy Markdown
Member

@jakebailey jakebailey Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish we would just make that first example legal TS already...

Copy link
Copy Markdown
Member Author

@ahejlsberg ahejlsberg Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the biggest thing we wanted to avoid by using the reparsing approach for JSDoc

Here's the scenario that concerns me. Say you have a CommonJS module like this:

/** @typedef {string[]} Foo */
module.exports.a = 1
module.exports.b = "hello"

and a client that imports it:

const m = require("./mod");
/** @type {m.Foo} */
let x;

This works fine and you get to reference m as a namespace alias in the @type annotation. But if you instead use a module.exports assignment in the module:

/** @typedef {string[]} Foo */
module.exports = { a: 1, b: "hello" }

then the client breaks because you can't reference m.Foo. You instead have to write @type {import("./mod").Foo} which does work (because, inconsistently, we haven't made that an error). And if we don't make it an error, then we still have to support the namespace pattern you mention in declaration emit.

Do note, the later can still fail if whatever the export= points at doesn't support normal namespace merges! But that's more rare, at least.

I don't think there are cases where it fails. The export= symbol generated by a module.exports assignment will never have a namespace meaning, so it's always happy to merge with an uninstantiated namespace containing the @typedef types.

Copy link
Copy Markdown
Member Author

@ahejlsberg ahejlsberg Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I think there are two options. Either have module.exports assignments merge with an uninstantiated namespace that contains the @typedef types and support that in declaration emit, or completely eliminate exporting of @typedef types from modules with module.exports assignments. Right now we're sorta sitting in between two chairs because type annotations can use import to get at the types.

Copy link
Copy Markdown
Member

@weswigham weswigham Feb 26, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is another interesting input:

const a = require("./mod")

/**
 * @typedef {string | number} Foo
 */

module.exports = a

if you merge Foo down into a's target, Foo appears in mod, which seems wrong (but iirc is what strada does), however there's afaik no uniformly valid declaration emit. Another one is

const a = require("./mod")

/**
 * @typedef {string | number} Foo
 */

module.exports = a
module.exports[a.fieldName] = 12

ie, late-bound commonjs module exports (which maybe we don't support any more, but we used to) - those are a mire with this merge in place, and strada's behavior for both is... questionable. Especially in the context of isolated or syntactic declaration emit.

Now, a lot of this can be resolved if we do the work to allow merges like those in .d.ts files, too; that way we can just emit

export type Foo = number | string
declare const _exports: number
export = _exports

or

import * as mod from "./mod"
export type Foo = number | string
export = mod

and not error on read-back; but the issue I had when I tried to implement that is that it exposes all the badness of this merge to all TS users. If we can support it without implementation jankiness, it would definitely be preferable, though.

Copy link
Copy Markdown
Member

@sandersn sandersn Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have much to add to what Wesley said, except:

  1. In JS, we need typedefs to export alongside module.exports assignments; that's the real feedback we got from an internal user, and webpack still relies on it all over the place. We can't abandon the feature entirely.
  2. If we only support JS, we don't have to support every case. I scanned my JS corpus and found no occurrences of the re-export+typedef pattern, for example. That doesn't apply if we also want to support TS.

Overall I'm with Wesley and Jake--I think we should go for the least-janky implementation that covers TS and JS completely.

I'm not current on the state of reparsing anymore, but wouldn't a JS-only implementation shrink reparsing even further. Maybe all statement-level reparsing would get dropped?

Copy link
Copy Markdown
Member

@weswigham weswigham Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A JS-only implementation struggles to be represented as a declaration file for incremental or publishing purposes, which is strada's issue. It's OK insofar as the analysis works, but definitely not ideal for the whole stack. We can take that approach again, but we need to introduce a lot of errors banning many forms of it when declaration emit is on because we can't roundtrip it, which... maybe makes it much less appealing.

It'd definitely be best to get something working across languages (ideally requiring only syntactic analysis to produce declarations, for isolatedDeclaration's sake), for sure. The issue I had is that "an alias with stuff attached" does not behave well in our current checking pipelines - I was hacking stuff up left and right and it just felt suuuuuper janky - even moreso than this second-order merge - making things that look like aliases to constant values have type exports - but only if you're still at the alias!

Honestly, I think we probably do need a dedicated non-alias thing for the module to look like when you merge an export= into a module with type exports (since it's no longer "effectively replaced by" the target of the alias, as it's augmenting it), the trouble is maintaining that and making it "look like" the target of the alias at the same time in all the places that matters without silently dropping parts of the symbol's meaning along the way. It probably does just touch a lot, since it basically requires all the handling you'd associate with a new root symbol kind flag, I think.

Copy link
Copy Markdown
Member Author

@ahejlsberg ahejlsberg Feb 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'm going to come up with an implementation that allows both TS and JS to export types from modules with export assignments. I think I will also move the merging logic to the binder so it is generally available. I don't think we need a new kind of symbol. We should be able to add an ast.SymbolFlagsNamespaceModule flag to the export= symbol and then add the exported type symbols to the export= symbol's Exports map (which is otherwise empty). This means that the exported types will take precedence over any exported types in the export= target, but seeing as this whole construct is currently an error, it is not like it will break anything. At least in theory.

merged.Flags |= ast.SymbolFlagsValueModule
exports := ast.GetExports(merged)
for _, symbol := range moduleSymbol.Exports {
if symbol.Name != ast.InternalSymbolNameExportEquals {
if existing := exports[symbol.Name]; existing != nil {
exports[symbol.Name] = c.mergeSymbol(existing, symbol, false /*unidirectional*/)
} else {
exports[symbol.Name] = symbol
}
}
}
return moduleSymbol
links.mergedExportEquals = merged
return merged
}

// An external module with an 'export =' declaration may be referenced as an ES6 module provided the 'export ='
Expand Down Expand Up @@ -16203,13 +16227,13 @@ func (c *Checker) getTypeOfVariableOrParameterOrPropertyWorker(symbol *ast.Symbo
result = c.checkShorthandPropertyAssignment(declaration, true /*inDestructuringPattern*/, CheckModeNormal)
case ast.KindMethodDeclaration:
result = c.checkObjectLiteralMethod(declaration, CheckModeNormal)
case ast.KindExportAssignment, ast.KindJSExportAssignment:
case ast.KindExportAssignment:
if declaration.Type() != nil {
result = c.getTypeFromTypeNode(declaration.Type())
} else {
result = c.widenTypeForVariableLikeDeclaration(c.checkExpressionCached(declaration.Expression()), declaration, false /*reportErrors*/)
}
case ast.KindBinaryExpression, ast.KindCallExpression, ast.KindCommonJSExport:
case ast.KindBinaryExpression, ast.KindCallExpression, ast.KindJSExportAssignment, ast.KindCommonJSExport:
result = c.getWidenedTypeForAssignmentDeclaration(symbol)
case ast.KindJsxAttribute:
result = c.checkJsxAttribute(declaration, CheckModeNormal)
Expand Down Expand Up @@ -17661,7 +17685,7 @@ func (c *Checker) getWidenedTypeForAssignmentDeclaration(symbol *ast.Symbol) *Ty
if t == nil {
var types []*Type
for i, declaration := range symbol.Declarations {
if ast.IsBinaryExpression(declaration) && declaration.Type() != nil {
if (ast.IsBinaryExpression(declaration) || ast.IsJSExportAssignment(declaration)) && declaration.Type() != nil {
t = c.getTypeFromTypeNode(declaration.Type())
break
}
Expand Down Expand Up @@ -17696,6 +17720,9 @@ func (c *Checker) getAssignmentDeclarationInitializerType(node *ast.Node) *Type
if ast.IsCallExpression(node) {
return c.getTypeFromPropertyDescriptor(node.Arguments()[2])
}
if ast.IsJSExportAssignment(node) {
return c.getRegularTypeOfLiteralType(c.checkExpressionCached(node.Expression()))
}
if ast.IsCommonJSExport(node) {
return c.getRegularTypeOfLiteralType(c.checkExpressionCached(node.Initializer()))
}
Expand Down
1 change: 1 addition & 0 deletions internal/checker/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ type AliasSymbolLinks struct {
type ModuleSymbolLinks struct {
resolvedExports ast.SymbolTable // Resolved exports of module or combined early- and late-bound static members of a class.
typeOnlyExportStarMap map[string]*ast.Node // Set on a module symbol when some of its exports were resolved through a 'export type * from "mod"' declaration
mergedExportEquals *ast.Symbol // Resolved clone of export= symbol with containing module exports merged into it
exportsChecked bool
}

Expand Down
7 changes: 1 addition & 6 deletions testdata/baselines/reference/compiler/erasableSyntaxOnlyJS.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
foo.js(1,1): error TS1203: Export assignment cannot be used when targeting ECMAScript modules. Consider using 'export default' or another module format instead.
index.ts(2,1): error TS1202: Import assignment cannot be used when targeting ECMAScript modules. Consider using 'import * as ns from "mod"', 'import {a} from "mod"', 'import d from "mod"', or another module format instead.
index.ts(2,1): error TS1294: This syntax is not allowed when 'erasableSyntaxOnly' is enabled.
index.ts(3,1): error TS1202: Import assignment cannot be used when targeting ECMAScript modules. Consider using 'import * as ns from "mod"', 'import {a} from "mod"', 'import d from "mod"', or another module format instead.
Expand All @@ -23,12 +22,8 @@ index.ts(3,1): error TS1294: This syntax is not allowed when 'erasableSyntaxOnly
a: 1,
}

==== foo.js (1 errors) ====
==== foo.js (0 errors) ====
module.exports = {
~~~~~~~~~~~~~~~~~~
b: 2,
~~~~~~~~~
}
~
!!! error TS1203: Export assignment cannot be used when targeting ECMAScript modules. Consider using 'export default' or another module format instead.

13 changes: 2 additions & 11 deletions testdata/baselines/reference/compiler/nestedJSDocImportType.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,25 +1,16 @@
b.js(1,1): error TS1203: Export assignment cannot be used when targeting ECMAScript modules. Consider using 'export default' or another module format instead.
b.js(3,24): error TS2306: File 'a.js' is not a module.


==== a.js (0 errors) ====
/** @typedef {string} A */

==== b.js (2 errors) ====
==== b.js (1 errors) ====
module.exports = {
~~~~~~~~~~~~~~~~~~
create() {
~~~~~~~~~~~~
/** @param {import("./a").A} x */
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
~~~~~
!!! error TS2306: File 'a.js' is not a module.
function f(x) {}
~~~~~~~~~~~~~~~~~~~~
return f("hi");
~~~~~~~~~~~~~~~~~~~
}
~~~
}
~
!!! error TS1203: Export assignment cannot be used when targeting ECMAScript modules. Consider using 'export default' or another module format instead.
}
14 changes: 0 additions & 14 deletions testdata/baselines/reference/conformance/typedefModuleExportsIndirect1.errors.txt
View file
Open in desktop

This file was deleted.

29 changes: 29 additions & 0 deletions testdata/baselines/reference/conformance/typedefModuleExportsIndirect1.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,32 @@ type C = import('./typedefModuleExportsIndirect1').C;
/** @typedef {import('./typedefModuleExportsIndirect1').C} C */
/** @type {C} */
declare var c: C;


//// [DtsFileErrors]


dist/typedefModuleExportsIndirect1.d.ts(5,1): error TS2309: An export assignment cannot be used in a module with other exported elements.
dist/typedefModuleExportsIndirect1.d.ts(5,10): error TS2304: Cannot find name 'dummy'.
dist/use.d.ts(1,52): error TS2694: Namespace 'unknown' has no exported member 'C'.


==== dist/typedefModuleExportsIndirect1.d.ts (2 errors) ====
export type C = {
a: 1;
m: 1;
};
export = dummy;
~~~~~~~~~~~~~~~
!!! error TS2309: An export assignment cannot be used in a module with other exported elements.
~~~~~
!!! error TS2304: Cannot find name 'dummy'.

==== dist/use.d.ts (1 errors) ====
type C = import('./typedefModuleExportsIndirect1').C;
~
!!! error TS2694: Namespace 'unknown' has no exported member 'C'.
/** @typedef {import('./typedefModuleExportsIndirect1').C} C */
/** @type {C} */
declare var c: C;

14 changes: 0 additions & 14 deletions testdata/baselines/reference/conformance/typedefModuleExportsIndirect2.errors.txt
View file
Open in desktop

This file was deleted.

29 changes: 29 additions & 0 deletions testdata/baselines/reference/conformance/typedefModuleExportsIndirect2.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,32 @@ type C = import('./typedefModuleExportsIndirect2').C;
/** @typedef {import('./typedefModuleExportsIndirect2').C} C */
/** @type {C} */
declare var c: C;


//// [DtsFileErrors]


dist/typedefModuleExportsIndirect2.d.ts(5,1): error TS2309: An export assignment cannot be used in a module with other exported elements.
dist/typedefModuleExportsIndirect2.d.ts(5,10): error TS2304: Cannot find name 'f'.
dist/use.d.ts(1,52): error TS2694: Namespace 'unknown' has no exported member 'C'.


==== dist/typedefModuleExportsIndirect2.d.ts (2 errors) ====
export type C = {
a: 1;
m: 1;
};
export = f;
~~~~~~~~~~~
!!! error TS2309: An export assignment cannot be used in a module with other exported elements.
~
!!! error TS2304: Cannot find name 'f'.

==== dist/use.d.ts (1 errors) ====
type C = import('./typedefModuleExportsIndirect2').C;
~
!!! error TS2694: Namespace 'unknown' has no exported member 'C'.
/** @typedef {import('./typedefModuleExportsIndirect2').C} C */
/** @type {C} */
declare var c: C;

14 changes: 0 additions & 14 deletions testdata/baselines/reference/conformance/typedefModuleExportsIndirect3.errors.txt
View file
Open in desktop

This file was deleted.

29 changes: 29 additions & 0 deletions testdata/baselines/reference/conformance/typedefModuleExportsIndirect3.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,32 @@ type C = import('./typedefModuleExportsIndirect3').C;
/** @typedef {import('./typedefModuleExportsIndirect3').C} C */
/** @type {C} */
declare var c: C;


//// [DtsFileErrors]


dist/typedefModuleExportsIndirect3.d.ts(5,1): error TS2309: An export assignment cannot be used in a module with other exported elements.
dist/typedefModuleExportsIndirect3.d.ts(5,10): error TS2304: Cannot find name 'o'.
dist/use.d.ts(1,52): error TS2694: Namespace 'unknown' has no exported member 'C'.


==== dist/typedefModuleExportsIndirect3.d.ts (2 errors) ====
export type C = {
a: 1;
m: 1;
};
export = o;
~~~~~~~~~~~
!!! error TS2309: An export assignment cannot be used in a module with other exported elements.
~
!!! error TS2304: Cannot find name 'o'.

==== dist/use.d.ts (1 errors) ====
type C = import('./typedefModuleExportsIndirect3').C;
~
!!! error TS2694: Namespace 'unknown' has no exported member 'C'.
/** @typedef {import('./typedefModuleExportsIndirect3').C} C */
/** @type {C} */
declare var c: C;

34 changes: 0 additions & 34 deletions testdata/baselines/reference/submodule/compiler/amdLikeInputDeclarationEmit.errors.txt
View file
Open in desktop

This file was deleted.

38 changes: 0 additions & 38 deletions testdata/baselines/reference/submodule/compiler/amdLikeInputDeclarationEmit.errors.txt.diff
View file
Open in desktop

This file was deleted.

Loading
Loading