[PUB-1094] Infer key name of the LiveMap in its direct subscription update object#1995
Conversation
WalkthroughThis change set refines the type definitions for the LiveMap and LiveMapUpdate interfaces and their implementations. A generic type parameter Changes
Sequence Diagram(s)sequenceDiagram
participant LM as LiveMap
participant CB as Callback Handler
participant SW as Switch-Case Evaluator
LM->>CB: Emit update event (with update object)
CB->>CB: Extract key -> assign to typedKeyOnMap
CB->>SW: Pass typedKeyOnMap for evaluation
SW-->>CB: Return matched case (e.g., 'undefined' or default)
CB->>LM: Process update based on matched case
Possibly related PRs
Suggested reviewers
Poem
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Git: Failed to clone repository. Please run the ✨ Finishing Touches
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. 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/plugins/objects/livemap.ts (2)
50-51: Consider distinguishing “added” vs “updated” keys
Currently, theLiveMapUpdate<T>interface provides only'updated'and'removed'states inupdate. This may conflate genuinely new entries with modified ones, complicating downstream logic. Consider introducing a separate'added'state to better highlight new entries.- update: { [keyName in keyof T & string]?: 'updated' | 'removed' }; + update: { [keyName in keyof T & string]?: 'added' | 'updated' | 'removed' };
654-709: _applyMapSet always marks updates as “updated”
The method treats both genuinely new and modified keys as'updated', which can be acceptable in LWW. However, if you need to differentiate a brand-new key from a modified key, consider adding an'added'status. Also note that the method does not deep-compare the new vs. old data, so identical values will still emit'updated'. If that is desired, no further changes are necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ably.d.ts(2 hunks)src/plugins/objects/livemap.ts(10 hunks)test/package/browser/template/src/index-objects.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (20.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-node (16.x)
- GitHub Check: test-npm-package
🔇 Additional comments (13)
test/package/browser/template/src/index-objects.ts (3)
45-47: The update object now infers keys from the map type correctlyThe change from directly using
update.someKeyto first assigningupdate.stringKeyto a variabletypedKeyOnMapaligns with the improved typing in the LiveMapUpdate interface. This change correctly leverages the inference of keys from the map type, making the code more type-safe.
50-50: Added necessary handling for undefined caseAdding the
undefinedcase is important as it properly handles situations where a key might not be present in the update object, which can happen in real-world scenarios. This makes the switch statement more robust.
54-54: Updated exhaustiveness check to use the new variableThe exhaustiveness check has been properly updated to use the new
typedKeyOnMapvariable, ensuring that the TypeScript compiler will still enforce that all possible types are handled in the switch statement.ably.d.ts (3)
2302-2302: Enhanced type safety with generic parameter in LiveMap interfaceThe LiveMap interface has been updated to extend
LiveObject<LiveMapUpdate<T>>instead ofLiveObject<LiveMapUpdate>, ensuring that the LiveMapUpdate is parameterized with the same type T as the LiveMap. This change improves type safety by creating a stronger relationship between the map and its updates.
2362-2362: Added generic type parameter to LiveMapUpdate interfaceMaking the LiveMapUpdate interface accept a type parameter T that extends LiveMapType is a good improvement for type safety. This allows the update interface to be aware of the specific structure of the LiveMap it's associated with.
2368-2368: Implemented mapped type for more precise update property typingThe update property in LiveMapUpdate has been changed from a simple object with string keys to a mapped type that uses
keyof T & stringas keys. This is an excellent improvement as it ensures that only keys that exist in the LiveMap type can be used in the update object, along with their associated update statuses ('updated' or 'removed').src/plugins/objects/livemap.ts (7)
54-54: Great use of generics for improved type safety
LeveragingLiveMapUpdate<T>in the class signature elegantly enforces type constraints. No issues found here.
394-394: Variable declaration for combined update looks correct
Storing the combinedLiveMapUpdate<T>orLiveObjectUpdateNoopin a local variable ensures clarity when logic differentiates between a no-op and an actual update.
438-438: Method override for object state is well-typed
AcceptingObjectStateand returningLiveMapUpdate<T>orLiveObjectUpdateNoopfollows the new generic approach consistently.
529-583: _updateFromDataDiff logic is consistent with LWW semantics
This method scans differences betweenprevDataRefandnewDataRef, marking keys as'updated'or'removed'. The approach is clear and matches last-writer-wins (LWW) semantics. If you want to separately track newly added vs. modified keys, consider refining'updated'into multiple states. Otherwise, the current logic is perfectly valid for LWW.
585-619: Successively merging MAP_CREATE operations
Merging initial create operations into an aggregated update object is an elegant approach. It provides consolidated reporting of changes without spamming numerous discrete operations.
629-652: _applyMapCreate method properly checks second create attempts
By guarding with_createOperationIsMerged, the code ensures only one MAP_CREATE operation is applied. This helps avoid duplicate initializations. No changes required.
711-744: _applyMapRemove is straightforward and consistent
Removing a key sets its tombstone state and correctly emits a'removed'update. This is aligned with the new generic type approach.
2 participants
Resolves PUB-1094
Summary by CodeRabbit
Refactor
Bug Fixes