[DX-1204] Mark v1 callback API as deprecated#2226
Conversation
umair-ably
force-pushed
the
DX-1204/v1-callback-overloads
branch
from
1d3cdf7 to
e149a7f
Compare
github-actions
Bot
temporarily deployed
to
staging/pull/2226/bundle-report
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ably.d.ts`:
- Around line 1993-2008: The deprecated authorize overload is missing
parameter-level JSDoc which triggers Typedoc warnings; add `@param` entries for
tokenParams, authOptions, and callback inside the authorize overload's comment
block (the deprecated v1 variant) so Typedoc has docs for each parameter and the
warning is resolved—repeat the same addition for the other deprecated Auth
overload blocks referenced (around the authorize overloads at the other ranges).
- Around line 2639-2647: The deprecation note incorrectly tells users to "await
the returned promise" for RealtimeChannel.unsubscribe; update the comment for
RealtimeChannel.unsubscribe to reflect that the v2 API is synchronous (returns
void) — remove any wording about awaiting a promise and adjust the example to
show simply calling channel.unsubscribe('event', listener) (no await), while
keeping the migration guide link if desired; ensure the docstring and example
align with the declared signature of RealtimeChannel.unsubscribe.
In `@src/common/lib/client/auth.ts`:
- Around line 257-260: The v1 callback detection currently passes
v1CallbackPosition=2 to Utils.detectV1Callback in Auth.authorize (and the
similar spots), so calls like authorize(callback) or authorize(tokenParams,
callback) are not recognized as v1-style and misrouted; update the
detectV1Callback invocation(s) to use v1CallbackPosition=1 (i.e., change the
third numeric arg from 2 to 1) for Auth.authorize (and the same spot(s) that
mirror it) so the helper recognizes a callback at index 0 or 1 and routes those
calls correctly.
In `@src/common/lib/util/utils.ts`:
- Around line 301-304: The remediation message in detectV1Callback incorrectly
always tells users to "await" a returned promise; update detectV1Callback to
emit a context-aware hint: if the guarded API is known to return a promise (e.g.
async methods), keep the current "Remove the trailing callback and `await` the
returned promise — e.g. `await ${exampleCall}`" message, but for non-promise
APIs (such as unsubscribe) emit a generic "Remove the trailing callback and call
the method without a callback — e.g. `${exampleCall}`" message instead;
implement this by adding a boolean parameter or internal check in
detectV1Callback to determine promise-returning methods and branch the
ErrorInfo.hint accordingly, referencing methodName and exampleCall when building
the hint.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0abf3211-8d3c-4401-8204-72cd70458312
📒 Files selected for processing (12)
ably.d.tssrc/common/lib/client/auth.tssrc/common/lib/client/connection.tssrc/common/lib/client/realtimechannel.tssrc/common/lib/client/realtimepresence.tssrc/common/lib/types/errorinfo.tssrc/common/lib/util/utils.tssrc/platform/react-hooks/src/hooks/useChannel.tstest/realtime/auth.test.jstest/realtime/channel.test.jstest/realtime/connection.test.jstest/realtime/presence.test.js
umair-ably
force-pushed
the
DX-1204/v1-callback-overloads
branch
from
e149a7f to
7742481
Compare
github-actions
Bot
temporarily deployed
to
staging/pull/2226/bundle-report
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR synchronously rejects legacy v1 trailing callbacks across Auth, Connection, RealtimeChannel, and RealtimePresence using a new detectV1Callback helper that throws ErrorInfo with an actionable hint; TypeScript declarations add deprecated callback overloads and tests validate synchronous throws. ChangesV1 Callback Deprecation with Error Hints
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ably.d.ts (1)
1993-2053:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrettier check is still failing for this file (merge blocker).
Lint reports
ably.d.tsfailsprettier --check; please run Prettier on this file and commit the formatting-only diff so CI can pass.Also applies to: 2139-2270, 2642-2656, 2689-2702, 2743-2757, 2783-2797, 3488-3500, 3911-3918
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ably.d.ts` around lines 1993 - 2053, Prettier formatting is failing for ably.d.ts; run Prettier (or your repo's format script) on ably.d.ts and commit the resulting formatting-only changes so CI passes. Focus on normalizing spacing/line breaks around the Auth methods shown (authorize, createTokenRequest, requestToken and their deprecated overloads) and the adjacent JSDoc blocks; ensure the file matches the project's Prettier configuration and only contains whitespace/formatting edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/common/lib/client/realtimechannel.ts`:
- Around line 464-467: The two-argument v1 callback shape (subscribe(listener,
callback) and unsubscribe(listener, callback)) is not being detected because
Utils.detectV1Callback(args, 2) only checks the third parameter; update the
detection to check the second parameter instead by calling
Utils.detectV1Callback(args, 1) in both subscribe and unsubscribe before
delegating to _subscribeImpl/_unsubscribeImpl so the trailing callback is caught
and the synchronous migration error is raised.
In `@src/common/lib/client/realtimepresence.ts`:
- Around line 435-437: The subscribe method is incorrectly calling
Utils.detectV1Callback(_args, 2) which skips detecting the v1 two-argument
callback form (both args functions), causing the trailing callback to be
ignored; update the call in subscribe to use the correct max-arguments value
(change the second parameter from 2 to 1) so Utils.detectV1Callback correctly
recognizes the v1 (listener, callback) shape and then call
this._subscribeImpl(_args) as before.
---
Outside diff comments:
In `@ably.d.ts`:
- Around line 1993-2053: Prettier formatting is failing for ably.d.ts; run
Prettier (or your repo's format script) on ably.d.ts and commit the resulting
formatting-only changes so CI passes. Focus on normalizing spacing/line breaks
around the Auth methods shown (authorize, createTokenRequest, requestToken and
their deprecated overloads) and the adjacent JSDoc blocks; ensure the file
matches the project's Prettier configuration and only contains
whitespace/formatting edits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 809a918d-5675-4134-96a4-410c0b316c93
📒 Files selected for processing (13)
ably.d.tsscripts/moduleReport.tssrc/common/lib/client/auth.tssrc/common/lib/client/connection.tssrc/common/lib/client/realtimechannel.tssrc/common/lib/client/realtimepresence.tssrc/common/lib/types/errorinfo.tssrc/common/lib/util/utils.tssrc/platform/react-hooks/src/hooks/useChannel.tstest/realtime/auth.test.jstest/realtime/channel.test.jstest/realtime/connection.test.jstest/realtime/presence.test.js
umair-ably
force-pushed
the
DX-1204/v1-callback-overloads
branch
from
7742481 to
c52b7e6
Compare
github-actions
Bot
temporarily deployed
to
staging/pull/2226/bundle-report
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/realtime/auth.test.js (1)
1657-1659: ⚡ Quick winDisable autoconnect for these sync-throw tests.
These cases only validate argument-shape rejection; opening a realtime connection adds avoidable async noise.
♻️ Suggested change
- var helper = this.test.helper, - realtime = helper.AblyRealtime(); + var helper = this.test.helper, + realtime = helper.AblyRealtime({ autoConnect: false });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/realtime/auth.test.js` around lines 1657 - 1659, The tests call helper.AblyRealtime() which opens a realtime connection and adds async noise; change the instantiation to disable autoconnect by calling helper.AblyRealtime({ autoConnect: false }) (or the equivalent option your helper uses) so the tests validate argument-shape rejection synchronously without opening a connection; update any other occurrences in these "sync-throw" tests that call AblyRealtime() to use the same { autoConnect: false } option.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/realtime/auth.test.js`:
- Around line 1657-1659: The tests call helper.AblyRealtime() which opens a
realtime connection and adds async noise; change the instantiation to disable
autoconnect by calling helper.AblyRealtime({ autoConnect: false }) (or the
equivalent option your helper uses) so the tests validate argument-shape
rejection synchronously without opening a connection; update any other
occurrences in these "sync-throw" tests that call AblyRealtime() to use the same
{ autoConnect: false } option.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 993dce25-2bb1-453d-b036-21d753c22fd9
📒 Files selected for processing (6)
ably.d.tssrc/platform/react-hooks/src/hooks/useChannel.tstest/realtime/auth.test.jstest/realtime/channel.test.jstest/realtime/connection.test.jstest/realtime/presence.test.js
umair-ably
force-pushed
the
DX-1204/v1-callback-overloads
branch
from
c52b7e6 to
90a42de
Compare
github-actions
Bot
temporarily deployed
to
staging/pull/2226/bundle-report
| const n = args.length; | ||
| if (typeof args[n - 1] !== 'function') return; | ||
| if (n <= minV2Args && typeof args[n - 2] !== 'function') return; | ||
| const err = new ErrorInfo('v1 callback signature is no longer supported.', 40000, 400); |
There was a problem hiding this comment.
40000 is a generic and often overused error code, can we have a new one specifically for this and/or a generic "no longer supported" code?
There was a problem hiding this comment.
This was intentional in that I'm aware we have an issue with overloaded error codes. However, I opted against addressing this here because it's not really the intention of this PR, and more importantly - Lewis is undergoing the de-overloading efforts for the server side error codes first.
I'll take guidance from his work there and apply it on a wider scale here in a separate PR.
Happy to be challenged if you think we should just address this specifically for this error now. (I can introduce a 40024, but again Lewis's work might mean he starts using this currently unused code, which is why I'd rather wait)
There was a problem hiding this comment.
I think it's important. By giving it its own error code it follows the precedent we want to follow in the future and gives LLMs another avenue for discovery (the unique error code). If we must stick with existing error codes, then we should use 40003 for invalid parameter value. But I think a standalone code for "no longer supported" is more specific
There was a problem hiding this comment.
fair enough - updated ably-common here
also updated this PR to use the new code
| async authorize(tokenParams: API.TokenParams | null, authOptions: AuthOptions | null): Promise<API.TokenDetails>; | ||
|
|
||
| async authorize( | ||
| authorize(...args: any[]): Promise<API.TokenDetails> { |
There was a problem hiding this comment.
Should we use unknown consistently everywhere? Generally more favourable than any
|
|
||
| async publish(...args: any[]): Promise<API.PublishResult> { | ||
| publish(...args: any[]): Promise<API.PublishResult> { | ||
| // Detect a v1-shape trailing callback synchronously, before the async body |
There was a problem hiding this comment.
We've added this comment in one place but not the myriad of others - is the comment necessary here?
| if (n <= minV2Args && typeof args[n - 2] !== 'function') return; | ||
| const err = new ErrorInfo('v1 callback signature is no longer supported.', 40000, 400); | ||
| err.hint = `Remove the trailing callback. See https://github.com/ably/ably-js/blob/main/docs/migration-guides/v2/lib.md`; | ||
| throw err; |
There was a problem hiding this comment.
Should this be calling out "go and use the Promise signature" explicitly rather than "dont use a callback"
| } | ||
|
|
||
| unsubscribe(...args: unknown[] /* [event], listener */): void { | ||
| Utils.detectV1Callback(args, 2); |
There was a problem hiding this comment.
unsubscribe never had a callback, so this is redundant
| * trailing functions, so e.g. `subscribe(listener, callback)` is always v1 | ||
| * even though it matches the arity of `subscribe(event, listener)`. | ||
| * | ||
| * @param minV2Args - The maximum number of args the v2 form accepts. For methods |
There was a problem hiding this comment.
We say min and then immediately say "the maximum" so this name is misleading.
Perhaps we'd call it v2TrailingFnArity, i.e.
/**
* @param v2TrailingFnArity - The arity at which v2 legitimately ends in a function
* (e.g. `subscribe(event, listener)` → 2). For methods where v2 never ends in a
* function (`authorize`, `publish`, `requestToken`, ...), pass 0.
*/
github-actions
Bot
temporarily deployed
to
staging/pull/2226/bundle-report
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/common/lib/types/errorinfo.ts`:
- Line 11: IConvertibleToErrorInfo and IConvertibleToPartialErrorInfo don't
include the new hint field, so update their input contracts to accept hint
(e.g., add hint?: string) and ensure any fromValues methods/signatures that
consume those interfaces (refer to fromValues) accept and propagate hint into
the constructed ErrorInfo/PartialErrorInfo; update IConvertibleToErrorInfo,
IConvertibleToPartialErrorInfo and the fromValues implementation(s) to include
and pass through hint.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e2aed483-dc1f-43d2-8061-57577d8c36c4
📒 Files selected for processing (13)
ably.d.tsscripts/moduleReport.tssrc/common/lib/client/auth.tssrc/common/lib/client/connection.tssrc/common/lib/client/realtimechannel.tssrc/common/lib/client/realtimepresence.tssrc/common/lib/types/errorinfo.tssrc/common/lib/util/utils.tssrc/platform/react-hooks/src/hooks/useChannel.tstest/realtime/auth.test.jstest/realtime/channel.test.jstest/realtime/connection.test.jstest/realtime/presence.test.js
- rename minV2Args -> v2TrailingFnArity (clearer semantics; docstring was
contradicting itself with "maximum" wording)
- use unknown[] consistently in rest signatures across auth, connection,
channel, and presence guarded methods
- point the hint at the Promise migration explicitly ("v2 uses Promises")
- remove redundant unsubscribe guard (v1 unsubscribe never took a callback)
- drop one-off "synchronous" inline comment from publish; the JSDoc on
detectV1Callback already explains the sync-throw rationale
- use autoConnect:false in sync-throw tests so they don't open a connection
just to validate argument-shape rejection
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
umair-ably
force-pushed
the
DX-1204/v1-callback-overloads
branch
from
40e33ba to
71ea4f3
Compare
github-actions
Bot
temporarily deployed
to
staging/pull/2226/bundle-report
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/platform/react-hooks/src/hooks/useChannel.ts (1)
59-62: ⚡ Quick winConsider simplifying to match the
publishpattern.The wrapper function only forwards the first parameter, which means two-argument v1 calls like
history(params, callback)would silently drop the callback. Additionally, the type cast toAbly.RealtimeChannel['history']is unsafe because the wrapper doesn't actually implement all overloads of that type.Consider using
.bind()instead, matching the pattern used forpublish:♻️ Simplify using `.bind()` to preserve all overloads
- const history: Ably.RealtimeChannel['history'] = useMemo( - () => ((params?: Ably.RealtimeHistoryParams) => channel.history(params)) as Ably.RealtimeChannel['history'], - [channel], - ); + const history: Ably.RealtimeChannel['history'] = useMemo( + () => channel.history.bind(channel), + [channel], + );This approach:
- Preserves all method overloads and arguments correctly (including multi-arg v1 forms for proper detection)
- Removes the unsafe type cast
- Matches the pattern already used for
publish(line 48)- Ensures the v1 callback detection in
channel.historyreceives all arguments as intended🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/react-hooks/src/hooks/useChannel.ts` around lines 59 - 62, The current useMemo wrapper for history only forwards the first param and unsafely casts to Ably.RealtimeChannel['history'], dropping multi-arg v1 callbacks; replace the manual wrapper with a bound method like using channel.history.bind(channel) inside useMemo (same approach as publish) so all overloads/arguments are preserved, remove the unsafe type cast, and keep history and channel references in the dependency array.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/platform/react-hooks/src/hooks/useChannel.ts`:
- Around line 59-62: The current useMemo wrapper for history only forwards the
first param and unsafely casts to Ably.RealtimeChannel['history'], dropping
multi-arg v1 callbacks; replace the manual wrapper with a bound method like
using channel.history.bind(channel) inside useMemo (same approach as publish) so
all overloads/arguments are preserved, remove the unsafe type cast, and keep
history and channel references in the dependency array.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 90a5b4ea-e0cf-4041-b908-940cb914fa5d
📒 Files selected for processing (13)
ably.d.tsscripts/moduleReport.tssrc/common/lib/client/auth.tssrc/common/lib/client/connection.tssrc/common/lib/client/realtimechannel.tssrc/common/lib/client/realtimepresence.tssrc/common/lib/types/errorinfo.tssrc/common/lib/util/utils.tssrc/platform/react-hooks/src/hooks/useChannel.tstest/realtime/auth.test.jstest/realtime/channel.test.jstest/realtime/connection.test.jstest/realtime/presence.test.js
AndyTWF
left a comment
AndyTWF
left a comment
There was a problem hiding this comment.
Please add a separate PR to the docs to document the new error code there 
2 participants
Intent
This PR brings back the deleted v1 callback styled APIs and instead
Why
LLMs relying on training data take more turns than necessary to self-heal when using old v1 callbacks.
Outcome and Evidence
I tasked Claude under emulated constraints e.g. here is the v1 styled callbacks API, assume this is your training data, you are not allowed to use any webfetch/websearch tools, etc.. 3 tasks:
Task 1 - Publish POC with actual training data
Across three models (Opus 4.7, Sonnet 4.6, Haiku 4.5) × five attempts each x for both the pre and post this PR SDK (so 30 total runs), only 1 run actually used v1 callbacks from training. The run that encountered it was running an Ably SDK which included this PR, so the very next tool call fixed the issue.
This shows recent training data has largely caught up with v2 promises, however, the non-deterministic nature of LLMs is still evidenced by that 1 errant run.
Task 2 - Publish POC with emulated training data
If we extrapolate the errant run from task 1 to all runs i.e. emulate stale training data for every run, we start to see some results. All v1 styled runs failed on the very first iteration and hung until the agent stepped in to debug the issue. Opus and Sonnet weren't horrendous here but they took anywhere from 12-14 tool calls to get to a working solution. Haiku on the other hand, took upwards of 25 tool calls.
Likewise, all the v2 styled runs also failed on the very first iteration, however all of them took only 8 tool calls to rectify the issue, including Haiku.
Task 3 - Unit test
Same idea as task 2 except the agent gets a failing unit test instead of a POC, and the goal is to make node --test exit 0. The test runner behaves slightly differently in that v1 failures show up as either an unhandled rejection from deep in the SDK or a test timeout. Opus fixed it in 5 tool calls from reverting to it's own training data, Sonnet went in circles for 22 tool calls, and Haiku couldn't figure it out until it's 5th and final iteration.
Positively, all v2 styled runs landed in 2 iterations at 6-8 tool calls. The logs/conversations clearly showing the error and hint was what steered them to fix it immediately after.
Summary by CodeRabbit
Breaking Changes
Improvements
Tests
Chores