Skip to content

Conversation

@owenpearson
Copy link
Member

@owenpearson owenpearson commented Oct 8, 2025

We shouldn't be expecting users to construct their own ErrorInfo here when their auth callback fails, since they won't know what code to use. ISTM the typings here were written to conform to the internal tokenRequestCallback variable? The code seems to treat the error opaquely so just changing to Error should be safe. NB: not a breaking change since I've maintained the string option and ErrorInfo inherits from Error.

Summary by CodeRabbit

  • Bug Fixes

    • Improved authentication error handling to accept standard Error objects in callbacks, reducing edge-case failures during token requests and authorization flows.
    • Enhanced robustness of error propagation in auth-related callbacks, preventing unexpected crashes and improving reliability.
  • Refactor

    • Aligned callback error types across authentication flows for greater consistency and compatibility with common error objects.

@coderabbitai
Copy link

coderabbitai bot commented Oct 8, 2025

Walkthrough

The change widens error parameter types for authentication callbacks. In ably.d.ts, AuthOptions.authCallback’s first argument changes from ErrorInfo to Error. In src/common/lib/client/auth.ts, Auth.requestToken’s callback adds Error to its accepted error union.

Changes

Cohort / File(s) Summary
Public type definitions
ably.d.ts
AuthOptions.authCallback error param type changed from `ErrorInfo
Auth token request callback
src/common/lib/client/auth.ts
Auth.requestToken’s tokenRequestCallback error type widened from `API.ErrorInfo

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

A bunny taps keys with a hop and a twirl,
Error got wider—now give it a whirl!
From Info to Error, the types realign,
Callbacks feel safer; the signatures shine.
Carrot-typed joy in the burrow tonight—
Ship it, then nibble; the diff looks right. 🥕

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly and concisely summarizes the primary change by indicating that the typings for authCallback now accept a generic Error, matching the actual modifications in the pull request.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch auth-callback-error-typing

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 220646b and c0bb437.

📒 Files selected for processing (2)
  • ably.d.ts (1 hunks)
  • src/common/lib/client/auth.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/common/lib/client/auth.ts (2)
src/common/lib/types/errorinfo.ts (1)
  • ErrorInfo (35-66)
src/common/types/http.ts (1)
  • RequestResultError (12-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: test-node (20.x)
  • GitHub Check: test-browser (firefox)
  • GitHub Check: test-node (16.x)
  • GitHub Check: test-browser (webkit)
  • GitHub Check: test-node (18.x)
  • GitHub Check: test-browser (chromium)
🔇 Additional comments (2)
ably.d.ts (1)

652-655: LGTM! Backward-compatible widening of error type.

The change from ErrorInfo to Error is backward compatible since ErrorInfo extends Error, allowing users to pass generic Error instances without needing to construct an ErrorInfo with an appropriate error code.

src/common/lib/client/auth.ts (1)

400-407: LGTM! Consistent with public API and runtime behavior.

Adding Error to the union type aligns the internal implementation with the public API change in ably.d.ts. The runtime already handles generic Error instances correctly via normaliseAuthcallbackError (line 629), which wraps non-ErrorInfo errors appropriately.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions bot temporarily deployed to staging/pull/2099/features October 8, 2025 13:03 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/2099/typedoc October 8, 2025 13:04 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/2099/bundle-report October 8, 2025 13:04 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants