[PUB-1235] Add enumeration API to LiveMap and BatchContextLiveMap
#1981
Conversation
WalkthroughThis update enhances the LiveMap functionality by adding new iteration methods. The changes include code examples in the documentation and the introduction of three iterable methods— Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant B as BatchContextLiveMap
participant M as Internal Map
U->>B: Call entries()
B->>B: throwIfInvalidAccessApiConfiguration()
B->>B: throwIfClosed()
B->>M: Iterate over entries
M-->>B: Yield [key, value]
B-->>U: Return each key-value pair
sequenceDiagram
participant U as User
participant L as LiveMap
participant R as _getResolvedValueFromStateData
U->>L: Call get(key)
L->>R: Resolve state data for key
R-->>L: Return value (primitive or LiveObject)
L-->>U: Return the resolved value
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ast-grep (0.31.1)test/realtime/live_objects.test.js✨ Finishing Touches
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 (
|
github-actions
bot
temporarily deployed
to
staging/pull/1981/bundle-report
Resolves PUB-1235
VeskeR
force-pushed
the
PUB-1235/enumerate-map-keys
branch
from
22abbd1 to
24aba84
Compare
![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 (3)
README.md (1)
675-684: Clear and Concise Enumeration ExamplesThe newly added examples for iterating over the map using
.entries(),.keys(), and.values()are well aligned with the standard JavaScript Map API. The use of placeholder comments (/**/) keeps the examples succinct, although you might consider briefly commenting on the expected behavior or output for added clarity to users unfamiliar with these methods.ably.d.ts (1)
2317-2330: Interface enhancement for improved LiveMap iteration.The addition of the standard Map iteration methods (
entries(),keys(), andvalues()) to theLiveMapinterface is well-implemented, with proper TypeScript typing that ensures type safety during iteration.Consider adding JSDoc comments to these methods to document the iteration behavior with filtered elements, especially if the implementation excludes tombstoned entries or other special cases. Example:
/** * Returns an iterable of key/value pairs for every entry in the map. * Note: Tombstoned entries or deleted LiveObjects will not be included in the iteration. */ entries<TKey extends keyof T & string>(): IterableIterator<[TKey, T[TKey]]>;src/plugins/liveobjects/livemap.ts (1)
289-303: Consider caching size for performance.
This method correctly counts only non-tombstoned entries but runs in O(n). If the map can grow large orsize()is frequently called, caching and invalidating the size upon mutations could improve performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(1 hunks)ably.d.ts(1 hunks)src/plugins/liveobjects/batchcontextlivemap.ts(1 hunks)src/plugins/liveobjects/livemap.ts(2 hunks)test/realtime/live_objects.test.js(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- 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)
🔇 Additional comments (16)
src/plugins/liveobjects/batchcontextlivemap.ts (3)
31-35: Implementation of entries() method for LiveMap batch context.The implementation uses correct generator syntax and proper validation checks before iteration.
37-41: Implementation of keys() method for LiveMap batch context.The implementation uses correct generator syntax and proper validation checks before iteration.
43-47:Details
❓ Verification inconclusive
Implementation of values() method for LiveMap batch context.
The implementation uses correct generator syntax and proper validation checks before iteration.
One minor observation: The
get()method has special handling for LiveObject instances by wrapping them viagetWrappedObject(), but thevalues()method directly yields the map's values without similar wrapping. Is this intentional? Depending on the use case, it might be worth checking if LiveObjects should be wrapped consistently across both methods.
🏁 Script executed:
#!/bin/bash # Check for LiveObject wrapping in the implementation of LiveMap's values method # First, find the LiveMap class implementation to see how the values() method is implemented echo "Searching for LiveMap.values() implementation..." grep -A 15 "values.*IterableIterator" --include="*.ts" -r src/plugins/liveobjects/ # Also check for references to wrapping LiveObjects in iteration methods echo "Searching for LiveObject wrapping in iteration methods..." grep -A 5 -B 5 "getWrappedObject" --include="*.ts" -r src/plugins/liveobjects/ | grep -E "entries|keys|values"Length of output: 3201
Attention: Confirm Consistency in LiveObject Wrapping
- File:
src/plugins/liveobjects/batchcontextlivemap.ts(Lines: 43-47)- Observation: The
values()method correctly performs its validation checks and yields values directly from the map. In contrast, theget()method applies a special wrapping for LiveObject instances viagetWrappedObject(). The current implementation ofvalues()does not include such wrapping.- Action: Please verify whether the absence of wrapping in the
values()method is intentional. If ensuring a consistent behavior betweenget()andvalues()methods is desired, consider incorporating wrapping for the yielded LiveObjects.src/plugins/liveobjects/livemap.ts (6)
286-287: Non-null assertion is justified given prior tombstone and data existence checks.
This usage of the!operator is safe here, as the logic guaranteeselement.datais defined for non-tombstoned entries.
305-318: Generator-based entries() method is well-structured.
Skipping tombstoned entries and using_getResolvedValueFromStateDataaligns with the intended semantics. This approach cleanly mimics JavaScript’s native Map.
320-324: keys() method correctly reuses entries.
Yielding keys fromentries()is consistent with typical Map semantics and ensures consistent tombstone filtering.
326-330: values() method correctly reuses entries.
This approach ensures values reflect the same tombstone and resolved data handling.
793-816: _getResolvedValueFromStateData logic is solid.
Returningundefinedfor tombstoned or missing references is consistent with the user-facing contract. It neatly separates primitive vs. object reference handling.
817-834: _isMapEntryTombstoned centralizes tombstone checks.
Including a check for tombstoned referenced objects ensures the map’s external API won’t expose dead objects.test/realtime/live_objects.test.js (7)
3202-3213: Thorough testing of the new enumeration methodsGood addition of tests for the new LiveMap enumeration methods (.entries(), .keys(), and .values()). The tests verify both functionality and synchronous behavior in batch context.
3340-3342: Complete verification of enumeration methods in error casesProperly testing that enumeration methods throw the correct errors when the batch is closed.
3388-3399: Good refactoring of batch API error testingGreat refactoring of batch API error testing logic into reusable utility functions. This improves maintainability and readability of the tests.
3453-3577: Comprehensive testing of LiveMap and BatchContextLiveMap enumerationThe test scenarios thoroughly verify that:
- Enumeration methods work properly for both LiveMap and BatchContextLiveMap
- Tombstoned entries are correctly excluded from enumeration results
- The methods return the expected types and values
This provides good coverage for the new API methods that implement JavaScript Map-equivalent behavior.
3496-3510: Validation of tombstone handling in enumeration methodsGood test coverage of tombstone handling in enumeration methods. The test confirms that:
- size() doesn't count tombstoned entries
- entries() doesn't include tombstoned entries
- keys() and values() don't include keys or values from tombstoned entries
This is important for ensuring consistent behavior with JavaScript Map.
4304-4306: Complete coverage of access API error testingThe extended access API error testing now includes verification for the new enumeration methods, ensuring they throw appropriate errors in expected failure cases.
4340-4342: Complete coverage of batch API error testingProper testing of all batch context enumeration methods in error scenarios ensures they throw appropriate errors when the batch is closed.
Adds javascript Map's equivalents .keys(), .values(), .entries() to LiveMap and BatchContextLiveMap.
Resolves PUB-1235
Summary by CodeRabbit
New Features
Documentation