-
Notifications
You must be signed in to change notification settings - Fork 56
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
Update Message
to support updateSerial
#1916
Conversation
WalkthroughThe changes involve modifications to the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
ably.d.ts
(1 hunks)src/common/lib/types/message.ts
(3 hunks)
🔇 Additional comments (4)
src/common/lib/types/message.ts (3)
410-410
: LGTM: String representation is consistent.
The toString()
implementation correctly uses the updateSerial
property name, maintaining consistency with the class property.
344-344
: Property declaration looks good but consider type constraints.
The updateSerial
optional string property aligns with the PR objectives. However, consider if this should be a required property or if there should be constraints on the string format for better type safety.
Let's check if there are any existing format constraints for serial properties:
Line range hint 344-410
: Verify backwards compatibility with message updates.
The change from deletedAt
to updateSerial
represents a significant change in how message updates are tracked. We should ensure this change is backward compatible and properly handled in message processing logic.
Let's analyze the impact:
ably.d.ts (1)
2359-2361
: LGTM: The updateSerial
property addition aligns with the PR objectives.
The addition of the updateSerial
property to the Message
interface is correctly typed as an optional string and properly documented. This change aligns with the PR's goal of replacing deletedAt
with updateSerial
for tracking message updates.
Added an updateSerial to allow LWW semantics to be applied to a message resulting from an operation. Removed the deletedAt timestamp, we will rely only on updateSerial to apply ordering of operations. UpdateAt will still be used, but just so clients can render a simple datetime and not have to understand/parse the serial.
5731271
to
21e42ea
Compare
To handle ordering of operation messages, we have added an
updateSerial
field to theMessage
type.We will now use this new field and
updatedAt
to define the global and total order of operations (updates/deletes)Summary by CodeRabbit
New Features
updateSerial
in the Message interface.deletedAt
property.Documentation