Skip to content

Commit 71ea4f3

Browse files
umair-ablyclaude
andcommitted
DX-1204: address review feedback on v1 callback guard
- 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>
1 parent 7fecab1 commit 71ea4f3

9 files changed

Lines changed: 41 additions & 58 deletions

File tree

src/common/lib/client/auth.ts

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -254,9 +254,12 @@ class Auth {
254254
*/
255255
async authorize(tokenParams: API.TokenParams | null, authOptions: AuthOptions | null): Promise<API.TokenDetails>;
256256

257-
authorize(...args: any[]): Promise<API.TokenDetails> {
257+
authorize(...args: unknown[]): Promise<API.TokenDetails> {
258258
Utils.detectV1Callback(args, 0);
259-
return this._authorizeImpl(args[0], args[1]);
259+
return this._authorizeImpl(
260+
args[0] as Record<string, any> | null | undefined,
261+
args[1] as AuthOptions | null | undefined,
262+
);
260263
}
261264

262265
private async _authorizeImpl(
@@ -395,9 +398,9 @@ class Auth {
395398
*/
396399
async requestToken(tokenParams: API.TokenParams | null, authOptions: AuthOptions): Promise<API.TokenDetails>;
397400

398-
requestToken(...args: any[]): Promise<API.TokenDetails> {
401+
requestToken(...args: unknown[]): Promise<API.TokenDetails> {
399402
Utils.detectV1Callback(args, 0);
400-
return this._requestTokenImpl(args[0], args[1]);
403+
return this._requestTokenImpl(args[0] as API.TokenParams | null | undefined, args[1] as AuthOptions | undefined);
401404
}
402405

403406
private async _requestTokenImpl(
@@ -757,9 +760,9 @@ class Auth {
757760
* - timestamp: (optional) the time in ms since the epoch. If none is specified,
758761
* the system will be queried for a time value to use.
759762
*/
760-
createTokenRequest(...args: any[]): Promise<API.TokenRequest> {
763+
createTokenRequest(...args: unknown[]): Promise<API.TokenRequest> {
761764
Utils.detectV1Callback(args, 0);
762-
return this._createTokenRequestImpl(args[0], args[1]);
765+
return this._createTokenRequestImpl(args[0] as API.TokenParams | null, args[1]);
763766
}
764767

765768
private async _createTokenRequestImpl(

src/common/lib/client/connection.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ class Connection extends EventEmitter {
4747
this.connectionManager.requestState({ state: 'connecting' });
4848
}
4949

50-
ping(...args: any[]): Promise<number> {
50+
ping(...args: unknown[]): Promise<number> {
5151
Utils.detectV1Callback(args, 0);
5252
return this._pingImpl();
5353
}

src/common/lib/client/realtimechannel.ts

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -250,10 +250,7 @@ class RealtimeChannel extends EventEmitter {
250250
return false;
251251
}
252252

253-
publish(...args: any[]): Promise<API.PublishResult> {
254-
// Detect a v1-shape trailing callback synchronously, before the async body
255-
// starts — agents pasting v1 patterns need the throw to surface at the call
256-
// site, not as an unobserved Promise rejection.
253+
publish(...args: unknown[]): Promise<API.PublishResult> {
257254
Utils.detectV1Callback(args, 0);
258255
return this._publishImpl(args);
259256
}
@@ -489,7 +486,6 @@ class RealtimeChannel extends EventEmitter {
489486
}
490487

491488
unsubscribe(...args: unknown[] /* [event], listener */): void {
492-
Utils.detectV1Callback(args, 2);
493489
const [event, listener] = RealtimeChannel.processListenerArgs(args);
494490

495491
// If we either have a filtered listener, a filter or both we need to do additional processing to find the original function(s)
@@ -999,9 +995,9 @@ class RealtimeChannel extends EventEmitter {
999995
}
1000996
}
1001997

1002-
history = function (this: RealtimeChannel, ...args: any[]): Promise<PaginatedResult<Message>> {
998+
history = function (this: RealtimeChannel, ...args: unknown[]): Promise<PaginatedResult<Message>> {
1003999
Utils.detectV1Callback(args, 0);
1004-
return this._historyImpl(args[0]);
1000+
return this._historyImpl(args[0] as RealtimeHistoryParams | null);
10051001
} as any;
10061002

10071003
private _historyImpl = async function (

src/common/lib/client/realtimepresence.ts

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ class RealtimePresence extends EventEmitter {
5454
this.pendingPresence = [];
5555
}
5656

57-
enter(...args: any[]): Promise<void> {
57+
enter(...args: unknown[]): Promise<void> {
5858
Utils.detectV1Callback(args, 0);
5959
return this._enterImpl(args[0]);
6060
}
@@ -66,7 +66,7 @@ class RealtimePresence extends EventEmitter {
6666
return this._enterOrUpdateClient(undefined, undefined, data, 'enter');
6767
}
6868

69-
update(...args: any[]): Promise<void> {
69+
update(...args: unknown[]): Promise<void> {
7070
Utils.detectV1Callback(args, 0);
7171
return this._updateImpl(args[0]);
7272
}
@@ -139,7 +139,7 @@ class RealtimePresence extends EventEmitter {
139139
}
140140
}
141141

142-
leave(...args: any[]): Promise<void> {
142+
leave(...args: unknown[]): Promise<void> {
143143
Utils.detectV1Callback(args, 0);
144144
return this._leaveImpl(args[0]);
145145
}
@@ -191,9 +191,9 @@ class RealtimePresence extends EventEmitter {
191191
}
192192
}
193193

194-
get(...args: any[]): Promise<PresenceMessage[]> {
194+
get(...args: unknown[]): Promise<PresenceMessage[]> {
195195
Utils.detectV1Callback(args, 0);
196-
return this._getImpl(args[0]);
196+
return this._getImpl(args[0] as RealtimePresenceParams | undefined);
197197
}
198198

199199
private async _getImpl(params?: RealtimePresenceParams): Promise<PresenceMessage[]> {
@@ -224,9 +224,9 @@ class RealtimePresence extends EventEmitter {
224224
return toMessages(this.members);
225225
}
226226

227-
history(...args: any[]): Promise<PaginatedResult<PresenceMessage>> {
227+
history(...args: unknown[]): Promise<PaginatedResult<PresenceMessage>> {
228228
Utils.detectV1Callback(args, 0);
229-
return this._historyImpl(args[0]);
229+
return this._historyImpl(args[0] as RealtimeHistoryParams | null);
230230
}
231231

232232
private async _historyImpl(params: RealtimeHistoryParams | null): Promise<PaginatedResult<PresenceMessage>> {

src/common/lib/util/utils.ts

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -274,23 +274,24 @@ export function isErrorInfoOrPartialErrorInfo(err: unknown): err is ErrorInfo |
274274
* trailing function (e.g. ClientOptions.authCallback).
275275
*
276276
* Fires when the trailing arg is a function AND either:
277-
* - `args.length > minV2Args` (the trailing fn is beyond the v2 form's max), or
277+
* - `args.length > v2TrailingFnArity` (the trailing fn is beyond the arity at
278+
* which v2 legitimately ends in a function), or
278279
* - the second-to-last arg is also a function — none of the v2 forms take two
279280
* trailing functions, so e.g. `subscribe(listener, callback)` is always v1
280281
* even though it matches the arity of `subscribe(event, listener)`.
281282
*
282-
* @param minV2Args - The maximum number of args the v2 form accepts. For methods
283-
* whose v2 form never takes a trailing function (e.g. `authorize`, `publish`),
284-
* pass `0`. For methods whose v2 form accepts a trailing listener (e.g.
285-
* `subscribe(event, listener)`), pass the v2 arg count so the listener at the
286-
* v2 max isn't misclassified as v1.
283+
* @param v2TrailingFnArity - The arity at which v2 legitimately ends in a function
284+
* (e.g. `subscribe(event, listener)` → 2). For methods where v2 never ends in a
285+
* function (`authorize`, `publish`, `requestToken`, ...), pass 0.
287286
*/
288-
export function detectV1Callback(args: ArrayLike<unknown>, minV2Args: number): void {
287+
export function detectV1Callback(args: ArrayLike<unknown>, v2TrailingFnArity: number): void {
289288
const n = args.length;
290289
if (typeof args[n - 1] !== 'function') return;
291-
if (n <= minV2Args && typeof args[n - 2] !== 'function') return;
290+
if (n <= v2TrailingFnArity && typeof args[n - 2] !== 'function') return;
292291
const err = new ErrorInfo('v1 callback signature is no longer supported.', 40000, 400);
293-
err.hint = `Remove the trailing callback. See https://github.com/ably/ably-js/blob/main/docs/migration-guides/v2/lib.md`;
292+
err.hint =
293+
'v2 uses Promises — drop the trailing callback and `await` the returned promise. ' +
294+
'See https://github.com/ably/ably-js/blob/main/docs/migration-guides/v2/lib.md';
294295
throw err;
295296
}
296297

test/realtime/auth.test.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1655,7 +1655,7 @@ define(['ably', 'shared_helper', 'async', 'chai'], function (Ably, Helper, async
16551655
].forEach(function (testCase) {
16561656
it('v1_callback_auth_' + testCase.method + '_throws_synchronously', function (done) {
16571657
var helper = this.test.helper,
1658-
realtime = helper.AblyRealtime();
1658+
realtime = helper.AblyRealtime({ autoConnect: false });
16591659

16601660
try {
16611661
testCase.invoke(realtime.auth);
@@ -1670,7 +1670,7 @@ define(['ably', 'shared_helper', 'async', 'chai'], function (Ably, Helper, async
16701670
expect(err.message).to.contain('v1 callback signature');
16711671
expect(err.message).to.contain('no longer supported');
16721672
expect(err.hint).to.be.a('string');
1673-
expect(err.hint).to.contain('Remove the trailing callback');
1673+
expect(err.hint).to.contain('v2 uses Promises');
16741674
expect(err.hint).to.contain('https://github.com/ably/ably-js/blob/main/docs/migration-guides/v2/lib.md');
16751675
helper.closeAndFinish(done, realtime);
16761676
} catch (assertionErr) {

test/realtime/channel.test.js

Lines changed: 5 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1451,9 +1451,9 @@ define(['ably', 'shared_helper', 'async', 'chai'], function (Ably, Helper, async
14511451

14521452
/**
14531453
* v1-style trailing-callback shape on RealtimeChannel.{publish, subscribe,
1454-
* unsubscribe, history} throws synchronously with a steering ErrorInfo whose
1455-
* message diagnoses *what* went wrong and whose hint prescribes the v2
1456-
* replacement call.
1454+
* history} throws synchronously with a steering ErrorInfo whose message
1455+
* diagnoses *what* went wrong and whose hint prescribes the v2 replacement
1456+
* call.
14571457
*/
14581458
[
14591459
{ method: 'publish', invoke: (ch) => ch.publish('event', { hello: 'world' }, () => {}) },
@@ -1474,28 +1474,11 @@ define(['ably', 'shared_helper', 'async', 'chai'], function (Ably, Helper, async
14741474
() => {},
14751475
),
14761476
},
1477-
{
1478-
method: 'unsubscribe',
1479-
invoke: (ch) =>
1480-
ch.unsubscribe(
1481-
'event',
1482-
() => {},
1483-
() => {},
1484-
),
1485-
},
1486-
{
1487-
method: 'unsubscribe_listener_callback',
1488-
invoke: (ch) =>
1489-
ch.unsubscribe(
1490-
() => {},
1491-
() => {},
1492-
),
1493-
},
14941477
{ method: 'history', invoke: (ch) => ch.history(null, () => {}) },
14951478
].forEach(function (testCase) {
14961479
it('v1_callback_' + testCase.method + '_throws_synchronously', function (done) {
14971480
var helper = this.test.helper,
1498-
realtime = helper.AblyRealtime(),
1481+
realtime = helper.AblyRealtime({ autoConnect: false }),
14991482
channel = realtime.channels.get('v1cb_channel_' + testCase.method);
15001483

15011484
try {
@@ -1511,7 +1494,7 @@ define(['ably', 'shared_helper', 'async', 'chai'], function (Ably, Helper, async
15111494
expect(err.message).to.contain('v1 callback signature');
15121495
expect(err.message).to.contain('no longer supported');
15131496
expect(err.hint).to.be.a('string');
1514-
expect(err.hint).to.contain('Remove the trailing callback');
1497+
expect(err.hint).to.contain('v2 uses Promises');
15151498
expect(err.hint).to.contain('https://github.com/ably/ably-js/blob/main/docs/migration-guides/v2/lib.md');
15161499
helper.closeAndFinish(done, realtime);
15171500
} catch (assertionErr) {

test/realtime/connection.test.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -480,7 +480,7 @@ define(['ably', 'shared_helper', 'async', 'chai'], function (Ably, Helper, async
480480
*/
481481
it('v1_callback_ping_throws_synchronously', function (done) {
482482
const helper = this.test.helper;
483-
const realtime = helper.AblyRealtime();
483+
const realtime = helper.AblyRealtime({ autoConnect: false });
484484
try {
485485
realtime.connection.ping(function noopCallback(err) {
486486
void err;
@@ -492,7 +492,7 @@ define(['ably', 'shared_helper', 'async', 'chai'], function (Ably, Helper, async
492492
expect(err.message).to.contain('v1 callback signature');
493493
expect(err.message).to.contain('no longer supported');
494494
expect(err.hint).to.be.a('string');
495-
expect(err.hint).to.contain('Remove the trailing callback');
495+
expect(err.hint).to.contain('v2 uses Promises');
496496
expect(err.hint).to.contain('https://github.com/ably/ably-js/blob/main/docs/migration-guides/v2/lib.md');
497497
helper.closeAndFinish(done, realtime);
498498
} catch (assertionErr) {

test/realtime/presence.test.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2340,7 +2340,7 @@ define(['ably', 'shared_helper', 'async', 'chai'], function (Ably, Helper, async
23402340
].forEach(function (testCase) {
23412341
it('v1_callback_presence_' + testCase.method + '_throws_synchronously', function (done) {
23422342
var helper = this.test.helper,
2343-
realtime = helper.AblyRealtime({ clientId: testClientId }),
2343+
realtime = helper.AblyRealtime({ clientId: testClientId, autoConnect: false }),
23442344
channel = realtime.channels.get('v1cb_presence_' + testCase.method);
23452345

23462346
try {
@@ -2356,7 +2356,7 @@ define(['ably', 'shared_helper', 'async', 'chai'], function (Ably, Helper, async
23562356
expect(err.message).to.contain('v1 callback signature');
23572357
expect(err.message).to.contain('no longer supported');
23582358
expect(err.hint).to.be.a('string');
2359-
expect(err.hint).to.contain('Remove the trailing callback');
2359+
expect(err.hint).to.contain('v2 uses Promises');
23602360
expect(err.hint).to.contain('https://github.com/ably/ably-js/blob/main/docs/migration-guides/v2/lib.md');
23612361
helper.closeAndFinish(done, realtime);
23622362
} catch (assertionErr) {

0 commit comments

Comments
 (0)