Repository navigation
fix(sdk): give CommonJS consumers their own type declarations - #792
Conversation
The package is "type": "module" and exports["."] shared one types entry (dist/index.d.ts) between import and require, which TypeScript reads as ESM. A CommonJS project under node16/nodenext resolution got TS1479 on import even though require() works. Give import and require their own types and default, pointing require at the dist/index.d.cts the build already emits. smoke-ts-dist now compiles a CommonJS and an ESM consumer against the built package with tsc --module node16, offline, and the exports fields are checked against the files. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wjv2nu841pxTMdUWSAPmDH
The development guide, the Makefile comments and help text still described build-ts as only smoke-loading the entry points. The SDK page's CommonJS section now notes the matching .d.cts declarations. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wjv2nu841pxTMdUWSAPmDH
The badge filtered out only a tag named exactly "client", so the SDK release clients/ts/v0.1.1 won and the badge read that instead of v0.1.0. Select the server tags (v*) instead of excluding one name. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wjv2nu841pxTMdUWSAPmDH
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (8)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🧰 Additional context used📚 Code guidelines (4)📓 Path-based instructions (5)Source excerpt: Pick between the in-repo Docker Compose, pulling a container image, or `go install`.📄 CodeRabbit inference engine (README.md) Files:
Source excerpt: **Editors see WH001 in `.md` only.**📄 CodeRabbit inference engine (AGENTS.md) Files:
Source excerpt: **Docs prose**: never hard-wrap Markdown — one paragraph is one line.📄 CodeRabbit inference engine (CONTRIBUTING.md) Files:
Source excerpt: Run `make fix` to apply them.📄 CodeRabbit inference engine (docs/src/content/docs/development.md) Files:
Source excerpt: **WH001 applies to every tracked Markdown file, with no carve-out** — `AGENTS.md`, `CHANGELOG.md`, `.github/` CI docs and `.claude/` agent prompts included.📄 CodeRabbit inference engine (AGENTS.md) Files:
🔇 Additional comments (1)
📝 SummarySummary by CodeRabbit
WalkthroughThe SDK package now provides separate runtime and declaration targets for ESM and CommonJS consumers. Its smoke check validates export targets and type-checks both consumer formats with Node16 module resolution. The README release badge now filters tags matching ChangesSDK TypeScript declarations
Release badge filter
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable issue is established; the change is mergeable after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)✅ Passed checks (4 passed)Full details: Out of Scope Changes checkExplanation The
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
📚 Docs preview is live → https://ada68284-wavehouse-docs.wave-rf.workers.dev
|
Code Coverage OverviewLanguages: Go GoThe overall line coverage in commit fa8705a in the Show a line coverage summary of the most impacted files.
Updated |
|
@coderabbitai review |
|
Brings in #786 and the PRs merged since. CHANGELOG.md conflicted only because two new Fixed entries landed at the same spot; both are kept. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wjv2nu841pxTMdUWSAPmDH
|
@coderabbitai review |
✅ Action performedReview finished.
|
Both PRs added their Fixed entries at the top of the section after merging main, so whichever merged second would conflict. These now sit below the TestClaims entry from main. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wjv2nu841pxTMdUWSAPmDH
Summary
A TypeScript project compiled as CommonJS (
"type": "commonjs",moduleResolutionnode16/nodenext) got TS1479 importing@wavehouse/sdk, thoughrequire()worked at runtime. The package is"type": "module"andexports["."]gaveimportandrequireone sharedtypesentry,dist/index.d.ts, which TypeScript reads as ESM.exports["."]now has per-condition typings:importcarriesindex.d.tsandindex.js,requirecarriesindex.d.cts(already emitted by the build, previously unreferenced) andindex.cjs. Top-levelmain,moduleandtypesare unchanged. The codegen bin and the IIFE bundle ship no declarations, so they need nothing.make smoke-ts-dist(run bymake build-ts) compiles a CommonJS and an ESM consumer against the built package withtsc --module node16, offline, from a scratch project that resolves the package by name. It also checks theexportstypings and defaults against the top-level fields and the files on disk..d.ctsdeclarations, and the development guide,make helpand the SDK README describe the new type check.filter=!clientexcluded only a tag named exactlyclient, so the newest SDK release won and the badge readrelease: clients/ts/v0.1.1. It now selects the server tags withfilter=v*and readsrelease: v0.1.0(both rendered against shields.io).Test plan
package.jsonfails withTS1479for the CommonJS consumer only; the ESM consumer compiles.require()andimportstill resolve through theexportsmap and expose the same 12 names.make build-tsgreen (build, then the smoke at the oldest supported Node, v22.0.0).make verifygreen.Related Issues
Closes #751. Part of #740.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Wjv2nu841pxTMdUWSAPmDH