Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 35 additions & 1 deletion test/uts/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,22 @@ let _savedNow: any = null;
// Tracked clients for cleanup — ensures timers are released even if a test crashes
const _trackedClients: any[] = [];

// Track all Platform.Config.setTimeout timers so restoreAll() can cancel orphans.
// ably-js has a bug where connectWs() overwrites timer handles without cancelling
// the previous ones, leaking timers that prevent process exit.
const _allPlatformTimers = new Set<any>();
const _origPlatformSetTimeout = Platform.Config.setTimeout;
const _origPlatformClearTimeout = Platform.Config.clearTimeout;
Platform.Config.setTimeout = function (fn: any, ms?: number, ...args: any[]) {
const timer = _origPlatformSetTimeout.call(this, fn, ms, ...args);
_allPlatformTimers.add(timer);
return timer;
} as any;
Platform.Config.clearTimeout = function (timer: any) {
_allPlatformTimers.delete(timer);
return _origPlatformClearTimeout.call(this, timer);
Comment on lines +41 to +50
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# First, find the test/uts/helpers.ts file and read the relevant lines
if [ -f "test/uts/helpers.ts" ]; then
  echo "=== File: test/uts/helpers.ts (lines 35-55) ==="
  sed -n '35,55p' test/uts/helpers.ts | cat -n
else
  echo "File not found at test/uts/helpers.ts"
  git ls-files | grep -i helpers.ts
fi

Repository: ably/ably-js

Length of output: 1182


🏁 Script executed:

#!/bin/bash
# Find where Platform.Config is defined
echo "=== Searching for Platform.Config definition ==="
rg "Platform\.Config" -A 5 -B 5 --type ts --type tsx | head -100

Repository: ably/ably-js

Length of output: 151


🏁 Script executed:

#!/bin/bash
# Look for IPlatformConfig or similar type definitions
echo "=== Searching for Platform type definitions ==="
rg "interface.*Platform|type.*Platform" --type ts --type tsx | head -30

Repository: ably/ably-js

Length of output: 150


🏁 Script executed:

#!/bin/bash
# Try searching for Platform config without specifying ts/tsx
echo "=== Searching for Platform.Config definition ==="
rg "Platform" -t ts -A 3 | grep -E "(Config|setTimeout|clearTimeout)" | head -50

Repository: ably/ably-js

Length of output: 4588


🏁 Script executed:

#!/bin/bash
# Search for interface or type definitions containing setTimeout
echo "=== Searching for setTimeout definitions ==="
rg "setTimeout|clearTimeout" -t ts -B 3 -A 1 | grep -E "(interface|type|setTimeout|clearTimeout|\s+:)" | head -80

Repository: ably/ably-js

Length of output: 8166


🏁 Script executed:

#!/bin/bash
# Look for tsconfig.json or compilation setup
echo "=== Looking for TypeScript configuration ==="
find . -name "tsconfig.json" -o -name "tsc" | head -10

Repository: ably/ably-js

Length of output: 507


🏁 Script executed:

#!/bin/bash
# Check if there's a dist/es directory or build output that shows compilation errors
echo "=== Checking for compilation errors or issue tracking ==="
rg "TS2683|TS2556" -t ts -B 5 -A 5 | head -50

Repository: ably/ably-js

Length of output: 114


🏁 Script executed:

#!/bin/bash
cat -n src/common/types/IPlatformConfig.d.ts | head -50

Repository: ably/ably-js

Length of output: 2183


Fix the Platform.Config wrapper signatures to match IPlatformConfig.

The wrapper functions use as any to bypass type errors. The interface defines setTimeout: (handler: () => void, timeout?: number) => ... and clearTimeout: (id: ... | null | undefined) => void, but the wrapper adds a variadic ...args: any[], uses untyped this context, and returns a value from clearTimeout. Remove the variadic parameters, switch to arrow functions, and eliminate the unnecessary return in clearTimeout.

Proposed fix
-Platform.Config.setTimeout = function (fn: any, ms?: number, ...args: any[]) {
-  const timer = _origPlatformSetTimeout.call(this, fn, ms, ...args);
+Platform.Config.setTimeout = ((fn: () => void, ms?: number) => {
+  const timer = _origPlatformSetTimeout(fn, ms);
   _allPlatformTimers.add(timer);
   return timer;
-} as any;
-Platform.Config.clearTimeout = function (timer: any) {
+}) as typeof Platform.Config.setTimeout;
+Platform.Config.clearTimeout = ((timer) => {
   _allPlatformTimers.delete(timer);
-  return _origPlatformClearTimeout.call(this, timer);
-} as any;
+  _origPlatformClearTimeout(timer);
+}) as typeof Platform.Config.clearTimeout;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const _origPlatformSetTimeout = Platform.Config.setTimeout;
const _origPlatformClearTimeout = Platform.Config.clearTimeout;
Platform.Config.setTimeout = function (fn: any, ms?: number, ...args: any[]) {
const timer = _origPlatformSetTimeout.call(this, fn, ms, ...args);
_allPlatformTimers.add(timer);
return timer;
} as any;
Platform.Config.clearTimeout = function (timer: any) {
_allPlatformTimers.delete(timer);
return _origPlatformClearTimeout.call(this, timer);
const _origPlatformSetTimeout = Platform.Config.setTimeout;
const _origPlatformClearTimeout = Platform.Config.clearTimeout;
Platform.Config.setTimeout = ((fn: () => void, ms?: number) => {
const timer = _origPlatformSetTimeout(fn, ms);
_allPlatformTimers.add(timer);
return timer;
}) as typeof Platform.Config.setTimeout;
Platform.Config.clearTimeout = ((timer) => {
_allPlatformTimers.delete(timer);
_origPlatformClearTimeout(timer);
}) as typeof Platform.Config.clearTimeout;
🧰 Tools
🪛 GitHub Actions: API Reference

[error] 44-44: TypeScript compilation error TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.


[error] 44-44: TypeScript compilation error TS2556: A spread argument must either have a tuple type or be passed to a rest parameter.


[error] 50-50: TypeScript compilation error TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.

🪛 GitHub Actions: Bundle size report

[error] 44-44: TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.


[error] 44-44: TS2556: A spread argument must either have a tuple type or be passed to a rest parameter.


[error] 50-50: TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.

🪛 GitHub Actions: Lint

[error] 44-44: TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.


[error] 44-44: TS2556: A spread argument must either have a tuple type or be passed to a rest parameter.


[error] 50-50: TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.

🪛 GitHub Actions: Spec coverage report

[error] 44-44: TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.


[error] 44-44: TS2556: A spread argument must either have a tuple type or be passed to a rest parameter.


[error] 50-50: TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.

🪛 GitHub Actions: Test (react hooks)

[error] 44-44: TypeScript TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.


[error] 44-44: TypeScript TS2556: A spread argument must either have a tuple type or be passed to a rest parameter.


[error] 50-50: TypeScript TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.

🪛 GitHub Actions: Test NodeJS

[error] 44-44: TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.


[error] 44-44: TS2556: A spread argument must either have a tuple type or be passed to a rest parameter.


[error] 50-50: TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.

🪛 GitHub Actions: Test NPM package

[error] 44-44: TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.


[error] 44-44: TS2556: A spread argument must either have a tuple type or be passed to a rest parameter.


[error] 50-50: TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/uts/helpers.ts` around lines 41 - 50, The Platform.Config wrapper
functions currently diverge from IPlatformConfig and use unsafe casts; update
the wrappers for Platform.Config.setTimeout and Platform.Config.clearTimeout to
match the interface by removing the variadic ...args and using arrow functions
so they inherit the correct lexical this, call
_origPlatformSetTimeout/_origPlatformClearTimeout with the expected (handler,
timeout) signature, add proper parameter types (handler: () => void, timeout?:
number) and ensure clearTimeout does not return a value (void) while still
deleting the timer from _allPlatformTimers; keep references to
_origPlatformSetTimeout, _origPlatformClearTimeout and _allPlatformTimers to
maintain behavior.

} as any;

/**
* Install a MockHttpClient as the platform HTTP implementation.
* Call uninstallMockHttp() in afterEach to restore the original.
Expand Down Expand Up @@ -151,7 +167,7 @@ class FakeClock {
// Yield to the event loop (not just the microtask queue) so that all
// chained process.nextTick callbacks (e.g. mock WebSocket error/close
// events) are fully drained before the next fake timer fires.
await new Promise<void>((resolve) => setTimeout(resolve, 0));
await new Promise<void>((resolve) => setImmediate(resolve));
}
this._now = targetTime;
}
Expand Down Expand Up @@ -222,6 +238,12 @@ function restoreAll(): void {
// Ignore errors during cleanup
}
}
// Cancel all Platform.Config timers that weren't cleared by client.close().
// Covers orphaned timers from ably-js's connectWs() overwrite bug.
for (const timer of _allPlatformTimers) {
_origPlatformClearTimeout(timer);
}
_allPlatformTimers.clear();
uninstallMockHttp();
uninstallMockWebSocket();
// Restore fake timers if installed
Expand All @@ -235,6 +257,17 @@ function restoreAll(): void {
}
}

/**
* Flush the async event loop — yields to both microtasks and the macrotask
* queue so that pending promise callbacks, nextTick handlers, and queued
* mock WebSocket/HTTP deliveries all settle before the test continues.
*
* Replaces all `await new Promise(r => setTimeout(r, N))` delays in tests.
*/
function flushAsync(): Promise<void> {
return new Promise<void>((resolve) => setImmediate(resolve));
}

export {
Ably,
Platform,
Expand All @@ -246,4 +279,5 @@ export {
FakeClock,
trackClient,
restoreAll,
flushAsync,
};
Loading
Loading