-
Notifications
You must be signed in to change notification settings - Fork 0
[DX-255] Improve error code formatting #174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -79,41 +79,43 @@ export default class ChannelsPresenceSubscribe extends AblyBaseCommand { | |
| ); | ||
| } | ||
|
|
||
| channel.presence.subscribe((presenceMessage: Ably.PresenceMessage) => { | ||
| const timestamp = formatMessageTimestamp(presenceMessage.timestamp); | ||
| const presenceData = { | ||
| id: presenceMessage.id, | ||
| timestamp, | ||
| action: presenceMessage.action, | ||
| channel: channelName, | ||
| clientId: presenceMessage.clientId, | ||
| connectionId: presenceMessage.connectionId, | ||
| data: presenceMessage.data, | ||
| }; | ||
| this.logCliEvent( | ||
| flags, | ||
| "presence", | ||
| presenceMessage.action!, | ||
| `Presence event: ${presenceMessage.action} by ${presenceMessage.clientId}`, | ||
| presenceData, | ||
| ); | ||
|
|
||
| if (this.shouldOutputJson(flags)) { | ||
| this.logJsonEvent({ presenceMessage: presenceData }, flags); | ||
| } else { | ||
| const displayFields: PresenceDisplayFields = { | ||
| await channel.presence.subscribe( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are you adding awaits throughout? |
||
| (presenceMessage: Ably.PresenceMessage) => { | ||
| const timestamp = formatMessageTimestamp(presenceMessage.timestamp); | ||
| const presenceData = { | ||
| id: presenceMessage.id, | ||
| timestamp: presenceMessage.timestamp ?? Date.now(), | ||
| action: presenceMessage.action || "unknown", | ||
| timestamp, | ||
| action: presenceMessage.action, | ||
umair-ably marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| channel: channelName, | ||
| clientId: presenceMessage.clientId, | ||
| connectionId: presenceMessage.connectionId, | ||
| data: presenceMessage.data, | ||
| }; | ||
| this.log(formatPresenceOutput([displayFields])); | ||
| this.log(""); // Empty line for better readability | ||
| } | ||
| }); | ||
| this.logCliEvent( | ||
| flags, | ||
| "presence", | ||
| presenceMessage.action!, | ||
| `Presence event: ${presenceMessage.action} by ${presenceMessage.clientId}`, | ||
| presenceData, | ||
| ); | ||
|
|
||
| if (this.shouldOutputJson(flags)) { | ||
| this.logJsonEvent({ presenceMessage: presenceData }, flags); | ||
| } else { | ||
| const displayFields: PresenceDisplayFields = { | ||
| id: presenceMessage.id, | ||
| timestamp: presenceMessage.timestamp ?? Date.now(), | ||
| action: presenceMessage.action || "unknown", | ||
| channel: channelName, | ||
| clientId: presenceMessage.clientId, | ||
| connectionId: presenceMessage.connectionId, | ||
| data: presenceMessage.data, | ||
| }; | ||
| this.log(formatPresenceOutput([displayFields])); | ||
| this.log(""); // Empty line for better readability | ||
| } | ||
| }, | ||
| ); | ||
|
|
||
| if (!this.shouldOutputJson(flags)) { | ||
| this.log( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -154,7 +154,7 @@ export default class ChannelsSubscribe extends AblyBaseCommand { | |
| }); | ||
|
|
||
| // Subscribe to messages on all channels | ||
| const attachPromises: Promise<void>[] = []; | ||
| const subscribePromises: Promise<unknown>[] = []; | ||
umair-ably marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| for (const channel of channels) { | ||
| this.logCliEvent( | ||
|
|
@@ -177,19 +177,8 @@ export default class ChannelsSubscribe extends AblyBaseCommand { | |
| includeUserFriendlyMessages: true, | ||
| }); | ||
|
|
||
| // Track attachment promise | ||
| const attachPromise = new Promise<void>((resolve) => { | ||
| const checkAttached = () => { | ||
| if (channel.state === "attached") { | ||
| resolve(); | ||
| } | ||
| }; | ||
| channel.once("attached", checkAttached); | ||
| checkAttached(); // Check if already attached | ||
| }); | ||
| attachPromises.push(attachPromise); | ||
|
|
||
| channel.subscribe((message: Ably.Message) => { | ||
| // Subscribe and collect promise (rejects on capability/auth errors) | ||
| const subscribePromise = channel.subscribe((message: Ably.Message) => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is genuinely an issue in the SDK that we only get errors if we subscribe (with implicit attach) instead of attach then subscribe, why are we not fixing his upstream? |
||
| this.sequenceCounter++; | ||
| const timestamp = formatMessageTimestamp(message.timestamp); | ||
umair-ably marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| const messageData = { | ||
|
|
@@ -251,10 +240,11 @@ export default class ChannelsSubscribe extends AblyBaseCommand { | |
| this.log(""); // Empty line for readability between messages | ||
| } | ||
| }); | ||
| subscribePromises.push(subscribePromise); | ||
| } | ||
|
|
||
| // Wait for all channels to attach | ||
| await Promise.all(attachPromises); | ||
| // Wait for all channels to attach via subscribe | ||
| await Promise.all(subscribePromises); | ||
umair-ably marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| // Log the ready signal for E2E tests | ||
| if (channelNames.length === 1 && !this.shouldOutputJson(flags)) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,3 +4,26 @@ | |
| export function errorMessage(error: unknown): string { | ||
| return error instanceof Error ? error.message : String(error); | ||
| } | ||
|
|
||
| /** | ||
| * Return a friendly, actionable hint for known Ably error codes. | ||
| * Returns undefined for unknown codes. | ||
| */ | ||
| const hints: Record<number, string> = { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I appreciate the addition about the CLI specifically, re: login or authenticating again. However, what I am not a fan of is the addition of what seems like error messages that should be addressed upstream, such as "The current credentials do not have the capability to perform this operation on the requested resource". I could be reading this wrong, but it appears you are hard coding in improved error messages into the CLI because the upstream error messages aren't adequate, which I identified in Aug in https://github.com/ably/ably-os-to-be-archived/pull/3, and created PRs for SDKs & core to address this. As above, I may be misreading this, but it now seems we're pushing this problem down two layers, i.e. not at source (realtime), not at SDK level, but now at the CLI level. Should we not just fix this at source? |
||
| 40101: | ||
| 'The credentials provided are not valid. Check your API key or token, or re-authenticate with "ably login".', | ||
| 40103: 'The token has expired. Please re-authenticate with "ably login".', | ||
| 40110: | ||
| 'Unable to authorize. Check your authentication configuration or re-authenticate with "ably login".', | ||
| 40160: | ||
| "The current credentials do not have the capability to perform this operation on the requested resource. Check the token or key capability configuration in the Ably dashboard.", | ||
| 40161: | ||
| "The current credentials do not have publish capability on this resource. Check the token or key capability configuration in the Ably dashboard.", | ||
| 40171: | ||
| "The requested operation is not permitted by the current credentials' capabilities. Check the token or key capability configuration in the Ably dashboard.", | ||
| }; | ||
|
|
||
| export function getFriendlyAblyErrorHint(code?: number): string | undefined { | ||
| if (code === undefined) return undefined; | ||
| return hints[code]; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -118,6 +118,32 @@ describe("channels:occupancy:subscribe command", () => { | |
| expect(error).toBeDefined(); | ||
| expect(error?.message).toMatch(/No mock|client/i); | ||
| }); | ||
|
|
||
| it("should handle capability error gracefully", async () => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why mock it? Should we not just test it at least once end to end to ensure it works with the API as expected? |
||
| const mock = getMockAblyRealtime(); | ||
| const channel = mock.channels._getChannel("test-channel"); | ||
|
|
||
| channel.subscribe.mockRejectedValue( | ||
| Object.assign( | ||
| new Error("Channel denied access based on given capability"), | ||
| { | ||
| code: 40160, | ||
| statusCode: 401, | ||
| href: "https://help.ably.io/error/40160", | ||
| }, | ||
| ), | ||
| ); | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| const { error } = await runCommand( | ||
umair-ably marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ["channels:occupancy:subscribe", "test-channel"], | ||
| import.meta.url, | ||
| ); | ||
|
|
||
| expect(error).toBeDefined(); | ||
| expect(error?.message).toContain("Channel denied access"); | ||
| expect(error?.message).toContain("capability"); | ||
| expect(error?.message).toContain("Ably dashboard"); | ||
| }); | ||
| }); | ||
|
|
||
| describe("output formats", () => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What has this got to do with improving error code formatting?