DX-1205: throw on legacy v1 ably/promises and ably/callbacks imports#2227
DX-1205: throw on legacy v1 ably/promises and ably/callbacks imports#2227umair-ably wants to merge 6 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR replaces the deprecated v1 ChangesLegacy Entry Point Deprecation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Add subpath exports for the two v1 entry points (callbacks.js and promises.js at the package root, both removed in mid-2023 during v2 prep) so that code written against v1 — or generated by an LLM trained on v1 docs — fails on import with an actionable migration message instead of Node's generic ERR_PACKAGE_PATH_NOT_EXPORTED. The .js shims throw at module load; the .d.ts siblings keep `@arethetypeswrong/cli` happy across node10/node16/bundler resolution modes, carry an `@deprecated` tag for IDE hovers, and resolve to `never` so any property access errors at compile time. URL points at the in-repo migration guide, matching the convention from DX-1204 (src/common/lib/util/utils.ts:293). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
70e5cff to
4d23cde
Compare
Aligns with the DX-1204 / DX-1209 convention that ErrorInfo throws separate *what failed* (message) from *how to fix it* (hint). The shims still throw plain Error rather than ErrorInfo — pulling in ErrorInfo would mean require'ing the built bundle just to throw, which defeats the point of a top-level alias shim. - promises.js / callbacks.js: stamp .hint on the thrown Error. - Tests assert on err.message + err.hint separately (drift-pin against silent rephrasing). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
40e33ba to
71ea4f3
Compare
Summary
./promisesand./callbackssubpath exports topackage.json, each pointing at a tiny.jsshim that throws on module load. The throw is a structuredErrorwhosemessagenames the dropped v1 entry point and whosehintdirects the user at the v2 import and the migration guide.message(what failed) /hint(how to fix) split matches the convention DX-1204 / DX-1209 use forErrorInfothrows elsewhere in the SDK.ERR_PACKAGE_PATH_NOT_EXPORTED, which gives an agent or migrating user no signal that the path was renamed. The shim turns that into an actionable error naming the new entry point.Background — were these real?
Yes. Both were genuine v1 entry points at the package root, removed as separate commits during v2 prep:
ably/callbacks—callbacks.jsremoved in2a2ed49e("Remove the callbacks public API", Jun 2023). The v1 file re-exported./build/ably-node.ably/promises—promises.jsremoved in6cf248fb("Remove promises.js / .d.ts", Jun 2023). The v1 file wrapped the constructors to setoptions.internal.promises = true.docs/migration-guides/v2/lib.md:68-70already documents both as v1 spellings users would have written.Implementation notes
.jsfile per subpath: works for bothrequireandimportbecause the package is not"type": "module". Module-evaluationthrowpropagates synchronously out of both.promises.js,callbacks.js) alongside their.d.tssiblings — hand-written, kept separate frombuild/so they aren't wiped bygrunt build. Added tofiles[]individually so they ship in the npm package.Errorwitherr.hintstamped on, rather than anErrorInfo. Pulling inErrorInfowould meanrequireing the built bundle just to throw out of an alias path, defeating the point of a top-level shim. Theerr.message+err.hintshape is what LLM-eval tooling already inspects everywhere else, so the structural alignment withErrorInfo-based throws is preserved without the runtime cost..d.tsfiles (promises.d.ts,callbacks.d.ts) declare each subpath as@deprecated neverso TypeScript users get a strikethrough at the import site and an IDE-rendered migration message before the runtime throw ever fires.https://github.com/ably/ably-js/blob/main/docs/migration-guides/v2/lib.md), matching the convention introduced by DX-1204 insrc/common/lib/util/utils.ts:293.Base branch
DX-1204/v1-callback-overloads, notmain. Merge DX-1204 first; this branch will then auto-retarget tomainor can be rebased.Test plan
npx mocha test/unit/legacy-import-shims.test.js→ 4 passing:ably/promisesshim throws naming the v1 entry point, with a hint pointing at the migration guideably/callbacksshim throws naming the v1 callback API, with a hint pointing at the migration guidepackage.jsonexportsmap wires the legacy subpaths to the shim files and their typesfiles[]ships the legacy shim.js/.d.tsfiles in the published packageexportsresolution (e.g.npm pack+require('ably/promises')from a temp project) — optional, the unit-level wiring test already covers the mapping.🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
Tests