[DTP-958, DTP-959] Liveobjects/subscriptions #1917
Conversation
WalkthroughThe pull request introduces several modifications across multiple files to enhance the functionality of the LiveObjects system. Key changes include the addition of external dependencies in the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Warning Tool Failures:Tool Failure Count: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/1917/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1917/features
github-actions
bot
temporarily deployed
to
staging/pull/1917/typedoc
VeskeR
force-pushed
the
liveobjects/subscriptions
branch
from
b09f2f9 to
2f13d65
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1917/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1917/features
github-actions
bot
temporarily deployed
to
staging/pull/1917/typedoc
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (7)
src/plugins/liveobjects/liveobjects.ts (1)
200-215: Consider pre-allocating the updates array.The implementation correctly captures updates for batch processing. However, since we know the potential size from
_syncLiveObjectsDataPool, consider pre-allocating the array:-const existingObjectUpdates: { object: LiveObject; update: LiveObjectUpdate }[] = []; +const existingObjectUpdates: { object: LiveObject; update: LiveObjectUpdate }[] = + new Array(this._syncLiveObjectsDataPool.size());This could improve performance by avoiding array resizing during push operations.
src/plugins/liveobjects/liveobject.ts (2)
63-72: Consider removing the standalone 'unsubscribe' method in favor of the returned 'unsubscribe' function.Since the
subscribemethod returns anunsubscribefunction, the standaloneunsubscribemethod might be redundant.If the standalone
unsubscribemethod is necessary for other use cases, consider clarifying its purpose. Otherwise, you may remove it to simplify the API.-export abstract class LiveObject<...> { ... - unsubscribe(listener: (update: TUpdate) => void): void { - // implementation - } ... -}
74-76: Provide documentation for 'unsubscribeAll' to clarify its usage.Adding a doc comment will help users understand when to use
unsubscribeAll.Example:
+/** + * Unsubscribes all listeners from update events. + */ unsubscribeAll(): void {src/plugins/liveobjects/livecounter.ts (2)
10-12: Consider adding documentation toLiveCounterUpdateinterfaceAdding JSDoc comments to the
LiveCounterUpdateinterface will enhance code readability and help other developers understand the purpose of theupdateproperty.
78-79: Specify return type of_throwNoPayloadErrorasneverSince
_throwNoPayloadErroralways throws an error, consider changing its return type tonever. This allows TypeScript to understand that the function does not return, eliminating the need for explicitreturnstatements after it's called.To implement this, modify the method signature:
- private _throwNoPayloadError(op: StateOperation): void { + private _throwNoPayloadError(op: StateOperation): never {And remove the explicit
returnafter calling_throwNoPayloadError(op);:if (this._client.Utils.isNil(op.counterOp)) { this._throwNoPayloadError(op); - return; } else { update = this._applyCounterInc(op.counterOp); }Also applies to: 105-107
src/plugins/liveobjects/livemap.ts (2)
45-47: Add JSDoc comments to 'LiveMapUpdate' interfaceDocumenting the
LiveMapUpdateinterface with JSDoc comments enhances code readability and helps other developers understand its purpose and usage.Consider adding:
/** * Interface representing the update information for LiveMap. */ export interface LiveMapUpdate extends LiveObjectUpdate { /** * An object mapping keys to their update status ('updated' or 'deleted'). */ update: { [keyName: string]: 'updated' | 'deleted' }; }
195-243: Consider refactoring '_updateFromDataDiff' method for clarityThe
_updateFromDataDiffmethod is complex and could be refactored for better readability and maintainability. Breaking it into smaller helper functions or adding inline comments would improve understanding of the logic.🧰 Tools
🪛 Biome
[error] 238-238: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
grunt/esbuild/build.js(1 hunks)package.json(2 hunks)src/plugins/liveobjects/livecounter.ts(6 hunks)src/plugins/liveobjects/livemap.ts(10 hunks)src/plugins/liveobjects/liveobject.ts(4 hunks)src/plugins/liveobjects/liveobjects.ts(3 hunks)src/plugins/liveobjects/liveobjectspool.ts(0 hunks)test/realtime/live_objects.test.js(4 hunks)
💤 Files with no reviewable changes (1)
- src/plugins/liveobjects/liveobjectspool.ts
🧰 Additional context used
📓 Learnings (1)
src/plugins/liveobjects/livecounter.ts (1)
Learnt from: VeskeR
PR: ably/ably-js#1897
File: src/plugins/liveobjects/livecounter.ts:93-93
Timestamp: 2024-10-22T13:26:59.680Z
Learning: In the `LiveCounter` class's `_applyCounterCreate` method, it's intentional to increment the counter's value using `+=` instead of initializing it with `=` because the counter may have a pre-existing non-zero value.
🪛 Biome
src/plugins/liveobjects/livemap.ts
[error] 238-238: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
🔇 Additional comments (9)
grunt/esbuild/build.js (1)
85-85: LGTM! The external dependency configuration looks correct.
Adding 'deep-equal' as an external dependency aligns with the existing pattern used for other plugins and ensures the runtime dependency is handled appropriately.
package.json (2)
50-50: LGTM! Appropriate dependency choice for deep equality checks.
The addition of [email protected] is well-suited for implementing the subscription mechanism, as it provides reliable deep comparison capabilities needed for detecting state changes in live objects.
76-76: Verify the external module configuration in the build system.
The TypeScript types are correctly added as a dev dependency. Since deep-equal is configured as an external module in the build system, let's verify this configuration.
✅ Verification successful
deep-equal is properly configured as an external module
The verification confirms that deep-equal is correctly configured as an external module in the build system:
- Found in
grunt/esbuild/build.jswhere it's explicitly listed in theexternalarray - The module is properly imported in
src/plugins/liveobjects/livemap.ts - No bundling configuration in
package.jsonthat would override this setting
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify deep-equal is properly configured as external module
# Test: Check build configuration
echo "Checking build configuration for deep-equal..."
rg -l "deep-equal.*external" grunt/
Length of output: 140
Script:
#!/bin/bash
# Let's check different build configuration files for external module setup
echo "Checking webpack configuration..."
fd -e js -e ts webpack.config
echo -e "\nChecking rollup configuration..."
fd -e js -e ts rollup.config
echo -e "\nChecking any build related configuration..."
fd -e js -e ts -e json build.config
echo -e "\nChecking package bundling configuration in package.json..."
cat package.json | jq '.bundledDependencies, .bundleDependencies, .peerDependencies'
echo -e "\nChecking for deep-equal usage..."
rg "deep-equal" --type js --type ts -C 2
Length of output: 1269
src/plugins/liveobjects/liveobjects.ts (2)
7-7: LGTM: Import addition aligns with subscription feature.
The addition of LiveObjectUpdate type import supports the new subscription notification functionality.
240-242: Verify update notification behavior.
The batch processing of updates ensures notifications are sent only after all sync operations complete. Let's verify this behavior:
This will help ensure the update batching mechanism is consistently implemented and there are no bypass paths.
✅ Verification successful
Let me gather more information to verify the update notification behavior comprehensively.
Update notification behavior is correctly implemented
The verification confirms that the update notification mechanism is properly batched during sync operations:
- Updates are collected in
existingObjectUpdatesarray during sync processing - Notifications are sent only after all sync operations complete, as evidenced by the final
forEachcall - The implementation in
_applySync()ensures atomic updates by:- First collecting all updates
- Then removing extra objects
- Finally notifying subscribers in a single batch
The code follows a consistent pattern where individual LiveObject instances don't notify during sync processing, and instead, notifications are handled centrally after all changes are applied.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any direct notification calls during sync that might bypass the batching mechanism
# Test 1: Look for direct calls to notifyUpdated during sync
echo "Checking for direct notification calls during sync..."
rg "notifyUpdated.*sync" -B 5 -A 5
# Test 2: Verify the update batching pattern is consistent across the codebase
echo "Verifying update batching pattern..."
ast-grep --pattern 'existingObjectUpdates.push({ object: $_, update: $_ })'
Length of output: 314
Script:
#!/bin/bash
# Let's analyze the update notification pattern more broadly
# Check for all usages of notifyUpdated
echo "Checking all usages of notifyUpdated..."
rg "notifyUpdated" -B 5 -A 5
# Look for the existingObjectUpdates collection and its usage
echo "Checking existingObjectUpdates usage..."
rg "existingObjectUpdates" -B 10 -A 10
# Look for sync-related methods and their update handling
echo "Checking sync-related update handling..."
ast-grep --pattern 'sync($$$) {
$$$
}'
Length of output: 6958
src/plugins/liveobjects/liveobject.ts (1)
131-132: Implement the '_getZeroValueData' and '_updateFromDataDiff' methods or provide default implementations.
As these abstract methods are essential for the class functionality, ensure they are implemented in derived classes.
Please confirm that all subclasses of LiveObject implement these methods appropriately.
src/plugins/liveobjects/livecounter.ts (2)
Line range hint 114-139: Updates to _applyCounterCreate method are correct
The modifications to _applyCounterCreate properly handle the creation logic and return appropriate updates, accounting for the intentional use of += as per previous learnings.
142-144: Returning LiveCounterUpdate from _applyCounterInc is appropriate
The method now returns a LiveCounterUpdate, ensuring consistency in how updates are handled throughout the LiveCounter class.
test/realtime/live_objects.test.js (1)
1235-1748: Well-structured and comprehensive test scenarios for subscription callbacks
The newly added test cases under subscriptionCallbacksScenarios effectively cover various aspects of subscribing and unsubscribing to LiveCounter and LiveMap updates. The use of promises and async/await ensures proper synchronization, and the assertions are thorough, enhancing the reliability of these tests.
github-actions
bot
temporarily deployed
to
staging/pull/1917/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1917/features
github-actions
bot
temporarily deployed
to
staging/pull/1917/typedoc
VeskeR
force-pushed
the
liveobjects/subscriptions
branch
from
1456885 to
a5fc792
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1917/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1917/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1917/features
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
src/plugins/liveobjects/liveobject.ts (1)
15-17: Consider strengthening the type safety of LiveObjectUpdate.The
update: anytype is very permissive and could lead to type-safety issues. Consider using a generic type parameter or a more specific type union.-export interface LiveObjectUpdate { - update: any; -} +export interface LiveObjectUpdate<T = unknown> { + update: T; +}src/plugins/liveobjects/livecounter.ts (2)
78-79: Enhance the comment for better clarity.The current comment could better explain the TypeScript type narrowing purpose.
- // leave an explicit return here, so that TS knows that update object is always set after the switch statement. + // Explicit return for TypeScript control flow analysis: ensures 'update' variable is definitely assigned in all code paths
Line range hint
114-139: Enhance documentation for the counter initialization logic.While the implementation is correct (as confirmed by previous learnings), the comment could better explain the rationale for summing the initial value.
- // note that it is intentional to SUM the incoming count from the create op. - // if we get here, it means that current counter instance wasn't initialized from the COUNTER_CREATE op, - // so it is missing the initial value that we're going to add now. + // We intentionally ADD (not assign) the incoming count from the create op because: + // 1. This code path means the counter wasn't initialized via COUNTER_CREATE + // 2. The counter might have accumulated values from other operations + // 3. The initial value needs to be incorporated while preserving existing changessrc/plugins/liveobjects/livemap.ts (2)
198-246: Clean up unnecessary continue statement and consider extracting comparison logic.The diff comparison logic is well-implemented, but there are a few improvements we can make:
- Remove the unnecessary continue statement on line 241 (as flagged by static analysis)
- Consider extracting the entry comparison logic into a separate method for better readability
Apply this diff to remove the unnecessary continue:
const valueChanged = !deepEqual(currentEntry.data, newEntry.data, { strict: true }); if (valueChanged) { update.update[key] = 'updated'; - continue; }🧰 Tools
🪛 Biome
[error] 241-241: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
Line range hint
256-297: Consider performance optimization for large maps.The implementation correctly handles map creation and merging, but for large maps, the repeated Object.assign operations in the loop could impact performance.
Consider collecting updates in an array during iteration and performing a single Object.assign at the end:
const updates: Record<string, 'updated' | 'deleted'>[] = []; // ... in the loop updates.push(update.update); // ... after the loop Object.assign(aggregatedUpdate.update, ...updates);scripts/moduleReport.ts (1)
41-45: Consider adding JSDoc comments for better documentation.While the implementation is solid, adding JSDoc comments to the
PluginInfointerface would improve code documentation and help future maintainers understand the purpose of each field.Example improvement:
+/** + * Represents metadata for a buildable plugin. + * @interface PluginInfo + * @property {string} description - Human-readable description of the plugin + * @property {string} path - Path to the plugin's entry point + * @property {string[]} [external] - Optional list of external dependencies to exclude from the bundle + */ interface PluginInfo { description: string; path: string; external?: string[]; }test/realtime/live_objects.test.js (1)
1235-1748: Well-structured test suite with room for additional coverage.The subscription callback scenarios are comprehensive and well-implemented. However, consider enhancing the test coverage with:
- Negative test cases (e.g., invalid subscription parameters)
- Error handling scenarios in subscription callbacks
- Race condition tests for concurrent subscriptions/unsubscriptions
Example test scenario for error handling:
{ description: 'handles errors in subscription callbacks gracefully', action: async (ctx) => { const { root, sampleCounterKey } = ctx; const counter = root.get(sampleCounterKey); counter.subscribe(() => { throw new Error('Simulated error in callback'); }); // Verify the error doesn't break subsequent updates // Add assertions here } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
scripts/moduleReport.ts(7 hunks)src/plugins/liveobjects/livecounter.ts(6 hunks)src/plugins/liveobjects/livemap.ts(11 hunks)src/plugins/liveobjects/liveobject.ts(4 hunks)src/plugins/liveobjects/liveobjects.ts(3 hunks)src/plugins/liveobjects/liveobjectspool.ts(0 hunks)test/common/globals/named_dependencies.js(1 hunks)test/realtime/live_objects.test.js(4 hunks)
💤 Files with no reviewable changes (1)
- src/plugins/liveobjects/liveobjectspool.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/plugins/liveobjects/liveobjects.ts
🧰 Additional context used
📓 Learnings (1)
src/plugins/liveobjects/livecounter.ts (1)
Learnt from: VeskeR
PR: ably/ably-js#1897
File: src/plugins/liveobjects/livecounter.ts:93-93
Timestamp: 2024-10-22T13:26:59.680Z
Learning: In the `LiveCounter` class's `_applyCounterCreate` method, it's intentional to increment the counter's value using `+=` instead of initializing it with `=` because the counter may have a pre-existing non-zero value.
🪛 Biome
src/plugins/liveobjects/livemap.ts
[error] 241-241: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
🔇 Additional comments (22)
src/plugins/liveobjects/liveobject.ts (9)
2-2: Fix EventEmitter import.
The previous review comment about importing EventEmitter without the 'type' modifier is still valid.
7-9: LGTM! Well-defined event enum.
The LiveObjectEvents enum follows TypeScript best practices with clear naming and single responsibility.
19-23: LGTM! Well-designed noop interface.
The LiveObjectUpdateNoop interface is well-designed with:
- Clear documentation explaining the undefined update field
- Explicit noop flag for type discrimination
29-32: LGTM! Well-implemented generic constraints.
The generic type parameters with constraints ensure type safety while maintaining flexibility.
53-61: LGTM! Clean subscription implementation.
The subscribe method follows best practices:
- Returns a clean unsubscribe interface
- Properly manages event listener lifecycle
- Aligns with the PR objectives for direct object subscription
63-72: Fix the null check implementation.
The previous review comment about replacing Utils.isNil with a direct comparison is still valid.
97-100: LGTM! Clear data update pattern.
The setData method properly:
- Generates update diff before changing state
- Returns the update object as specified in PR objectives
113-120: Improve type checking in notifyUpdated.
The previous review comment about using a type guard instead of type assertion is still valid.
131-132: LGTM! Well-defined abstract contract.
The abstract methods create a clear contract for derived classes:
_getZeroValueData: For initialization_updateFromDataDiff: For generating structured updates
src/plugins/liveobjects/livecounter.ts (3)
Line range hint 1-14: LGTM! Well-structured type definitions.
The new LiveCounterUpdate interface and updated class signature provide a clear contract for counter updates, which aligns well with the subscription mechanism objectives.
101-104: LGTM! Clean implementation of diff calculation.
The method correctly computes and structures counter updates, maintaining consistency with the LiveCounterUpdate interface.
142-144: LGTM! Clean and focused implementation.
The method correctly applies and structures the increment operation.
src/plugins/liveobjects/livemap.ts (4)
Line range hint 1-47: LGTM! Well-structured interface definition.
The new LiveMapUpdate interface properly extends LiveObjectUpdate and provides a clear structure for tracking map entry changes through the 'updated' or 'deleted' status.
Line range hint 156-191: LGTM! Good type safety improvements.
The changes properly handle update objects and include necessary explicit returns for TypeScript type inference. This ensures that the update variable is always defined before use.
Line range hint 300-350: LGTM! Robust implementation of Last-Write-Wins semantics.
The method properly handles:
- Timeserial comparison for conflict resolution
- Invalid data validation
- Zero-value object creation for referenced objects
Line range hint 353-382: LGTM! Consistent implementation with _applyMapSet.
The removal logic properly maintains consistency with the set operations, including:
- Timeserial-based conflict resolution
- Proper handling of non-existent entries
- Clear logging for operation skipping
scripts/moduleReport.ts (5)
41-50: Well-structured plugin management implementation!
The new PluginInfo interface and buildablePlugins constant provide a clean and type-safe way to manage plugin metadata and their external dependencies. This implementation aligns well with the modular architecture and makes plugin management more maintainable.
Line range hint 85-99: LGTM: Clean implementation of external dependency handling!
The addition of the optional external parameter to getBundleInfo enables proper tree-shaking of external dependencies while maintaining backward compatibility.
198-200: Good refactor: Improved plugin size calculation!
The update to use PluginInfo type simplifies the function signature while adding support for external dependencies in bundle size calculations.
215-215: Nice simplification of plugin size calculations!
The updates to calculatePushPluginSize and calculateLiveObjectsPluginSize leverage the new plugin management structure, making the code more maintainable.
Also applies to: 219-219
306-307: Consistent implementation of plugin file checks!
The updates to checkPushPluginFiles and checkLiveObjectsPluginFiles maintain consistency with the new plugin management structure while properly handling external dependencies.
Also applies to: 321-322
test/realtime/live_objects.test.js (1)
565-565: LGTM! Well-structured test setup code.
The code follows good testing practices by:
- Creating prerequisite objects first
- Using clear variable names
- Having proper verification steps
VeskeR
force-pushed
the
liveobjects/subscriptions
branch
from
a5fc792 to
51edafc
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1917/features
github-actions
bot
temporarily deployed
to
staging/pull/1917/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1917/bundle-report
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
scripts/moduleReport.ts (1)
307-307: Consider extracting the threshold value into a constantThe threshold value of 100 bytes is used in multiple places. Consider extracting it into a named constant at the top of the file for better maintainability.
+ const FILE_SIZE_THRESHOLD_BYTES = 100; - return checkBundleFiles(pushPluginBundleInfo, allowedFiles, 100); + return checkBundleFiles(pushPluginBundleInfo, allowedFiles, FILE_SIZE_THRESHOLD_BYTES); - return checkBundleFiles(pluginBundleInfo, allowedFiles, 100); + return checkBundleFiles(pluginBundleInfo, allowedFiles, FILE_SIZE_THRESHOLD_BYTES);Also applies to: 322-322
test/realtime/live_objects.test.js (2)
565-565: Consider adding error handling for state requests.The state request operations could fail and should include proper error handling.
Consider wrapping the state requests in try-catch blocks:
- const { objectId: referencedMapObjectId } = await liveObjectsHelper.stateRequest( - channelName, - liveObjectsHelper.mapCreateOp({ entries: { stringKey: { data: { value: 'stringValue' } } } }), - ); + let referencedMapObjectId; + try { + const response = await liveObjectsHelper.stateRequest( + channelName, + liveObjectsHelper.mapCreateOp({ entries: { stringKey: { data: { value: 'stringValue' } } } }), + ); + referencedMapObjectId = response.objectId; + } catch (error) { + console.error('Failed to create referenced map:', error); + throw error; + }
1235-1248: Improve test readability with more descriptive variable names.The variable names could be more descriptive to better convey their purpose.
Consider renaming:
subscriptionPromise→counterUpdatePromiseupdate→counterUpdate
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
scripts/moduleReport.ts(7 hunks)test/common/globals/named_dependencies.js(1 hunks)test/realtime/live_objects.test.js(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/common/globals/named_dependencies.js
🔇 Additional comments (5)
scripts/moduleReport.ts (5)
41-50: LGTM: Well-structured plugin metadata management
The new PluginInfo interface and buildablePlugins constant provide a clean and type-safe way to manage plugin metadata, including their external dependencies.
Line range hint 85-99: LGTM: Clean addition of external dependencies support
The getBundleInfo function has been properly updated to support external dependencies while maintaining backward compatibility.
198-200: LGTM: Clean refactor to use PluginInfo
The calculatePluginSize function has been properly updated to use the new PluginInfo type, making it more type-safe and maintainable.
215-215: LGTM: Clean updates to plugin size calculations
The plugin size calculation functions have been properly updated to use the new buildablePlugins structure.
Also applies to: 219-219
306-307: LGTM: Consistent updates to plugin file checks
The plugin file checks have been properly updated to use the new buildablePlugins structure, maintaining consistency across the codebase.
Also applies to: 321-322
| const counter = root.get(sampleCounterKey); | ||
| const expectedCounterIncrements = [100, -100, Number.MAX_SAFE_INTEGER, -Number.MAX_SAFE_INTEGER]; | ||
| let currentUpdateIndex = 0; | ||
|
|
||
| const subscriptionPromise = new Promise((resolve, reject) => | ||
| counter.subscribe((update) => { | ||
| try { | ||
| const expectedInc = expectedCounterIncrements[currentUpdateIndex]; | ||
| expect(update).to.deep.equal( | ||
| { update: { inc: expectedInc } }, | ||
| `Check counter subscription callback is called with an expected update object for ${currentUpdateIndex + 1} times`, | ||
| ); | ||
|
|
||
| if (currentUpdateIndex === expectedCounterIncrements.length - 1) { | ||
| resolve(); | ||
| } | ||
|
|
||
| currentUpdateIndex++; | ||
| } catch (error) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout handling for subscription promises.
The subscription promises should include timeouts to prevent tests from hanging indefinitely.
Consider adding a timeout wrapper:
- const subscriptionPromise = new Promise((resolve, reject) =>
+ const subscriptionPromise = Promise.race([
+ new Promise((resolve, reject) => {
counter.subscribe((update) => {
try {
const expectedInc = expectedCounterIncrements[currentUpdateIndex];
expect(update).to.deep.equal(
{ update: { inc: expectedInc } },
`Check counter subscription callback is called with an expected update object for ${currentUpdateIndex + 1} times`,
);
if (currentUpdateIndex === expectedCounterIncrements.length - 1) {
resolve();
}
currentUpdateIndex++;
} catch (error) {
reject(error);
}
});
+ }),
+ new Promise((_, reject) =>
+ setTimeout(() => reject(new Error('Subscription timeout')), 5000)
+ )
+ ]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const counter = root.get(sampleCounterKey); | |
| const expectedCounterIncrements = [100, -100, Number.MAX_SAFE_INTEGER, -Number.MAX_SAFE_INTEGER]; | |
| let currentUpdateIndex = 0; | |
| const subscriptionPromise = new Promise((resolve, reject) => | |
| counter.subscribe((update) => { | |
| try { | |
| const expectedInc = expectedCounterIncrements[currentUpdateIndex]; | |
| expect(update).to.deep.equal( | |
| { update: { inc: expectedInc } }, | |
| `Check counter subscription callback is called with an expected update object for ${currentUpdateIndex + 1} times`, | |
| ); | |
| if (currentUpdateIndex === expectedCounterIncrements.length - 1) { | |
| resolve(); | |
| } | |
| currentUpdateIndex++; | |
| } catch (error) { | |
| const counter = root.get(sampleCounterKey); | |
| const expectedCounterIncrements = [100, -100, Number.MAX_SAFE_INTEGER, -Number.MAX_SAFE_INTEGER]; | |
| let currentUpdateIndex = 0; | |
| const subscriptionPromise = Promise.race([ | |
| new Promise((resolve, reject) => { | |
| counter.subscribe((update) => { | |
| try { | |
| const expectedInc = expectedCounterIncrements[currentUpdateIndex]; | |
| expect(update).to.deep.equal( | |
| { update: { inc: expectedInc } }, | |
| `Check counter subscription callback is called with an expected update object for ${currentUpdateIndex + 1} times`, | |
| ); | |
| if (currentUpdateIndex === expectedCounterIncrements.length - 1) { | |
| resolve(); | |
| } | |
| currentUpdateIndex++; | |
| } catch (error) { | |
| reject(error); | |
| } | |
| }); | |
| }), | |
| new Promise((_, reject) => | |
| setTimeout(() => reject(new Error('Subscription timeout')), 5000) | |
| ) | |
| ]); |
VeskeR
force-pushed
the
liveobjects/subscriptions
branch
from
51edafc to
9826b58
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1917/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1917/features
github-actions
bot
temporarily deployed
to
staging/pull/1917/typedoc
src/plugins/liveobjects/livemap.ts
Outdated
| @@ -40,7 +42,11 @@ export interface LiveMapData extends LiveObjectData { | |||
| data: Map<string, MapEntry>; | |||
| } | |||
|
|
|||
| export class LiveMap extends LiveObject<LiveMapData> { | |||
| export interface LiveMapUpdate extends LiveObjectUpdate { | |||
| update: { [keyName: string]: 'updated' | 'deleted' }; | |||
There was a problem hiding this comment.
shall we call this update type removed instead of deleted, to match the internal name we use for the op? (I think we may have discussed this before but I can't remember the outcome)
There was a problem hiding this comment.
yea, we decided to keep parity with realtime, so removed.
this update object in the subscription api DR just had it still as deleted, so I used that.
I've updated the DR and will change it here to removed too.
There was a problem hiding this comment.
| } | ||
|
|
||
| // if tombstones are matching, need to compare values with deep equals to see if it was changed | ||
| const valueChanged = !deepEqual(currentEntry.data, newEntry.data, { strict: true }); |
There was a problem hiding this comment.
If objects are both tombstones, is that really an update?
There was a problem hiding this comment.
assuming MAP_REMOVE op doesn't send a data field for some reason, it is going to be undefined, just as the local data. so valueChanged = undefined !== undefined; // false, there will be no update.
however, I can make it as an explicit case just to make sure it always behaves like that
There was a problem hiding this comment.
VeskeR
force-pushed
the
liveobjects/timeserials-comparison-change
branch
from
e763a30 to
868560c
Compare
Also configure LiveObjects plugin default bundle to not include `deep-equal` package in the output.
VeskeR
force-pushed
the
liveobjects/subscriptions
branch
from
9826b58 to
e0fe4fa
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1917/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1917/features
github-actions
bot
temporarily deployed
to
staging/pull/1917/typedoc
VeskeR
force-pushed
the
liveobjects/subscriptions
branch
from
e0fe4fa to
cf43cb2
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1917/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1917/features
github-actions
bot
temporarily deployed
to
staging/pull/1917/typedoc
- subscription callback is invoked when the live object data is updated via incoming state operation - subscription callback is invoked when the live object data is updated via a sync sequence (once sync sequence is applied to all objects) - update object is passed to a callback function that describes a granular update made to the live object Resolves DTP-958
Resolves DTP-959
Also add `deep-equal` to the list of named dependencies for tests so that browser tests know where to get this package.
VeskeR
force-pushed
the
liveobjects/subscriptions
branch
from
cf43cb2 to
4bca1c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (11)
src/plugins/liveobjects/liveobject.ts (2)
53-76: Consider using type predicates for safer type checking.The subscription methods are well-implemented, but the type checking in
unsubscribecould be more TypeScript-idiomatic.Consider this improvement:
-if (this._client.Utils.isNil(listener)) { +const isNilListener = ( + listener: ((update: TUpdate) => void) | null | undefined +): listener is null | undefined => { + return this._client.Utils.isNil(listener); +}; + +if (isNilListener(listener)) {This change:
- Adds type safety through TypeScript type predicates
- Makes the code more maintainable and self-documenting
- Preserves the existing functionality
93-100: Document the order dependency in setData.The order of operations in
setDatais crucial - calculating the update before updating the reference. This dependency should be documented to prevent future maintenance issues.Add a comment explaining the order dependency:
setData(newDataRef: TData): TUpdate { + // Calculate update before updating reference to ensure correct diff calculation const update = this._updateFromDataDiff(this._dataRef, newDataRef); this._dataRef = newDataRef; return update; }src/plugins/liveobjects/livecounter.ts (2)
Line range hint
69-94: Consider simplifying the COUNTER_INC error handling.The error handling logic could be more concise by combining the error check and throw into a single statement.
Consider this refactor:
case StateOperationAction.COUNTER_INC: - if (this._client.Utils.isNil(op.counterOp)) { - this._throwNoPayloadError(op); - // leave an explicit return here, so that TS knows that update object is always set after the switch statement. - return; - } else { - update = this._applyCounterInc(op.counterOp); - } + if (this._client.Utils.isNil(op.counterOp)) this._throwNoPayloadError(op); + update = this._applyCounterInc(op.counterOp!); break;The
!assertion is safe here because_throwNoPayloadErroralways throws.
Line range hint
114-139: Consider clarifying the comment about initial value summation.While the implementation is correct (as confirmed by previous learnings), the comment could better explain why we sum the initial value instead of setting it directly.
Consider updating the comment to:
- // note that it is intentional to SUM the incoming count from the create op. - // if we get here, it means that current counter instance wasn't initialized from the COUNTER_CREATE op, - // so it is missing the initial value that we're going to add now. + // We intentionally SUM (+=) the incoming count instead of setting (=) it because: + // 1. This counter instance wasn't initialized from a COUNTER_CREATE op + // 2. The current value might include increments that occurred before this create op + // 3. Adding the initial value preserves these pre-existing incrementssrc/plugins/liveobjects/livemap.ts (3)
Line range hint
156-191: Consider consistent error handling pattern.While the explicit returns after error cases are good for type safety, consider extracting the error handling to maintain consistency:
- if (this._client.Utils.isNil(op.mapOp)) { - this._throwNoPayloadError(op); - // leave an explicit return here, so that TS knows that update object is always set after the switch statement. - return; - } else { - update = this._applyMapSet(op.mapOp, DefaultTimeserial.calculateTimeserial(this._client, msg.serial)); - } + if (this._client.Utils.isNil(op.mapOp)) { + this._throwNoPayloadError(op); + } + update = this._applyMapSet(op.mapOp!, DefaultTimeserial.calculateTimeserial(this._client, msg.serial));The
_throwNoPayloadErrormethod already throws, so the explicit return is redundant. Using non-null assertion after the check maintains type safety while simplifying the code.
236-239: Simplify tombstone comparison.The tombstone comparison could be simplified as it's already known both entries exist.
- if (currentEntry.tombstone === true && newEntry.tombstone === true) { - // both props are tombstoned - treat as noop, as there is no data to compare. - continue; - } + if (currentEntry.tombstone && newEntry.tombstone) { + return; // both tombstoned, no data to compare + }
241-246: Remove unnecessary continue statement.The continue statement at the end of the if block is unnecessary as it's the last statement in the loop iteration.
const valueChanged = !deepEqual(currentEntry.data, newEntry.data, { strict: true }); if (valueChanged) { update.update[key] = 'updated'; - continue; }🧰 Tools
🪛 Biome
[error] 245-245: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
scripts/moduleReport.ts (1)
Line range hint
40-322: Excellent architectural improvements to plugin managementThe changes introduce a well-structured approach to plugin management with several key benefits:
- Centralized plugin metadata management through
PluginInfointerface- Consistent handling of external dependencies across the build system
- Improved maintainability through standardized plugin configuration
- Preserved backward compatibility while enhancing functionality
This architectural improvement will make it easier to manage plugins and their dependencies as the system grows.
test/realtime/live_objects.test.js (3)
565-565: Add error handling for state requests.Consider adding error handling for the state requests to ensure tests fail gracefully when operations fail.
- const { objectId: referencedMapObjectId } = await liveObjectsHelper.stateRequest( + const { objectId: referencedMapObjectId } = await liveObjectsHelper.stateRequest( channelName, liveObjectsHelper.mapCreateOp({ entries: { stringKey: { data: { value: 'stringValue' } } } }), - ); + ).catch(error => { + throw new Error(`Failed to create referenced map: ${error.message}`); + });
1457-1699: Consider adding negative test cases for unsubscribe functionality.The unsubscribe tests are thorough for the happy path, but consider adding test cases for:
- Unsubscribing a non-existent listener
- Multiple unsubscribe calls on the same listener
- Unsubscribe during callback execution
Example structure:
it('handles unsubscribe of non-existent listener gracefully', async () => { const counter = root.get(sampleCounterKey); const randomListener = () => {}; // Should not throw counter.unsubscribe(randomListener); });
Line range hint
1-1748: Consider reorganizing tests for better maintainability.The test file is well-structured but quite large. Consider:
- Splitting the test suites into separate files by functionality (e.g., counter_operations.test.js, map_operations.test.js)
- Creating shared test utilities for common setup/teardown operations
- Using a test fixture factory pattern for creating test objects
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
grunt/esbuild/build.js(1 hunks)package.json(2 hunks)scripts/moduleReport.ts(7 hunks)src/plugins/liveobjects/livecounter.ts(6 hunks)src/plugins/liveobjects/livemap.ts(12 hunks)src/plugins/liveobjects/liveobject.ts(4 hunks)src/plugins/liveobjects/liveobjects.ts(3 hunks)src/plugins/liveobjects/liveobjectspool.ts(0 hunks)test/common/globals/named_dependencies.js(1 hunks)test/realtime/live_objects.test.js(4 hunks)
💤 Files with no reviewable changes (1)
- src/plugins/liveobjects/liveobjectspool.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- grunt/esbuild/build.js
- package.json
- src/plugins/liveobjects/liveobjects.ts
- test/common/globals/named_dependencies.js
🧰 Additional context used
📓 Learnings (2)
src/plugins/liveobjects/livecounter.ts (1)
Learnt from: VeskeR
PR: ably/ably-js#1897
File: src/plugins/liveobjects/livecounter.ts:93-93
Timestamp: 2024-10-22T13:26:59.680Z
Learning: In the `LiveCounter` class's `_applyCounterCreate` method, it's intentional to increment the counter's value using `+=` instead of initializing it with `=` because the counter may have a pre-existing non-zero value.
src/plugins/liveobjects/liveobject.ts (1)
Learnt from: VeskeR
PR: ably/ably-js#1917
File: src/plugins/liveobjects/liveobject.ts:2-2
Timestamp: 2024-11-08T05:28:30.955Z
Learning: In `src/plugins/liveobjects/liveobject.ts`, `EventEmitter` is accessed via the client object, so importing it with the `type` modifier is acceptable.
🪛 Biome
src/plugins/liveobjects/livemap.ts
[error] 245-245: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
🔇 Additional comments (17)
src/plugins/liveobjects/liveobject.ts (4)
7-27: LGTM! Well-structured interfaces for update handling.
The interfaces are well-designed with:
- Clear separation between regular updates and no-op updates
- Type safety through the
undefinedconstraint inLiveObjectUpdateNoop - A clean subscription response interface
29-46: LGTM! Well-structured class with proper generic constraints.
The class implementation shows good practices:
- Generic type parameters with appropriate constraints
- Proper initialization of EventEmitter through client
- Correct handling of optional constructor parameters
131-135: LGTM! Well-documented abstract methods.
The abstract methods are clearly defined with:
- Descriptive parameter names
- Clear documentation
- Proper TypeScript typing
110-120: 🛠️ Refactor suggestion
Use type guard for safer no-op detection.
The current type assertion could be replaced with a type guard for better type safety.
Apply this improvement:
-notifyUpdated(update: TUpdate | LiveObjectUpdateNoop): void {
- if ((update as LiveObjectUpdateNoop).noop) {
+notifyUpdated(update: TUpdate | LiveObjectUpdateNoop): void {
+ const isNoopUpdate = (
+ update: TUpdate | LiveObjectUpdateNoop
+ ): update is LiveObjectUpdateNoop => {
+ return 'noop' in update && update.noop === true;
+ };
+
+ if (isNoopUpdate(update)) {Likely invalid or redundant comment.
src/plugins/liveobjects/livecounter.ts (3)
Line range hint 1-14: LGTM! Well-structured type definitions.
The new LiveCounterUpdate interface and updated class declaration provide a clear contract for counter updates, aligning well with the subscription notification requirements.
101-104: LGTM! Clean diff computation.
The _updateFromDataDiff method provides a clear way to compute and structure counter updates, which is essential for the subscription mechanism.
142-144: LGTM! Clear and concise increment handling.
The _applyCounterInc method correctly updates the counter and returns a properly structured update object.
src/plugins/liveobjects/livemap.ts (2)
45-49: LGTM! Clean interface design for subscription updates.
The LiveMapUpdate interface and updated class signature provide a clear contract for subscription updates, using descriptive update types that align with the operation semantics.
Line range hint 260-386: LGTM! Well-structured operation methods with proper update handling.
The operation methods (_applyMapCreate, _applyMapSet, _applyMapRemove) are well-implemented with:
- Proper timeserial validation and comparison
- Consistent update structure
- Clear error messages with context
- Appropriate handling of edge cases
scripts/moduleReport.ts (5)
40-50: Well-structured plugin management implementation!
The new PluginInfo interface and buildablePlugins constant provide a clean and maintainable way to manage plugin metadata and their external dependencies. This structure will scale well as more plugins are added.
Line range hint 85-99: LGTM - Clean implementation of external dependency support
The addition of the optional external parameter maintains backward compatibility while enabling granular control over external dependencies in the bundle.
198-200: Good refactor - Improved function signature with PluginInfo
The function now accepts a single PluginInfo parameter instead of separate options, making it more maintainable and type-safe.
215-215: Clean implementation using the new plugin structure
The plugin-specific calculation functions are now more concise and maintainable by leveraging the buildablePlugins constant.
Also applies to: 219-219
306-307: Good use of destructuring for plugin configuration
The implementation cleanly extracts plugin configuration using destructuring and maintains consistency across different plugin checks.
Also applies to: 321-322
test/realtime/live_objects.test.js (3)
1235-1236: LGTM! Well-structured test scenarios.
The subscription scenarios are clearly defined with descriptive names.
1237-1309: Previous timeout handling comment still applies.
The subscription promise handling needs timeout protection as mentioned in the previous review.
1311-1455: Previous cleanup suggestion still applies.
The test isolation improvement suggestion from the previous review remains relevant.
Implements direct object subscription for Live Objects:
Resolves DTP-958, DTP-959
Summary by CodeRabbit
Release Notes
New Features
LiveCounterandLiveMapclasses for better update handling and subscription mechanisms.deep-equalmodule, improving equality checks.deep-equalfor both browser and Node.js environments.Bug Fixes
LiveObjectsPoolclass to manage unsupported actions and state messages more effectively.Tests
LiveCounterandLiveMapoperations, ensuring robust functionality and accurate updates.