Skip to content

Improve CommonJS support#2893

Open
ahejlsberg wants to merge 2 commits intomainfrom
cjs-module-exports
Open

Improve CommonJS support#2893
ahejlsberg wants to merge 2 commits intomainfrom
cjs-module-exports

Conversation

@ahejlsberg
Copy link
Member

In this PR:

  • Permit multiple module.exports assignments in a file and union the assigned types.
  • Make JSDoc @typedef declarations available on imports of CJS modules that use module.exports.
  • Exclude module.exports assignments from stricter checking of TypeScript export = declarations.

These changes align Corsa more closely with the current behavior in Strada and eliminate a large number of baseline differences.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR improves CommonJS support in the TypeScript compiler port by making three key changes:

  1. Permitting multiple module.exports assignments and unioning their types
  2. Making JSDoc @typedef declarations available on imports of CJS modules with module.exports
  3. Excluding module.exports assignments from stricter TypeScript export = checking

These changes align Corsa more closely with Strada's behavior and eliminate numerous baseline differences.

Changes:

  • Added mergedExportEquals field to ModuleSymbolLinks to track merged export types for CJS modules
  • Updated test baselines to reflect removal of TS1203 errors for module.exports assignments
  • Improved type resolution for CJS modules with multiple exports and typedefs

Reviewed changes

Copilot reviewed 295 out of 295 changed files in this pull request and generated no comments.

Show a summary per file
File Description
internal/checker/types.go Added mergedExportEquals field to support merging multiple CJS exports
testdata/baselines/reference/submodule/**/*.diff Removed TS1203 "Export assignment cannot be used when targeting ECMAScript modules" errors for module.exports
testdata/baselines/reference/submodule/**/*.types Updated type information to show proper CJS module export types
testdata/baselines/reference/submodule/**/*.symbols Updated symbol resolution for CJS exports
testdata/baselines/reference/conformance/**/*.errors.txt Removed erroneous TS1203 errors from conformance tests

if links.mergedExportEquals != nil {
return links.mergedExportEquals
}
merged := c.cloneSymbol(exportEquals)
Copy link
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.

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
Member

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
Member

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
Member Author

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
Member Author

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
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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants