fix: emit CommonJS declarations as .d.cts and point require.types at them - #433
Conversation
|
Plan
Verification notes
Adversarial review (two Codex passes, one spec-aware and one spec-blind) Acted on:
Dismissed:
CI annotation Verification tooling itself lands in #427 ( Status 2026-09-08 Head Remaining CI annotations dismissed, none of them from this branch:
Still waiting on the Rebased onto The CI on Copilot review skipped: it is not installed as a reviewer on this repo. Copilot did review after all, on Gates 1-5 hold apart from the Status 2026-09-09 — head Rebased onto The rebase crossed a substantially rewritten Answered @ianwremmel's thread on the script header: Two more Codex passes on the rebased diff, one spec-aware and one spec-blind. Acted on:
Dismissed:
Smoke-tested the converter standalone after the comment edits: reference paths, Gates 2-5 hold. Waiting on gate 1 (this CI run) and @ianwremmel's approval. Status 2026-09-09 Rebased onto CI annotations on this head are all notice-level deploy/teardown progress lines from the passing shards, plus the recurring Node.js 20 deprecation warning for third-party actions. Dismissed: no workflow file is in this diff, and the shards succeeded. Gates 1-5 hold. Waiting on @ianwremmel's approval. |
|
View your CI Pipeline Execution ↗ for commit a66aa2e
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #433 +/- ##
=======================================
Coverage 60.64% 60.64%
=======================================
Files 28 28
Lines 1601 1601
Branches 354 363 +9
=======================================
Hits 971 971
Misses 626 626
Partials 4 4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
CI on 937e5c0: everything green except all five This is not from this PR. Run 34163408910 on Root cause: Two separate problems: the splitter returning nothing, and Green here: @ianwremmel — how do you want to handle |
|
Resolving my own open question: no answer needed. Proceeding to rebase onto |
937e5c0 to
398e2a2
Compare
|
Out of draft and ready for your review, @ianwremmel. What changed: Verified against packed tarballs with a synthetic version injected: Two things to know before you approve:
Reply |
There was a problem hiding this comment.
🟢 Approval recommended
The changes consistently update the build pipeline and package export metadata to emit and reference .d.cts CommonJS declarations without introducing unresolved references.
Pull request overview
This PR fixes “Masquerading as ESM” type-declaration issues across the monorepo by emitting a dedicated CommonJS declaration tree (.d.cts) and updating exports["."].require.types to point at it, while keeping runtime require()/import() behavior unchanged.
Changes:
- Add
scripts/dts-to-dcts.mjsto convertdist/types→dist/cjs-typeswith.d.ctsfilenames and rewritten relative specifiers/source maps. - Replace the old
scripts/dmts-to-dtsflow and update the Nxbuild:typescommand to always generatedist/cjs-types. - Update package
exports.require.typesentries to reference./dist/cjs-types/index.d.cts.
File summaries
| File | Description |
|---|---|
| scripts/dts-to-dcts.mjs | New converter script producing .d.cts CJS declarations and fixing relative specifiers + source maps. |
| scripts/dmts-to-dts | Removed legacy bash-based conversion that stripped extensions / broke sourcemap URLs. |
| packages/@code-like-a-carpenter/workbench-config/package.json | Point exports.require.types to index.d.cts. |
| packages/@code-like-a-carpenter/wait-for/package.json | Point exports.require.types to index.d.cts. |
| packages/@code-like-a-carpenter/tooling-common/package.json | Point exports.require.types to index.d.cts. |
| packages/@code-like-a-carpenter/tool-tool/package.json | Point exports.require.types to index.d.cts. |
| packages/@code-like-a-carpenter/tool-stack/package.json | Point exports.require.types to index.d.cts. |
| packages/@code-like-a-carpenter/tool-json-schema/package.json | Point exports.require.types to index.d.cts. |
| packages/@code-like-a-carpenter/tool-inliner/package.json | Point exports.require.types to index.d.cts. |
| packages/@code-like-a-carpenter/tool-foundation/package.json | Point exports.require.types to index.d.cts. |
| packages/@code-like-a-carpenter/tool-deps/package.json | Point exports.require.types to index.d.cts. |
| packages/@code-like-a-carpenter/telemetry/package.json | Point exports.require.types to index.d.cts. |
| packages/@code-like-a-carpenter/sentry/package.json | Point exports.require.types to index.d.cts. |
| packages/@code-like-a-carpenter/parallel/package.json | Point exports.require.types to index.d.cts. |
| packages/@code-like-a-carpenter/logger/package.json | Point exports.require.types to index.d.cts. |
| packages/@code-like-a-carpenter/lambda-handlers/package.json | Point exports.require.types to index.d.cts. |
| packages/@code-like-a-carpenter/interact/package.json | Point exports.require.types to index.d.cts. |
| packages/@code-like-a-carpenter/graphql-codegen-helpers/package.json | Point exports.require.types to index.d.cts. |
| packages/@code-like-a-carpenter/foundation-transform-environment/package.json | Point exports.require.types to index.d.cts. |
| packages/@code-like-a-carpenter/foundation-runtime/package.json | Point exports.require.types to index.d.cts. |
| packages/@code-like-a-carpenter/foundation-plugin-typescript/package.json | Point exports.require.types to index.d.cts. |
| packages/@code-like-a-carpenter/foundation-plugin-cloudformation/package.json | Point exports.require.types to index.d.cts. |
| packages/@code-like-a-carpenter/foundation-parser/package.json | Point exports.require.types to index.d.cts. |
| packages/@code-like-a-carpenter/foundation-intermediate-representation/package.json | Point exports.require.types to index.d.cts. |
| packages/@code-like-a-carpenter/exception/package.json | Point exports.require.types to index.d.cts. |
| packages/@code-like-a-carpenter/errors/package.json | Point exports.require.types to index.d.cts. |
| packages/@code-like-a-carpenter/env/package.json | Point exports.require.types to index.d.cts. |
| packages/@code-like-a-carpenter/dotenv/package.json | Point exports.require.types to index.d.cts. |
| packages/@code-like-a-carpenter/contract-tests/package.json | Point exports.require.types to index.d.cts. |
| packages/@code-like-a-carpenter/cli/package.json | Point exports.require.types to index.d.cts. |
| packages/@code-like-a-carpenter/cli-plugin-example/package.json | Point exports.require.types to index.d.cts. |
| packages/@code-like-a-carpenter/cli-core/package.json | Point exports.require.types to index.d.cts. |
| packages/@code-like-a-carpenter/aws-env-loader/package.json | Point exports.require.types to index.d.cts. |
| packages/@code-like-a-carpenter/assert/package.json | Point exports.require.types to index.d.cts. |
| packages/@clc/test-helpers/package.json | Point exports.require.types to index.d.cts. |
| packages/@clc/dependencies/package.json | Point exports.require.types to index.d.cts. |
| packages/@clc/nx/src/create-nodes.mjs | Always run dts-to-dcts after tsc for build:types and widen source input globs to include m/c variants. |
| packages/@clc/nx/executors/package-json/executor.mjs | Generate exports.require.types as index.d.cts and document why. |
Review details
- Files reviewed: 38/38 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Correction to my engagement comment: Copilot did review after all, and it |
|
Merge-order correction. My earlier comment pointed at #437 for the red Nothing here changed. This branch is still at Copilot reviewed at this head: approval recommended, 38/38 files, 0 comments. So this needs two things from you: approve here, and merge #436 before this so |
|
@ianwremmel this PR is ready and needs your input to proceed:
Every other check (build, lint, diff, TSC, test-unit, codecov, Socket Security) is green. No other blockers on this PR. |
| /** | ||
| * Produces `dist/cjs-types` from `dist/types`. | ||
| * | ||
| * Every package is `"type": "module"`, so a `.d.ts` under it is read as ESM no |
There was a problem hiding this comment.
Is the problem here more that everything is type:module? by your logic here, it sounds like our commonjs exports serve no purpose
There was a problem hiding this comment.
Two questions in there, and I think only the second one is really open.
type: module is not the cause. TypeScript decides a declaration's module
format the same way Node decides a .js file's: the file extension if it is
explicit, otherwise the nearest package.json "type". exports picks which
file gets loaded; it has no say in how that file is parsed. So under either
"type" one of the two halves has to carry a disambiguating extension:
"type": "module"— the ESM half is fine as.d.ts, the CommonJS half needs
.d.cts."type": "commonjs"— mirror image: the CommonJS half is fine as.d.ts, the
ESM half needs.d.mts.
Flipping type moves the problem rather than removing it, and it would be the
wrong way round, since the sources are ESM. The runtime half of exports
already gets this right — index.mjs and index.cjs, both explicit. The
declarations were the one place the extension got dropped: dmts-to-dts
renamed .d.mts to .d.ts when it should have renamed it to .d.cts.
"Our CommonJS exports serve no purpose" — partly right, and worth deciding
deliberately rather than by accident.
Two things are easy to conflate here:
dist/cjs itself is load-bearing.
packages/@code-like-a-carpenter/tool-foundation/executors.json points Nx at
./dist/cjs/__generated__/foundation-executor.cjs, so that executor runs only
because the CommonJS build exists. But Nx loads it by path — it never goes
through exports.
The exports["."].require condition is the part with no in-repo consumer. I
grepped for one and came up empty; every require() in the tree is relative.
Its only audience is external CommonJS consumers of the published packages, and
these are published (@code-like-a-carpenter/assert is on npm at 2.4.0,
logger at 2.5.0).
So the real question is whether you still want to serve those consumers. If the
answer is no, the honest change is bigger than this PR: drop the require
condition, drop build:cjs for everything except tool-foundation, and delete
this script along with it. engines.node is 22.x and require(esm) has been
unflagged since 22.12, so that is a defensible call — but it is a
publishing-compatibility decision, not a build bug, and I don't want to make it
silently inside a PR scoped to "stop the declarations lying about their format".
Say the word and I'll file it as its own ticket. Either way this PR is the right
intermediate step: it makes the require half tell the truth, and if you later
delete the condition you delete this script with it, so nothing here becomes
sunk cost.
Done.
Every package is `"type": "module"`, so a `.d.ts` beneath one is read as
ESM whatever `exports` says. `exports["."].require.types` pointed CommonJS
consumers at those ESM declarations: attw reported "Masquerading as ESM"
for all 32 published packages, and a `moduleResolution: node16` CommonJS
consumer got TS1479 against a packed tarball.
scripts/dts-to-dcts.mjs replaces scripts/dmts-to-dts. It writes
dist/cjs-types/**/*.d.cts and rewrites relative specifiers to `.cjs`
(`./x.mjs` and `./x.ts` alike), where the old script stripped the
extension outright — `sed -e "s#.mjs#${EXTENSION}#g"` with EXTENSION
unset turned `export * from "./assert.mjs"` into `"./assert"`, which is
what attw reported as an internal resolution error. The same substitution
mangled `//# sourceMappingURL=index.d.mts.map` into `index.d.map`; the
new script rewrites sourceMappingURL and each map's `file` field, so all
230 declaration maps now resolve.
build:types runs the conversion for every package, not only those whose
entry point is `src/index.mjs` or `src/index.mts`, so the 22 `.ts`-source
packages get CommonJS declarations for the first time.
The converter fails rather than emitting output that cannot resolve: an
extensionless or directory specifier has no working rewrite under node16
CommonJS resolution, a name collision between `x.d.ts` and `x.d.mts` is
reported, and an empty `dist/types` is an error because the require
condition names a file that would not exist.
Two build:types inputs were wrong and are corrected here because they
decide whether these declarations get regenerated: the source glob
`src/**/*.[jt]s?(x)` matched no `.mjs` or `.mts` file, and the converter
itself was not an input.
34 manifests change, the 32 published packages plus the two private
`@clc` ones the same executor generates. Only `require.types` moves; the
`import` condition and the top-level `types` field are untouched.
Verified against packed tarballs with a synthetic version: attw reports
no problems for all 32 packages, publint --strict reports no
`require.types` error (cli-core and cli-plugin-example still fail on
`pkg.bin`, which is CLC-1131), and a `"type": "commonjs"` /
`moduleResolution: node16` fixture type-checks against tarballs of
@code-like-a-carpenter/assert (.mjs source) and
@code-like-a-carpenter/exception (.ts source).
Refs CLC-1130
398e2a2 to
a66aa2e
Compare
|
The precondition from my last comment is gone: #436 merged, this branch is rebased onto That leaves only your approval. The one open question is on the |
All 32 packages under
packages/@code-like-a-carpenter/*failattwwith "Masquerading as ESM" and failpublintwith the matching error. Every package is"type": "module", so any.d.tsbeneath it is read as ESM, and both shapes ofexports["."].require.typespoint at a.d.ts.Two causes:
.ts-source packages have no CJS declarations at all —build:typesrunsscripts/dmts-to-dtsonly forsrc/index.mjs/src/index.mtsentry points, so theirrequire.typespoints at the same./dist/types/index.d.tsasimport.types..mjs-source packages do producedist/cjs-types, but.d.tsundertype: moduleis still ESM, anddmts-to-dtsstrips extensions (sed -e "s#.mjs#${EXTENSION}#g"withEXTENSIONunset) instead of rewriting them, which also manglessourceMappingURLso every declaration map dangles.This changes declaration emit only; runtime
require()/import()already work for all 32 packages.Ticket: https://linear.app/code-like-a-carpenter/issue/CLC-1130/emit-commonjs-declarations-as-dcts-and-point-the-require-condition-at