Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves CommonJS support in the TypeScript compiler port by making three key changes:
- Permitting multiple
module.exportsassignments and unioning their types - Making JSDoc
@typedefdeclarations available on imports of CJS modules withmodule.exports - Excluding
module.exportsassignments from stricter TypeScriptexport =checking
These changes align Corsa more closely with Strada's behavior and eliminate numerous baseline differences.
Changes:
- Added
mergedExportEqualsfield toModuleSymbolLinksto track merged export types for CJS modules - Updated test baselines to reflect removal of TS1203 errors for
module.exportsassignments - 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) |
There was a problem hiding this comment.
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 = _exportswhich, 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;There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I wish we would just make that first example legal TS already...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This is another interesting input:
const a = require("./mod")
/**
* @typedef {string | number} Foo
*/
module.exports = aif 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] = 12ie, 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 = _exportsor
import * as mod from "./mod"
export type Foo = number | string
export = modand 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.
In this PR:
module.exportsassignments in a file and union the assigned types.@typedefdeclarations available on imports of CJS modules that usemodule.exports.module.exportsassignments from stricter checking of TypeScriptexport =declarations.These changes align Corsa more closely with the current behavior in Strada and eliminate a large number of baseline differences.