-
Notifications
You must be signed in to change notification settings - Fork 61
updateMessage and deleteMessage: remove Message response #2114
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
Conversation
WalkthroughUpdated Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Channel
participant RestMixin
participant Server
Caller->>Channel: await updateMessage(msg, op, params)
Channel->>RestMixin: updateDeleteMessage({isDelete:false}, msg, op, params)
RestMixin->>Server: HTTP PUT/POST (update/delete)
Server-->>RestMixin: 200 OK (message payload)
note right of RestMixin `#d6f5e0`: previously decoded and returned Message\nnow resolves without value
RestMixin-->>Channel: resolve void
Channel-->>Caller: resolve void
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/common/lib/client/restchannelmixin.ts (1)
102-149: Return-type change toPromise<void>is consistent; only small cleanups possible
updateDeleteMessagenow correctly exposes aPromise<void>while still awaiting the underlyingResource.post/patchso callers get proper error propagation. That matches the public API change and the tests that no longer depend on a returnedMessage.If you want to tighten things further, you could:
- Drop the unused
WireMessagegeneric frommethod<WireMessage>(...)(or change it tounknown) since the response body is ignored.- Optionally tweak the “cannot be updated” error string to mention deletes as well, now that this helper backs both operations.
ably.d.ts (2)
2836-2852: Channel typings and docs now match the void-return behaviourThe
Channel.updateMessage/deleteMessageJSDoc and signatures have been correctly updated toPromise<void>, avoiding any guarantee of a returnedMessagewhile still documenting the operation semantics.If you want to make the usage pattern clearer, you could add a brief note that callers should use
getMessageorgetMessageVersionsto fetch the resulting message state after an update or delete.
3079-3100: RealtimeChannel typings correctly aligned with REST counterparts
RealtimeChannel.updateMessageanddeleteMessagenow also returnPromise<void>in the public typings, with @returns text consistent with the new contract and matching the underlying implementation.As with the REST Channel interface, consider a short hint in the docs pointing developers to
getMessage/getMessageVersionsif they need the updated message object, but the current text is already accurate for this interim API.
📜 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.
📒 Files selected for processing (5)
ably.d.ts(2 hunks)src/common/lib/client/realtimechannel.ts(2 hunks)src/common/lib/client/restchannel.ts(2 hunks)src/common/lib/client/restchannelmixin.ts(2 hunks)test/rest/updates-deletes.test.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
test/rest/updates-deletes.test.js (2)
src/common/lib/client/realtimechannel.ts (1)
updateMessage(1026-1034)src/common/lib/client/restchannel.ts (1)
updateMessage(159-166)
src/common/lib/client/restchannelmixin.ts (1)
src/common/lib/types/message.ts (1)
WireMessage(176-228)
🪛 GitHub Actions: Lint
src/common/lib/client/realtimechannel.ts
[warning] 1-1: Code style issues found by Prettier. Run 'prettier --write' to fix.
src/common/lib/client/restchannel.ts
[warning] 1-1: Code style issues found by Prettier. Run 'prettier --write' to fix.
⏰ 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). (7)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (16.x)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (18.x)
- GitHub Check: test-npm-package
🔇 Additional comments (5)
test/rest/updates-deletes.test.js (1)
121-175: Tests correctly updated to no longer rely on a return valueUsing
await channel.updateMessage(...)/await channel.deleteMessage(...)and then verifying viagetMessagekeeps the tests aligned with the newPromise<void>contract while still asserting the resulting message state thoroughly.src/common/lib/client/realtimechannel.ts (2)
1026-1044: Realtime update/delete wrappers correctly aligned with new void contractChanging both
updateMessageanddeleteMessageto returnPromise<void>while still delegating torestMixin.updateDeleteMessagekeeps realtime behaviour intact and matches the new public typings and tests.
1-6: Prettier formatting issue resolvedThe file now passes Prettier validation checks. The formatting issue flagged in the lint pipeline has been successfully corrected.
src/common/lib/client/restchannel.ts (2)
159-175: REST channel update/delete APIs now correctly exposePromise<void>
RestChannel.updateMessageandRestChannel.deleteMessagenow just awaitchannelMixin.updateDeleteMessageand returnPromise<void>, matching the mixin and the public typings while preserving the underlying behaviour and logging.
1-8: Cannot conclusively verify CI formatting violations due to environment limitations.The visible code in lines 1-8 appears properly formatted: no trailing whitespace, line lengths are within reasonable limits, and import syntax is correct. However, the prettier tool fails to execute in this environment, and I have no access to actual CI reports. Please manually verify this against your CI logs and run
npm run format:checklocally to confirm whether violations still exist.
As agreed before the preview release. I am going to add a new publishResponse type, but it won't be a Message, so until we do that, I'm removing the Message response so we don't release a version of ably-js with that return type.
de06650 to
912fdde
Compare
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/common/lib/client/restchannelmixin.ts (1)
140-148: Response fetched but discarded (acceptable for temporary change).The HTTP request is still performed, but the response is no longer decoded or returned. This is slightly inefficient since the server still sends back a response body, but it's acceptable given the temporary nature of this change.
Optional: The generic type parameter
<WireMessage>is no longer needed since the response isn't used:- await method<WireMessage>( + await method( client, this.basePath(channel) + '/messages/' + encodeURIComponent(message.serial) + pathSuffix, requestBody, headers, params || {}, null, true, );
📜 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.
📒 Files selected for processing (5)
ably.d.ts(2 hunks)src/common/lib/client/realtimechannel.ts(1 hunks)src/common/lib/client/restchannel.ts(1 hunks)src/common/lib/client/restchannelmixin.ts(2 hunks)test/rest/updates-deletes.test.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/common/lib/client/realtimechannel.ts
- test/rest/updates-deletes.test.js
- ably.d.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/common/lib/client/restchannelmixin.ts (1)
src/common/lib/types/message.ts (1)
WireMessage(176-228)
src/common/lib/client/restchannel.ts (1)
ably.d.ts (2)
Message(3264-3322)MessageOperation(3447-3460)
⏰ 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). (7)
- GitHub Check: test-npm-package
- GitHub Check: test-browser (webkit)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (16.x)
🔇 Additional comments (3)
src/common/lib/client/restchannelmixin.ts (1)
102-108: LGTM: Return type correctly updated toPromise<void>.The signature change aligns with the PR objective to temporarily remove the Message response. The method still performs validation and constructs the request properly.
src/common/lib/client/restchannel.ts (2)
159-162: LGTM: Return type correctly updated toPromise<void>.The signature change is consistent with the updated
channelMixin.updateDeleteMessagemethod and aligns with the PR objective.
164-167: LGTM: Return type correctly updated toPromise<void>.The signature change is consistent with the updated
channelMixin.updateDeleteMessagemethod and aligns with the PR objective.
ttypic
left a comment
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.
LGTM
As agreed before the preview release. I am going to add a new publishResponse type, but it won't be a Message, so until we do that, I'm removing the Message response so we don't release a version of ably-js which has a return type that anyone might try and come to rely on.
spec pr: ably/specification#403
Summary by CodeRabbit
Breaking Changes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.