Add LiveObjects package test #1921
Conversation
WalkthroughThe changes introduced in this pull request enhance the Ably TypeScript definitions by adding several new interfaces and methods related to the Changes
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. 🔧 eslint (1.23.1)
warning eslint@7.32.0: This version is no longer supported. Please see https://eslint.org/version-support for other options. 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/1921/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1921/features
![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 (6)
ably.d.ts (2)
2035-2035: Reminder: Complete the documentation for the LiveObjects public APIThere's a TODO comment indicating that the public API for LiveObjects needs documentation. Please ensure that the documentation is completed before merging.
Would you like assistance in generating the documentation?
2038-2038: Clarify the return type of 'get' method in 'LiveMap'The
getmethod returnsLiveObject | StateValue | undefined, which may be ambiguous. Consider specifying more precise return types or overloading the method to improve type safety and developer experience.test/package/browser/template/package.json (1)
Line range hint
10-13: Consider adding a dedicated test script for LiveObjects.Since we're adding LiveObjects functionality, it would be beneficial to have a dedicated test script for LiveObjects tests, similar to how we have separate scripts for lib and hooks tests.
Consider adding this to the "scripts" section:
"test": "npm run test:lib && npm run test:hooks", "test:lib": "playwright test -c playwright-lib.config.js", "test:hooks": "playwright test -c playwright-hooks.config.ts", + "test:liveobjects": "playwright test -c playwright-liveobjects.config.ts", "test:install-deps": "playwright install chromium"test/package/browser/template/src/index-liveobjects.ts (2)
1-8: LGTM, but consider adding error handlingThe setup looks good with proper imports and initialization. However, consider adding error handling for the sandbox API key creation.
globalThis.testAblyPackage = async function () { - const key = await createSandboxAblyAPIKey(); + try { + const key = await createSandboxAblyAPIKey(); + } catch (error) { + console.error('Failed to create sandbox API key:', error); + throw error; + }
5-41: Consider splitting tests into smaller, focused test casesThe current implementation combines all tests into a single function. Consider splitting it into smaller, focused test cases for better maintainability and clearer failure isolation:
- Channel initialization
- LiveMap operations
- LiveMap subscriptions
- LiveCounter operations
This would make the tests more maintainable and easier to debug.
Would you like me to provide an example of how to structure these separate test cases?
🧰 Tools
🪛 Biome
[error] 27-27: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
test/package/browser/template/README.md (1)
29-30: Minor formatting suggestion for consistency.The build script documentation accurately describes the new bundles. Consider using semicolons instead of periods for items 2 and 3 to match the formatting of item 1:
- 2. a bundle containing `src/index-liveobjects.ts` and ably-js. - 3. a bundle containing `src/index-modular.ts` and ably-js. + 2. a bundle containing `src/index-liveobjects.ts` and ably-js; + 3. a bundle containing `src/index-modular.ts` and ably-js.🧰 Tools
🪛 LanguageTool
[uncategorized] ~29-~29: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...ntainingsrc/index-liveobjects.tsand ably-js. 3. a bundle containing `src/index-mo...(HYPHENATED_LY_ADVERB_ADJECTIVE)
[uncategorized] ~30-~30: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...e containingsrc/index-modular.tsand ably-js. -test: Using the bundles created by...(HYPHENATED_LY_ADVERB_ADJECTIVE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
ably.d.ts(1 hunks)test/package/browser/template/README.md(2 hunks)test/package/browser/template/package.json(1 hunks)test/package/browser/template/server/resources/index-liveobjects.html(1 hunks)test/package/browser/template/server/server.ts(1 hunks)test/package/browser/template/src/index-liveobjects.ts(1 hunks)test/package/browser/template/test/lib/package.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/package/browser/template/server/resources/index-liveobjects.html
🧰 Additional context used
🪛 LanguageTool
test/package/browser/template/README.md
[uncategorized] ~11-~11: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...liveobjects.ts imports the LiveObjects ably-js plugin (import LiveObjects from 'ably/...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
[uncategorized] ~29-~29: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...ntaining src/index-liveobjects.ts and ably-js. 3. a bundle containing `src/index-mo...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
[uncategorized] ~30-~30: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...e containing src/index-modular.ts and ably-js. - test: Using the bundles created by...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
🪛 Biome
test/package/browser/template/src/index-liveobjects.ts
[error] 27-27: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (3)
test/package/browser/template/server/server.ts (1)
8-8: LGTM! Verify the required files exist.
The addition of 'index-liveobjects.js' to the static files array follows the established pattern and supports the LiveObjects testing objective.
Let's verify the required files exist:
test/package/browser/template/package.json (1)
7-7: LGTM! Verify the build output.
The build script modification correctly includes the LiveObjects bundle while maintaining the existing bundling structure.
Let's verify the build output and bundle configuration:
✅ Verification successful
Let me gather more information about the source files to verify their existence and structure.
Let me check for the source files that are referenced in the build script.
Build script configuration is correct and source files are present
All three source files referenced in the build script exist in the correct location:
- test/package/browser/template/src/index-default.ts
- test/package/browser/template/src/index-liveobjects.ts
- test/package/browser/template/src/index-modular.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the build output structure and bundle configuration
# Check if the source files exist
echo "Checking source files..."
fd -t f "index-(default|modular|liveobjects)\.ts$" src/
# Verify esbuild can resolve all imports
echo "Verifying esbuild can resolve imports..."
for file in src/index-{default,liveobjects,modular}.ts; do
if [ -f "$file" ]; then
esbuild --bundle "$file" --outdir=dist --analyze
fi
done
Length of output: 484
Script:
#!/bin/bash
# Let's check the entire src directory structure and content
echo "Checking src directory structure:"
fd . src/
echo -e "\nChecking package.json location and content:"
fd package.json
cat test/package/browser/template/package.json
Length of output: 8303
Script:
#!/bin/bash
# Check for the existence of the specific index files in the test package
echo "Checking for index files in test/package/browser/template/src:"
ls -la test/package/browser/template/src/index-*.ts 2>/dev/null || echo "No index files found"
# Check if there are any index files anywhere in the repository
echo -e "\nSearching for index files repository-wide:"
fd "index-(default|modular|liveobjects)\.ts$"
Length of output: 910
test/package/browser/template/README.md (1)
11-11: LGTM! Clear documentation of LiveObjects plugin import.
The import statement documentation is well-structured and consistent with other import examples in the file.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~11-~11: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...liveobjects.ts imports the LiveObjects ably-js plugin (import LiveObjects from 'ably/...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
| update: { [keyName: string]: 'updated' | 'removed' }; | ||
| } | ||
|
|
||
| export type StateValue = string | number | boolean | Buffer | Uint8Array; |
There was a problem hiding this comment.
Replace 'Buffer' with 'ArrayBuffer' for browser compatibility
The use of Buffer in the StateValue type may cause compatibility issues in browser environments where Buffer is not available. Consider using ArrayBuffer to ensure cross-platform compatibility.
Apply this diff to replace Buffer with ArrayBuffer:
-export type StateValue = string | number | boolean | Buffer | Uint8Array;
+export type StateValue = string | number | boolean | ArrayBuffer | Uint8Array;📝 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.
| export type StateValue = string | number | boolean | Buffer | Uint8Array; | |
| export type StateValue = string | number | boolean | ArrayBuffer | Uint8Array; |
| } | ||
|
|
||
| export declare interface LiveObjectUpdate { | ||
| update: any; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using 'any' type for 'update' property in 'LiveObjectUpdate'
Using any defeats TypeScript's type safety. Consider defining a generic parameter or a more specific type for update to enhance type safety and code reliability.
Apply this diff to make LiveObjectUpdate generic:
-export declare interface LiveObjectUpdate {
- update: any;
+export declare interface LiveObjectUpdate<T = any> {
+ update: T;
}Adjust the implementations accordingly.
Committable suggestion skipped: line range outside the PR's diff.
| @@ -3,6 +3,7 @@ import { test, expect } from '@playwright/test'; | |||
| test.describe('NPM package', () => { | |||
| for (const scenario of [ | |||
| { name: 'default export', path: '/index-default.html' }, | |||
| { name: 'LiveObjects plugin export', path: '/index-liveobjects.html' }, | |||
There was a problem hiding this comment.
💡 Codebase verification
HTML file exists but JavaScript test file is missing
The test scenario is properly integrated, but there's a critical issue:
- The HTML file exists at
test/package/browser/template/server/resources/index-liveobjects.html - The source TypeScript file exists at
test/package/browser/template/src/index-liveobjects.ts - However, the required JavaScript file
index-liveobjects.jsreferenced in the HTML is missing from the resources directory
This suggests the TypeScript file needs to be compiled to JavaScript and placed in the correct location for the tests to work.
🔗 Analysis chain
LGTM! Test scenario properly integrated.
The new LiveObjects plugin export test scenario is correctly integrated into the existing test structure, maintaining consistency with other export tests.
Let's verify the referenced HTML file exists and contains proper test implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and content of the LiveObjects test HTML file
# Check if the HTML file exists
fd -t f "index-liveobjects.html" test/package/browser/template
# Examine its content for proper test setup
rg -A 10 "LiveObjects" test/package/browser/template/index-liveobjects.html
Length of output: 301
Script:
#!/bin/bash
# Let's check the content of the HTML file found in the server/resources directory
cat test/package/browser/template/server/resources/index-liveobjects.html
# Also verify if there are any references to this file in the test setup
rg -g "*.{ts,js}" "index-liveobjects.html" test/package/browser/template
Length of output: 590
Script:
#!/bin/bash
# Let's verify the existence and content of the referenced JavaScript files
cat test/package/browser/template/server/resources/index-liveobjects.js
cat test/package/browser/template/server/resources/runTest.js
# Also check if there are any LiveObjects-related test configurations
rg -g "*.{ts,js}" -B 2 -A 2 "LiveObjects" test/package/browser/template
Length of output: 1806
| const channel = realtime.channels.get('channel'); | ||
| // check liveObjects can be accessed | ||
| const liveObjects = channel.liveObjects; | ||
| const root = await liveObjects.getRoot(); | ||
|
|
||
| // check root is recognized as LiveMap TypeScript type | ||
| root.get('someKey'); | ||
| root.size(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add connection state handling and assertions
The LiveMap testing should verify channel connection state and include assertions for the operations.
const channel = realtime.channels.get('channel');
+ await new Promise((resolve, reject) => {
+ channel.once('attached', resolve);
+ channel.once('failed', (err) => reject(err));
+ });
+
// check liveObjects can be accessed
const liveObjects = channel.liveObjects;
const root = await liveObjects.getRoot();
// check root is recognized as LiveMap TypeScript type
- root.get('someKey');
- root.size();
+ // Verify LiveMap operations
+ const value = root.get('someKey');
+ const size = root.size();
+ console.assert(typeof size === 'number', 'size() should return a number');📝 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 channel = realtime.channels.get('channel'); | |
| // check liveObjects can be accessed | |
| const liveObjects = channel.liveObjects; | |
| const root = await liveObjects.getRoot(); | |
| // check root is recognized as LiveMap TypeScript type | |
| root.get('someKey'); | |
| root.size(); | |
| const channel = realtime.channels.get('channel'); | |
| await new Promise((resolve, reject) => { | |
| channel.once('attached', resolve); | |
| channel.once('failed', (err) => reject(err)); | |
| }); | |
| // check liveObjects can be accessed | |
| const liveObjects = channel.liveObjects; | |
| const root = await liveObjects.getRoot(); | |
| // check root is recognized as LiveMap TypeScript type | |
| // Verify LiveMap operations | |
| const value = root.get('someKey'); | |
| const size = root.size(); | |
| console.assert(typeof size === 'number', 'size() should return a number'); |
| // check LiveMap subscription callback has correct TypeScript types | ||
| const { unsubscribe } = root.subscribe(({ update }) => { | ||
| switch (update.someKey) { | ||
| case 'removed': | ||
| case 'updated': | ||
| break; | ||
| default: | ||
| // check all possible types are exhausted | ||
| const shouldExhaustAllTypes: never = update.someKey; | ||
| } | ||
| }); | ||
| unsubscribe(); | ||
|
|
There was a problem hiding this comment.
Fix switch statement scoping and add error handling
- The
shouldExhaustAllTypesdeclaration should be scoped to prevent access from other cases - Consider using try-finally to ensure subscription cleanup
// check LiveMap subscription callback has correct TypeScript types
- const { unsubscribe } = root.subscribe(({ update }) => {
- switch (update.someKey) {
- case 'removed':
- case 'updated':
- break;
- default:
- // check all possible types are exhausted
- const shouldExhaustAllTypes: never = update.someKey;
- }
- });
- unsubscribe();
+ let unsubscribe: (() => void) | undefined;
+ try {
+ unsubscribe = root.subscribe(({ update }) => {
+ switch (update.someKey) {
+ case 'removed':
+ case 'updated':
+ break;
+ default: {
+ // check all possible types are exhausted
+ const shouldExhaustAllTypes: never = update.someKey;
+ break;
+ }
+ }
+ }).unsubscribe;
+ } finally {
+ unsubscribe?.();
+ }📝 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.
| // check LiveMap subscription callback has correct TypeScript types | |
| const { unsubscribe } = root.subscribe(({ update }) => { | |
| switch (update.someKey) { | |
| case 'removed': | |
| case 'updated': | |
| break; | |
| default: | |
| // check all possible types are exhausted | |
| const shouldExhaustAllTypes: never = update.someKey; | |
| } | |
| }); | |
| unsubscribe(); | |
| // check LiveMap subscription callback has correct TypeScript types | |
| let unsubscribe: (() => void) | undefined; | |
| try { | |
| unsubscribe = root.subscribe(({ update }) => { | |
| switch (update.someKey) { | |
| case 'removed': | |
| case 'updated': | |
| break; | |
| default: { | |
| // check all possible types are exhausted | |
| const shouldExhaustAllTypes: never = update.someKey; | |
| break; | |
| } | |
| } | |
| }).unsubscribe; | |
| } finally { | |
| unsubscribe?.(); | |
| } |
🧰 Tools
🪛 Biome
[error] 27-27: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
VeskeR
force-pushed
the
liveobjects/package-test
branch
from
0e63675
to
b7eeff6
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1921/features
github-actions
bot
temporarily deployed
to
staging/pull/1921/bundle-report
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
test/package/browser/template/src/index-liveobjects.ts (1)
5-41: Consider splitting tests by functionalityThe current test function combines multiple concerns (LiveMap and LiveCounter testing) and mixes type checking with functional testing. Consider splitting it into separate test functions for better maintainability:
testLiveMapTypes- For LiveMap type checkingtestLiveMapFunctionality- For LiveMap runtime behaviortestLiveCounterTypes- For LiveCounter type checkingtestLiveCounterFunctionality- For LiveCounter runtime behaviorThis separation would make the tests more focused and easier to maintain.
🧰 Tools
🪛 Biome
[error] 27-27: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
test/package/browser/template/README.md (2)
29-30: Fix list numbering and enhance bundle documentation.The current documentation has inconsistent numbering and could benefit from additional context:
- Fix the numbering sequence
- Add a brief explanation of the LiveObjects bundle's purpose
Consider this improvement:
1. a bundle containing `src/index-default.ts` and ably-js; - 2. a bundle containing `src/index-liveobjects.ts` and ably-js. - 3. a bundle containing `src/index-modular.ts` and ably-js. + 2. a bundle containing `src/index-liveobjects.ts` and ably-js for testing LiveObjects plugin functionality; + 3. a bundle containing `src/index-modular.ts` and ably-js for testing tree-shakable imports.🧰 Tools
🪛 LanguageTool
[uncategorized] ~29-~29: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...ntainingsrc/index-liveobjects.tsand ably-js. 3. a bundle containing `src/index-mo...(HYPHENATED_LY_ADVERB_ADJECTIVE)
[uncategorized] ~30-~30: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...e containingsrc/index-modular.tsand ably-js. -test: Using the bundles created by...(HYPHENATED_LY_ADVERB_ADJECTIVE)
Line range hint
1-30: Consider adding TypeScript types testing documentation.The PR objectives mention validating TypeScript types for LiveObjects, but this isn't explicitly covered in the documentation. Consider adding a note about TypeScript types testing under the "typecheck" script section.
Add something like:
- `typecheck`: Type-checks the code that imports ably-js. + `typecheck`: Type-checks the code that imports ably-js, including validation of LiveObjects TypeScript types.🧰 Tools
🪛 LanguageTool
[uncategorized] ~8-~8: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...tains three files, each of which import ably-js in different manners, and provide a way...(HYPHENATED_LY_ADVERB_ADJECTIVE)
[uncategorized] ~10-~10: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...c/index-default.tsimports the default ably-js package (import { Realtime } from 'abl...(HYPHENATED_LY_ADVERB_ADJECTIVE)
[uncategorized] ~11-~11: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...liveobjects.tsimports the LiveObjects ably-js plugin (import LiveObjects from 'ably/...(HYPHENATED_LY_ADVERB_ADJECTIVE)
[style] ~12-~12: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...iveobjects'). -src/index-modular.tsimports the tree-shakable ably-js package (imp...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~12-~12: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...x-modular.tsimports the tree-shakable ably-js package (import { BaseRealtime, WebSoc...(HYPHENATED_LY_ADVERB_ADJECTIVE)
[style] ~13-~13: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... 'ably/modular'). -src/ReactApp.tsximports React hooks from the ably-js package (...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~13-~13: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...ctApp.tsximports React hooks from the ably-js package (import { useChannel } from 'a...(HYPHENATED_LY_ADVERB_ADJECTIVE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
test/package/browser/template/README.md(2 hunks)test/package/browser/template/package.json(1 hunks)test/package/browser/template/server/resources/index-liveobjects.html(1 hunks)test/package/browser/template/server/server.ts(1 hunks)test/package/browser/template/src/index-liveobjects.ts(1 hunks)test/package/browser/template/test/lib/package.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- test/package/browser/template/package.json
- test/package/browser/template/server/resources/index-liveobjects.html
- test/package/browser/template/server/server.ts
- test/package/browser/template/test/lib/package.test.ts
🧰 Additional context used
🪛 LanguageTool
test/package/browser/template/README.md
[uncategorized] ~11-~11: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...liveobjects.ts imports the LiveObjects ably-js plugin (import LiveObjects from 'ably/...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
[uncategorized] ~29-~29: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...ntaining src/index-liveobjects.ts and ably-js. 3. a bundle containing `src/index-mo...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
[uncategorized] ~30-~30: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...e containing src/index-modular.ts and ably-js. - test: Using the bundles created by...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
🪛 Biome
test/package/browser/template/src/index-liveobjects.ts
[error] 27-27: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (4)
test/package/browser/template/src/index-liveobjects.ts (3)
10-13: Add connection state handling
The previous review comment about adding connection state handling is still valid.
19-31: Fix switch statement scoping
The previous review comment about fixing switch statement scoping and adding error handling is still valid.
🧰 Tools
🪛 Biome
[error] 27-27: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
32-41: Test with actual LiveCounter instance
The previous review comment about testing with an actual LiveCounter instance is still valid.
test/package/browser/template/README.md (1)
11-11: LGTM! Clear and consistent documentation.
The LiveObjects plugin import documentation follows the established pattern and accurately reflects the intended usage.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~11-~11: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...liveobjects.ts imports the LiveObjects ably-js plugin (import LiveObjects from 'ably/...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
| // check root is recognized as LiveMap TypeScript type | ||
| root.get('someKey'); | ||
| root.size(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add runtime assertions for LiveMap operations
The current test only verifies TypeScript types. Add actual runtime assertions to verify the behavior.
// check root is recognized as LiveMap TypeScript type
- root.get('someKey');
- root.size();
+ const testKey = 'testKey';
+ const testValue = { data: 'test' };
+
+ await root.set(testKey, testValue);
+ const retrievedValue = root.get(testKey);
+ console.assert(
+ JSON.stringify(retrievedValue) === JSON.stringify(testValue),
+ 'Retrieved value should match set value'
+ );
+
+ const size = root.size();
+ console.assert(size > 0, 'Map should not be empty after setting a value');📝 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.
| // check root is recognized as LiveMap TypeScript type | |
| root.get('someKey'); | |
| root.size(); | |
| // check root is recognized as LiveMap TypeScript type | |
| const testKey = 'testKey'; | |
| const testValue = { data: 'test' }; | |
| await root.set(testKey, testValue); | |
| const retrievedValue = root.get(testKey); | |
| console.assert( | |
| JSON.stringify(retrievedValue) === JSON.stringify(testValue), | |
| 'Retrieved value should match set value' | |
| ); | |
| const size = root.size(); | |
| console.assert(size > 0, 'Map should not be empty after setting a value'); |
| globalThis.testAblyPackage = async function () { | ||
| const key = await createSandboxAblyAPIKey(); | ||
|
|
||
| const realtime = new Ably.Realtime({ key, environment: 'sandbox', plugins: { LiveObjects } }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cleanup for Realtime instance
The test should properly clean up resources by closing the Realtime instance after tests complete.
globalThis.testAblyPackage = async function () {
const key = await createSandboxAblyAPIKey();
+ let realtime: Ably.Realtime | undefined;
+ try {
const realtime = new Ably.Realtime({ key, environment: 'sandbox', plugins: { LiveObjects } });
+ // ... rest of the test code ...
+ } finally {
+ await realtime?.close();
+ }
};Committable suggestion skipped: line range outside the PR's diff.
VeskeR
force-pushed
the
liveobjects/package-test
branch
from
b7eeff6
to
f03ca11
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1921/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1921/features
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
test/package/browser/template/src/sandbox.ts (1)
3-3: Consider adding JSDoc documentation for the withOptions parameter.Since this is a test utility that others might use, it would be helpful to document the expected shape and purpose of the
withOptionsparameter.+/** + * Creates a sandbox API key for testing purposes + * @param withOptions - Optional configuration to merge with the default test app setup + * @returns Promise<string> The API key string + */ export async function createSandboxAblyAPIKey(withOptions?: object) {test/package/browser/template/src/index-liveobjects.ts (1)
33-33: Improve type safety of LiveCounter retrievalThe type assertion to
LiveCountercould be unsafe. Consider using type guards or runtime type checking.- const counter = root.get('randomKey') as Ably.LiveCounter | undefined; + const value = root.get('randomKey'); + const counter = value && 'value' in value && 'increment' in value + ? value as Ably.LiveCounter + : undefined;test/package/browser/template/README.md (1)
29-30: Enhance build script documentation clarity.While the additions correctly document the new bundles, consider these improvements for better clarity:
- Use consistent numbering format (all entries should end with ".")
- Add brief explanations of each bundle's purpose, especially for the LiveObjects and modular bundles.
Here's a suggested improvement:
1. a bundle containing `src/index-default.ts` and ably-js; - 2. a bundle containing `src/index-liveobjects.ts` and ably-js. - 3. a bundle containing `src/index-modular.ts` and ably-js. + 2. a bundle containing `src/index-liveobjects.ts` and ably-js for testing LiveObjects plugin functionality; + 3. a bundle containing `src/index-modular.ts` and ably-js for testing tree-shakable imports;🧰 Tools
🪛 LanguageTool
[uncategorized] ~29-~29: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...ntainingsrc/index-liveobjects.tsand ably-js. 3. a bundle containing `src/index-mo...(HYPHENATED_LY_ADVERB_ADJECTIVE)
[uncategorized] ~30-~30: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...e containingsrc/index-modular.tsand ably-js. -test: Using the bundles created by...(HYPHENATED_LY_ADVERB_ADJECTIVE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
test/package/browser/template/README.md(2 hunks)test/package/browser/template/package.json(1 hunks)test/package/browser/template/server/resources/index-liveobjects.html(1 hunks)test/package/browser/template/server/server.ts(1 hunks)test/package/browser/template/src/index-liveobjects.ts(1 hunks)test/package/browser/template/src/sandbox.ts(1 hunks)test/package/browser/template/test/lib/package.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- test/package/browser/template/package.json
- test/package/browser/template/server/resources/index-liveobjects.html
- test/package/browser/template/server/server.ts
- test/package/browser/template/test/lib/package.test.ts
🧰 Additional context used
🪛 LanguageTool
test/package/browser/template/README.md
[uncategorized] ~11-~11: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...liveobjects.ts imports the LiveObjects ably-js plugin (import LiveObjects from 'ably/...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
[uncategorized] ~29-~29: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...ntaining src/index-liveobjects.ts and ably-js. 3. a bundle containing `src/index-mo...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
[uncategorized] ~30-~30: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...e containing src/index-modular.ts and ably-js. - test: Using the bundles created by...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
🪛 Biome
test/package/browser/template/src/index-liveobjects.ts
[error] 27-27: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (2)
test/package/browser/template/src/sandbox.ts (1)
3-7: LGTM! Clean implementation of optional configuration.
The implementation correctly uses the spread operator with nullish coalescing to merge optional configurations while maintaining backward compatibility.
test/package/browser/template/README.md (1)
11-11: LGTM! Clear documentation of LiveObjects import.
The added documentation clearly shows how to import the LiveObjects plugin, which aligns well with the PR's objective of implementing LiveObjects plugin testing.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~11-~11: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...liveobjects.ts imports the LiveObjects ably-js plugin (import LiveObjects from 'ably/...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
| const liveObjects = channel.liveObjects; | ||
| const root = await liveObjects.getRoot(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for LiveObjects initialization
The getRoot operation should include error handling to gracefully handle initialization failures.
- const liveObjects = channel.liveObjects;
- const root = await liveObjects.getRoot();
+ const liveObjects = channel.liveObjects;
+ let root;
+ try {
+ root = await liveObjects.getRoot();
+ } catch (error) {
+ console.error('Failed to initialize LiveObjects root:', error);
+ throw error;
+ }📝 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 liveObjects = channel.liveObjects; | |
| const root = await liveObjects.getRoot(); | |
| const liveObjects = channel.liveObjects; | |
| let root; | |
| try { | |
| root = await liveObjects.getRoot(); | |
| } catch (error) { | |
| console.error('Failed to initialize LiveObjects root:', error); | |
| throw error; | |
| } |
VeskeR
force-pushed
the
liveobjects/package-test
branch
from
f03ca11
to
1fe3fd3
Compare
VeskeR
changed the base branch from
integration/liveobjects
to
liveobjects/isolated-create-ops-and-vector-timeserials
github-actions
bot
temporarily deployed
to
staging/pull/1921/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1921/features
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (29)
test/package/browser/template/README.md (2)
29-30: Consider enhancing bundle documentation.While the bundle additions are documented, it would be helpful to briefly explain the purpose of each bundle, particularly why LiveObjects requires a separate bundle. This would help developers understand the testing strategy better.
Consider adding brief explanations like:
2. a bundle containing `src/index-liveobjects.ts` and ably-js. + // Tests LiveObjects plugin integration and type definitions 3. a bundle containing `src/index-modular.ts` and ably-js. + // Tests tree-shakable package functionality🧰 Tools
🪛 LanguageTool
[uncategorized] ~29-~29: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...ntainingsrc/index-liveobjects.tsand ably-js. 3. a bundle containing `src/index-mo...(HYPHENATED_LY_ADVERB_ADJECTIVE)
[uncategorized] ~30-~30: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...e containingsrc/index-modular.tsand ably-js. -test: Using the bundles created by...(HYPHENATED_LY_ADVERB_ADJECTIVE)
Documentation needs to be enhanced with test implementation details
The README should be updated to include:
sandbox.ts: A utility file that provides sandbox API key creation for testingtest/hooks/ReactApp.spec.tsx: The Playwright component test implementation for React hookstest/lib/package.test.ts: The main test suite that verifies different package export scenariosThese files are integral to the testing setup and their documentation would provide valuable context for developers working with the test suite.
🔗 Analysis chain
Line range hint
1-30: Verify test documentation completeness.Let's ensure all test-related files are properly documented in the README.
Documentation missing for test files and test implementation details
The following test-related files are not documented in the README:
sandbox.ts: A source file that might be used for testingReactApp.spec.tsx: The React hooks test implementation filepackage.test.ts: The main package test implementation fileLet's verify the purpose of these files to ensure they warrant documentation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any test files that might need documentation # Find all test-related files echo "=== Test files that might need documentation ===" fd -e ts -e tsx . test/package/browser/template/src test/package/browser/template/test \ | grep -i "liveobjects\|test" \ | while read -r file; do if ! grep -q "$(basename "$file")" test/package/browser/template/README.md; then echo "Missing documentation for: $file" fi doneLength of output: 1762
Script:
#!/bin/bash # Check contents of undocumented files echo "=== sandbox.ts contents ===" head -n 20 test/package/browser/template/src/sandbox.ts echo -e "\n=== ReactApp.spec.tsx contents ===" head -n 20 test/package/browser/template/test/hooks/ReactApp.spec.tsx echo -e "\n=== package.test.ts contents ===" head -n 20 test/package/browser/template/test/lib/package.test.tsLength of output: 2525
🧰 Tools
🪛 LanguageTool
[uncategorized] ~8-~8: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...tains three files, each of which import ably-js in different manners, and provide a way...(HYPHENATED_LY_ADVERB_ADJECTIVE)
[uncategorized] ~10-~10: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...c/index-default.tsimports the default ably-js package (import { Realtime } from 'abl...(HYPHENATED_LY_ADVERB_ADJECTIVE)
[uncategorized] ~11-~11: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...liveobjects.tsimports the LiveObjects ably-js plugin (import LiveObjects from 'ably/...(HYPHENATED_LY_ADVERB_ADJECTIVE)
[style] ~12-~12: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...iveobjects'). -src/index-modular.tsimports the tree-shakable ably-js package (imp...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~12-~12: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...x-modular.tsimports the tree-shakable ably-js package (import { BaseRealtime, WebSoc...(HYPHENATED_LY_ADVERB_ADJECTIVE)
[style] ~13-~13: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... 'ably/modular'). -src/ReactApp.tsximports React hooks from the ably-js package (...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~13-~13: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...ctApp.tsximports React hooks from the ably-js package (import { useChannel } from 'a...(HYPHENATED_LY_ADVERB_ADJECTIVE)
src/plugins/liveobjects/syncliveobjectsdatapool.ts (2)
4-7: Architectural change: Simplified state management through StateObjectThe shift from
LiveObjectDatatoStateObjectrepresents a cleaner approach to state management. This change reduces complexity by eliminating the intermediateobjectDatalayer.Consider documenting this architectural decision in the codebase to explain the rationale behind using
StateObjectdirectly.
82-96: Consider further simplification of data entry creationThe
_createLiveCounterDataEntryand_createLiveMapDataEntrymethods now simply wrapstateObjectwith type information. This could be further simplified.Consider this more concise approach:
- private _createLiveCounterDataEntry(stateObject: StateObject): LiveCounterDataEntry { - const newEntry: LiveCounterDataEntry = { - stateObject, - objectType: 'LiveCounter', - }; - return newEntry; - } + private _createLiveCounterDataEntry(stateObject: StateObject): LiveCounterDataEntry { + return { stateObject, objectType: 'LiveCounter' }; + } - private _createLiveMapDataEntry(stateObject: StateObject): LiveMapDataEntry { - const newEntry: LiveMapDataEntry = { - stateObject, - objectType: 'LiveMap', - }; - return newEntry; - } + private _createLiveMapDataEntry(stateObject: StateObject): LiveMapDataEntry { + return { stateObject, objectType: 'LiveMap' }; + }src/plugins/liveobjects/timeserial.ts (2)
12-16: Enhance documentation for the siteCode propertyWhile the property is properly documented, consider enhancing the JSDoc comment to explain what a site code represents and its significance in the context of timeserials.
Line range hint
1-180: Consider test implications of temporary implementationSince this is part of adding LiveObjects package test and the
siteCodeimplementation is temporary, ensure that:
- Tests are aware of the temporary nature of the implementation
- Tests are designed to be adaptable when the implementation changes
- Consider adding a comment in the test files noting the dependency on this temporary implementation
This will help prevent test brittleness when the final implementation is added.
src/plugins/liveobjects/liveobjects.ts (2)
89-99: Consider adding error handling for invalid state messagesThe state message handling logic is correct, but consider adding validation for the incoming state messages to ensure robustness.
Consider adding validation:
handleStateMessages(stateMessages: StateMessage[]): void { + if (!Array.isArray(stateMessages)) { + this._client.Logger.logAction( + this._client.logger, + this._client.Logger.LOG_ERROR, + 'LiveObjects.handleStateMessages()', + 'Invalid state messages received', + ); + return; + } if (this._syncInProgress) { this._bufferedStateOperations.push(...stateMessages); return; } this._liveObjectsPool.applyStateMessages(stateMessages); }
Line range hint
198-214: Consider adding type validation before state overrideWhile the implementation is correct, consider adding type validation before overriding state objects to ensure type safety.
Consider adding validation:
if (existingObject) { + if (entry.objectType !== existingObject.type) { + throw new this._client.ErrorInfo( + `Type mismatch: existing=${existingObject.type}, new=${entry.objectType}`, + 50000, + 500 + ); + } const update = existingObject.overrideWithStateObject(entry.stateObject); existingObjectUpdates.push({ object: existingObject, update }); continue; }test/common/modules/live_objects_helper.js (1)
104-106: Document the purpose of map semantics field.The newly added
semanticsfield is hardcoded to 0 without any explanation of its purpose or possible values. Consider adding a comment or documentation explaining what this field represents and what other values it might support.src/common/lib/client/realtimechannel.ts (1)
652-652: Consider consistency between STATE and STATE_SYNC message handlingThere's an inconsistency in how channelSerial is handled between STATE and STATE_SYNC messages:
handleStateMessages(STATE): called without channelSerialhandleStateSyncMessages(STATE_SYNC): called with channelSerialThis difference in behavior should be documented or aligned if not intentional.
Also applies to: 687-687
ably.d.ts (8)
2031-2034: Add JSDoc documentation for LiveObjects interfaceThe
LiveObjectsinterface is missing JSDoc documentation to describe its purpose and usage. Consider adding comprehensive documentation similar to other interfaces in this file.+/** + * Enables real-time synchronization of data structures between clients. + * Provides access to live data types like maps and counters that automatically + * sync changes across connected clients. + */ export declare interface LiveObjects { getRoot(): Promise<LiveMap>; }
2037-2040: Add JSDoc documentation for LiveMap interface and methodsThe
LiveMapinterface and its methods lack documentation. Add JSDoc to explain the purpose of the interface and the behavior of each method.+/** + * Represents a synchronized map data structure that can contain both primitive values + * and nested LiveObjects. Changes to the map are automatically synchronized across + * connected clients. + */ export declare interface LiveMap extends LiveObject<LiveMapUpdate> { + /** + * Retrieves the value associated with the specified key. + * @param key - The key to look up in the map + * @returns The value if found (either a LiveObject or primitive), undefined if not found + */ get(key: string): LiveObject | StateValue | undefined; + /** + * Returns the number of key-value pairs in the map. + * @returns The size of the map + */ size(): number; }
2042-2044: Add JSDoc documentation for LiveMapUpdate interfaceThe
LiveMapUpdateinterface needs documentation to explain its role in tracking map changes.+/** + * Represents an update event for a LiveMap, containing information about + * which keys were updated or removed in the map. + */ export declare interface LiveMapUpdate extends LiveObjectUpdate { + /** + * A record of changes where each key maps to either 'updated' or 'removed' + * to indicate the type of change that occurred. + */ update: { [keyName: string]: 'updated' | 'removed' }; }
2048-2050: Add JSDoc documentation for LiveCounter interfaceThe
LiveCounterinterface needs documentation to explain its purpose and behavior.+/** + * Represents a synchronized counter that can be incremented or decremented + * across connected clients. All changes are automatically synchronized. + */ export declare interface LiveCounter extends LiveObject<LiveCounterUpdate> { + /** + * Returns the current value of the counter. + * @returns The current numeric value of the counter + */ value(): number; }
2052-2054: Add JSDoc documentation for LiveCounterUpdate interfaceThe
LiveCounterUpdateinterface needs documentation to explain its role in counter updates.+/** + * Represents an update event for a LiveCounter, containing the + * increment/decrement value that was applied. + */ export declare interface LiveCounterUpdate extends LiveObjectUpdate { + /** + * The value by which the counter was incremented (positive) + * or decremented (negative). + */ update: { inc: number }; }
2056-2060: Add JSDoc documentation for LiveObject interfaceThe base
LiveObjectinterface needs documentation to explain its role as the foundation for live data types.+/** + * Base interface for all live data types that can be synchronized across clients. + * Provides subscription capabilities for tracking changes to the object. + */ export declare interface LiveObject<TUpdate extends LiveObjectUpdate = LiveObjectUpdate> { + /** + * Subscribes to updates on this live object. + * @param listener - Callback function that will be invoked with update events + * @returns A subscription response object that can be used to unsubscribe + */ subscribe(listener: (update: TUpdate) => void): SubscribeResponse; + /** + * Removes a specific update listener. + * @param listener - The callback function to remove + */ unsubscribe(listener: (update: TUpdate) => void): void; + /** + * Removes all update listeners from this object. + */ unsubscribeAll(): void; }
2066-2068: Add JSDoc documentation for SubscribeResponse interfaceThe
SubscribeResponseinterface needs documentation to explain its purpose in subscription management.+/** + * Represents a subscription handle returned when subscribing to live object updates. + * Can be used to unsubscribe from further updates. + */ export declare interface SubscribeResponse { + /** + * Cancels the subscription and stops receiving updates. + */ unsubscribe(): void; }
2031-2068: Consider adding error handling and connection state managementThe LiveObjects implementation could benefit from additional considerations:
- Error Handling: Consider adding error cases in the type definitions, such as handling synchronization conflicts or network issues.
- Connection State: Consider exposing synchronization state to allow applications to show appropriate loading/error states.
- Batch Operations: Consider adding support for batch operations on LiveMap to optimize network usage.
src/plugins/liveobjects/liveobjectspool.ts (1)
92-92: Typo in comment: 'ba able' should be 'be able'Correct the typographical error to improve code documentation clarity.
Apply this diff to fix the typo:
- // since they need to ba able to eventually initialize themselves from that *_CREATE op. + // since they need to be able to eventually initialize themselves from that *_CREATE op.src/plugins/liveobjects/liveobject.ts (3)
114-125: Refactor_timeserialMapFromStringMapfor Improved ReadabilityThe current implementation using
reducemay be less readable. Consider refactoring the method for better clarity.Suggested refactor:
protected _timeserialMapFromStringMap(stringTimeserialsMap: Record<string, string>): Record<string, Timeserial> { const objTimeserialsMap: Record<string, Timeserial> = {}; for (const [key, timeserialString] of Object.entries(stringTimeserialsMap)) { objTimeserialsMap[key] = DefaultTimeserial.calculateTimeserial(this._client, timeserialString); } return objTimeserialsMap; }
137-137: Document Parameters inapplyOperationMethodEnsure that the abstract
applyOperationmethod has clear documentation detailing the purpose of theopandmsgparameters to guide subclass implementations.
155-156: Clarify Purpose of_updateFromDataDiffMethodEnsure that the
_updateFromDataDiffmethod accurately computes the update object based on the differences betweenprevDataRefandnewDataRef. Clear documentation will aid in maintaining and extending this functionality.src/plugins/liveobjects/statemessage.ts (1)
283-300: Simplify buffer handling in_encodeStateValuemethodThe buffer handling logic can be clarified for better readability and efficiency. Consider refactoring the method to make the conditions and returns more straightforward.
For example:
private static _encodeStateValue( platform: typeof Platform, value: StateValue | undefined, encoding: string | undefined, withBase64Encoding: boolean, ): { value: StateValue | undefined; encoding: string | undefined; } { if (!value || !platform.BufferUtils.isBuffer(value)) { return { value, encoding }; } if (withBase64Encoding) { return { value: platform.BufferUtils.base64Encode(value), encoding: encoding ? encoding + '/base64' : 'base64', }; } return { value: platform.BufferUtils.toBuffer(value), encoding, }; }src/plugins/liveobjects/livemap.ts (3)
183-240: Ensure 'overrideWithStateObject' method handles all validation casesThe
overrideWithStateObjectmethod performs multiple validations on thestateObject. Confirm that all possible invalid states are covered and that error messages are informative for easier debugging.Consider adding additional validation or refining error messages for clarity.
Line range hint
246-284: Optimize the data difference calculation in '_updateFromDataDiff'The
_updateFromDataDiffmethod compares previous and new data to determine updates. For performance enhancement, consider optimizing the loops or using more efficient data comparison strategies, especially for large datasets.For example, you might:
- Break early from loops when possible.
- Use more efficient data structures for lookup.
338-344: Clarify error handling in '_throwNoPayloadError'The
_throwNoPayloadErrormethod throws anErrorInfowhen no payload is found. Ensure that the error message includes sufficient context for debugging.Consider including the operation details in the error message:
- `No payload found for ${op.action} op for LiveMap objectId=${this.getObjectId()}` + `No payload found for ${op.action} operation on LiveMap with objectId=${this.getObjectId()}`test/realtime/live_objects.test.js (3)
623-623: Typographical Correction in Test DescriptionConsider rephrasing the test description for clarity. Suggest changing:
- From:
'MAP_CREATE state operation messages are applied based on the site timeserials vector of the object'- To:
'MAP_CREATE state operation messages are applied based on the object's site timeserials vector'
1449-1451: Potential Numeric Overflow withMath.pow(10, i + 1)In the loop incrementing the counter:
amount: Math.pow(10, i + 1)
- Be cautious with large exponentiation as it may lead to values exceeding the maximum safe integer limit in JavaScript (
Number.MAX_SAFE_INTEGER).- Consider using safe arithmetic operations or adding checks to prevent potential overflow.
1214-1220: Ensure Unique Test Channel NamesWhen generating test channel names based on the scenario description:
const channelName = scenario.description;
- Using the entire description as the channel name may result in long or duplicate names.
- Consider sanitizing and shortening the channel name to ensure it is unique and conforms to channel naming conventions.
🛑 Comments failed to post (14)
test/package/browser/template/src/index-liveobjects.ts (4)
8-8: 🛠️ Refactor suggestion
Add connection state verification
Verify that the Realtime instance is properly connected before proceeding with tests.
const realtime = new Ably.Realtime({ key, environment: 'sandbox', plugins: { LiveObjects } }); + await new Promise<void>((resolve, reject) => { + realtime.connection.once('connected', () => resolve()); + realtime.connection.once('failed', (err) => reject(err)); + });📝 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 realtime = new Ably.Realtime({ key, environment: 'sandbox', plugins: { LiveObjects } }); await new Promise<void>((resolve, reject) => { realtime.connection.once('connected', () => resolve()); realtime.connection.once('failed', (err) => reject(err)); });
21-28:
⚠️ Potential issueFix switch statement declaration scope
The
shouldExhaustAllTypesdeclaration can be erroneously accessed from other cases.switch (update.someKey) { case 'removed': case 'updated': break; default: { // check all possible types are exhausted const shouldExhaustAllTypes: never = update.someKey; + break; + } }📝 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.switch (update.someKey) { case 'removed': case 'updated': break; default: { // check all possible types are exhausted const shouldExhaustAllTypes: never = update.someKey; break; } }🧰 Tools
🪛 Biome (1.9.4)
[error] 27-27: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
5-41: 🛠️ Refactor suggestion
Improve overall test structure and error handling
The test should follow a proper structure with setup, execution, and cleanup phases.
-globalThis.testAblyPackage = async function () { +globalThis.testAblyPackage = async function () { + let realtime: Ably.Realtime | undefined; + let cleanup: (() => Promise<void>)[] = []; + + try { const key = await createSandboxAblyAPIKey({ featureFlags: ['enableChannelState'] }); const realtime = new Ably.Realtime({ key, environment: 'sandbox', plugins: { LiveObjects } }); + cleanup.push(() => realtime.close()); // ... rest of the test code ... + } catch (error) { + console.error('Test failed:', error); + throw error; + } finally { + // Execute cleanup in reverse order + for (const cleanupFn of cleanup.reverse()) { + try { + await cleanupFn(); + } catch (error) { + console.error('Cleanup failed:', error); + } + } + } };Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 27-27: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
32-41: 🛠️ Refactor suggestion
Add test coverage for LiveCounter operations
The current test only verifies TypeScript types. Add test coverage for actual LiveCounter operations:
- Increment/decrement operations
- Value updates
- Concurrent modifications
- // check LiveCounter types also behave as expected - const counter = root.get('randomKey') as Ably.LiveCounter | undefined; - // use nullish coalescing as we didn't actually create a counter object on the root, - // so the next calls would fail. we only need to check that TypeScript types work - counter?.value(); - const counterSubscribeResponse = counter?.subscribe(({ update }) => { - const shouldBeANumber: number = update.inc; - }); - counterSubscribeResponse?.unsubscribe(); + // Test actual LiveCounter operations + const counter = await liveObjects.createCounter('testCounter', 0); + try { + // Test initial value + console.assert(counter.value() === 0, 'Initial value should be 0'); + + // Test increment + await counter.increment(5); + console.assert(counter.value() === 5, 'Value should be 5 after increment'); + + // Test decrement + await counter.decrement(2); + console.assert(counter.value() === 3, 'Value should be 3 after decrement'); + + // Test subscription + const updates: number[] = []; + const subscription = counter.subscribe(({ update }) => { + updates.push(update.inc); + }); + + // Test concurrent modifications + await Promise.all([ + counter.increment(1), + counter.increment(2), + counter.decrement(1) + ]); + + subscription.unsubscribe(); + console.assert(updates.length === 3, 'Should receive 3 updates'); + } finally { + await root.delete('testCounter'); + }📝 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.// Test actual LiveCounter operations const counter = await liveObjects.createCounter('testCounter', 0); try { // Test initial value console.assert(counter.value() === 0, 'Initial value should be 0'); // Test increment await counter.increment(5); console.assert(counter.value() === 5, 'Value should be 5 after increment'); // Test decrement await counter.decrement(2); console.assert(counter.value() === 3, 'Value should be 3 after decrement'); // Test subscription const updates: number[] = []; const subscription = counter.subscribe(({ update }) => { updates.push(update.inc); }); // Test concurrent modifications await Promise.all([ counter.increment(1), counter.increment(2), counter.decrement(1) ]); subscription.unsubscribe(); console.assert(updates.length === 3, 'Should receive 3 updates'); } finally { await root.delete('testCounter'); }src/plugins/liveobjects/statemessage.ts (6)
154-155:
⚠️ Potential issueReplace 'this' with class name in static methods for clarity
Using
thisin static methods can be misleading, asthisrefers to the class itself. It's clearer and more conventional to use the class name directly. Please replacethiswithStateMessagein the following static methods.Apply the following changes:
Line 154:
- await this._decodeMapEntries(message.object.map.entries, inputContext, decodeDataFn); + await StateMessage._decodeMapEntries(message.object.map.entries, inputContext, decodeDataFn);Line 158:
- await this._decodeMapEntries(message.object.createOp.map.entries, inputContext, decodeDataFn); + await StateMessage._decodeMapEntries(message.object.createOp.map.entries, inputContext, decodeDataFn);Line 162:
- await this._decodeStateData(message.object.createOp.mapOp.data, inputContext, decodeDataFn); + await StateMessage._decodeStateData(message.object.createOp.mapOp.data, inputContext, decodeDataFn);Line 166:
- await this._decodeMapEntries(message.operation.map.entries, inputContext, decodeDataFn); + await StateMessage._decodeMapEntries(message.operation.map.entries, inputContext, decodeDataFn);Line 170:
- await this._decodeStateData(message.operation.mapOp.data, inputContext, decodeDataFn); + await StateMessage._decodeStateData(message.operation.mapOp.data, inputContext, decodeDataFn);Also applies to: 158-159, 162-163, 166-167, 170-171
🧰 Tools
🪛 Biome (1.9.4)
[error] 154-154: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
224-225:
⚠️ Potential issueReplace 'this' with class name in static methods for clarity
In the
_encodeStateOperationmethod, using the class name improves readability. Please update the references accordingly.Apply the following changes:
Line 224:
- stateOperationCopy.mapOp.data = this._encodeStateData(platform, stateOperation.mapOp?.data!, withBase64Encoding); + stateOperationCopy.mapOp.data = StateMessage._encodeStateData(platform, stateOperation.mapOp?.data!, withBase64Encoding);Line 230:
- entry.data = this._encodeStateData(platform, stateOperation?.map?.entries?.[key].data!, withBase64Encoding); + entry.data = StateMessage._encodeStateData(platform, stateOperation?.map?.entries?.[key].data!, withBase64Encoding);Also applies to: 230-231
🧰 Tools
🪛 Biome (1.9.4)
[error] 224-224: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
320-325:
⚠️ Potential issueClarify the use of
argumentsintoJSONmethodUsing
arguments.length > 0to detectJSON.stringify()calls can be unreliable and may lead to unexpected behaviors. Consider using a more explicit approach to determine when base64 encoding is needed.For instance, you could add a parameter to the method to indicate whether base64 encoding should be applied.
toJSON(withBase64Encoding = false): { ... } { // Existing logic using the `withBase64Encoding` parameter }
266-267:
⚠️ Potential issueReplace 'this' with class name in static methods for clarity
In the
_encodeStateDatamethod, replacingthis._encodeStateValuewithStateMessage._encodeStateValueenhances clarity.Apply the following change:
Line 266:
- const { value, encoding } = this._encodeStateValue(platform, data?.value, data?.encoding, withBase64Encoding); + const { value, encoding } = StateMessage._encodeStateValue(platform, data?.value, data?.encoding, withBase64Encoding);📝 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 { value, encoding } = StateMessage._encodeStateValue(platform, data?.value, data?.encoding, withBase64Encoding); return {🧰 Tools
🪛 Biome (1.9.4)
[error] 266-266: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
195-196:
⚠️ Potential issueReplace 'this' with class name in static methods for clarity
In the
_decodeMapEntriesstatic method, replacethis._decodeStateDatawithStateMessage._decodeStateDatato maintain consistency and avoid confusion.Apply the following change:
Line 195:
- await this._decodeStateData(entry.data, inputContext, decodeDataFn); + await StateMessage._decodeStateData(entry.data, inputContext, decodeDataFn);📝 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.await StateMessage._decodeStateData(entry.data, inputContext, decodeDataFn); }🧰 Tools
🪛 Biome (1.9.4)
[error] 195-195: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
259-260:
⚠️ Potential issueReplace 'this' with class name in static methods for clarity
In the
_encodeStateObjectmethod, updatethis._encodeStateOperationtoStateMessage._encodeStateOperation.Apply the following change:
Line 259:
- stateObjectCopy.createOp = this._encodeStateOperation(platform, stateObject.createOp!, withBase64Encoding); + stateObjectCopy.createOp = StateMessage._encodeStateOperation(platform, stateObject.createOp!, withBase64Encoding);📝 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.stateObjectCopy.createOp = StateMessage._encodeStateOperation(platform, stateObject.createOp!, withBase64Encoding); }🧰 Tools
🪛 Biome (1.9.4)
[error] 259-259: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
src/plugins/liveobjects/livemap.ts (3)
456-484:
⚠️ Potential issueEnsure proper handling of 'entry.data' in '_liveMapDataFromMapEntries'
In the
_liveMapDataFromMapEntriesmethod, when creatingliveData, the code assumes that eitherentry.data.objectIdorentry.data.valueexists. To prevent runtime errors, include validation to handle cases whereentry.datamight be missing or incomplete.Add checks to confirm that
entry.datais defined and contains the expected properties.
72-75:
⚠️ Potential issueHandle potential undefined 'semantics' in 'fromStateObject' method
The use of the non-null assertion operator
!onstateObject.map?.semantics!assumes thatstateObject.mapandstateObject.map.semanticsare always defined. IfstateObject.mapisundefined, this will result in a runtime error. It's safer to include null checks to ensure robustness.Consider modifying the code to include null checks:
- const obj = new LiveMap(liveobjects, stateObject.map?.semantics!, stateObject.objectId); + if (!stateObject.map || !stateObject.map.semantics) { + throw new this._client.ErrorInfo( + 'Invalid state object: missing map semantics', + 50000, + 500, + ); + } + const obj = new LiveMap(liveobjects, stateObject.map.semantics, stateObject.objectId);Committable suggestion skipped: line range outside the PR's diff.
346-369:
⚠️ Potential issueValidate semantics match in '_applyMapCreate' method
In the
_applyMapCreatemethod, you check ifthis._semanticsmatchesop.map?.semantics. Ifop.mapisundefined, accessingop.map.semanticscould cause an error. Add a null check forop.mapto prevent this.Modify the code to include a null check:
- if (this._semantics !== op.map?.semantics) { + if (!op.map || this._semantics !== op.map.semantics) {📝 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.private _applyMapCreate(op: StateOperation): LiveMapUpdate | LiveObjectUpdateNoop { if (this._createOperationIsMerged) { // There can't be two different create operation for the same object id, because the object id // fully encodes that operation. This means we can safely ignore any new incoming create operations // if we already merged it once. this._client.Logger.logAction( this._client.logger, this._client.Logger.LOG_MICRO, 'LiveMap._applyMapCreate()', `skipping applying MAP_CREATE op on a map instance as it was already applied before; objectId=${this._objectId}`, ); return { noop: true }; } if (!op.map || this._semantics !== op.map.semantics) { throw new this._client.ErrorInfo( `Cannot apply MAP_CREATE op on LiveMap objectId=${this.getObjectId()}; map's semantics=${this._semantics}, but op expected ${op.map?.semantics}`, 50000, 500, ); } return this._mergeInitialDataFromCreateOperation(op); }test/realtime/live_objects.test.js (1)
827-834:
⚠️ Potential issueInconsistent Comment Regarding Site CGOs
In the test cases for
COUNTER_CREATEoperations:
- For serial
'bbb@2-0', the comment reads "existing site, later CGO, applied", which is correct.- However, for serial
'aaa@0-0', the comment says "different site, earlier CGO, applied". This seems inconsistent since an earlier CGO would typically not be applied.- Please review and correct the comments to accurately reflect the logic.
VeskeR
force-pushed
the
liveobjects/isolated-create-ops-and-vector-timeserials
branch
from
e151919
to
567ad15
Compare
VeskeR
force-pushed
the
liveobjects/package-test
branch
from
1fe3fd3
to
9da78a6
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1921/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1921/features
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
test/package/browser/template/README.md (1)
29-30: Consider adding test file references for completeness.While the build script documentation is accurate, it would be helpful to also reference the corresponding test HTML files that use these bundles, particularly
index-liveobjects.htmlwhich was mentioned in the AI summary.Consider adding a note like:
2. a bundle containing `src/index-liveobjects.ts` and ably-js. 3. a bundle containing `src/index-modular.ts` and ably-js. + These bundles are used by corresponding HTML files (index-default.html, index-liveobjects.html, and index-modular.html) for testing.🧰 Tools
🪛 LanguageTool
[uncategorized] ~29-~29: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...ntainingsrc/index-liveobjects.tsand ably-js. 3. a bundle containing `src/index-mo...(HYPHENATED_LY_ADVERB_ADJECTIVE)
[uncategorized] ~30-~30: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...e containingsrc/index-modular.tsand ably-js. -test: Using the bundles created by...(HYPHENATED_LY_ADVERB_ADJECTIVE)
ably.d.ts (4)
2031-2033: Add JSDoc documentation for LiveObjects interfaceThe
LiveObjectsinterface lacks documentation explaining its purpose and usage. Consider adding JSDoc comments to improve API documentation.+/** + * Represents the LiveObjects functionality for a channel, providing access to live data structures. + */ export declare interface LiveObjects { + /** + * Returns the root LiveMap instance for this channel. + * @returns A promise that resolves with the root LiveMap. + */ getRoot(): Promise<LiveMap>; }
2035-2035: Address TODO comment regarding LiveObjects documentationThe TODO comment indicates missing documentation for the public API. This should be addressed before merging.
Would you like me to help generate comprehensive documentation for the LiveObjects public API? I can create a new GitHub issue to track this task.
2037-2040: Add JSDoc documentation for LiveMap interfaceThe
LiveMapinterface needs documentation for its methods and purpose.+/** + * Represents a live map data structure that can be synchronized across clients. + */ export declare interface LiveMap extends LiveObject<LiveMapUpdate> { + /** + * Retrieves a value from the map by key. + * @param key - The key to look up. + * @returns The value associated with the key, or undefined if not found. + */ get(key: string): LiveObject | StateValue | undefined; + /** + * Returns the number of key-value pairs in the map. + * @returns The size of the map. + */ size(): number; }
2056-2060: Improve type safety of subscription methodsThe subscription methods could benefit from stronger typing.
Consider adding type parameters to better type the listener callbacks:
-export declare interface LiveObject<TUpdate extends LiveObjectUpdate = LiveObjectUpdate> { +export declare interface LiveObject<TUpdate extends LiveObjectUpdate<T> = LiveObjectUpdate<any>, T = any> { - subscribe(listener: (update: TUpdate) => void): SubscribeResponse; + subscribe(listener: (update: TUpdate & { update: T }) => void): SubscribeResponse; unsubscribe(listener: (update: TUpdate) => void): void; unsubscribeAll(): void; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
ably.d.ts(1 hunks)test/package/browser/template/README.md(2 hunks)test/package/browser/template/package.json(1 hunks)test/package/browser/template/server/resources/index-liveobjects.html(1 hunks)test/package/browser/template/server/server.ts(1 hunks)test/package/browser/template/src/index-liveobjects.ts(1 hunks)test/package/browser/template/src/sandbox.ts(1 hunks)test/package/browser/template/test/lib/package.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- test/package/browser/template/package.json
- test/package/browser/template/server/resources/index-liveobjects.html
- test/package/browser/template/server/server.ts
- test/package/browser/template/src/sandbox.ts
- test/package/browser/template/test/lib/package.test.ts
🧰 Additional context used
🪛 LanguageTool
test/package/browser/template/README.md
[uncategorized] ~11-~11: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...liveobjects.ts imports the LiveObjects ably-js plugin (import LiveObjects from 'ably/...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
[uncategorized] ~29-~29: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...ntaining src/index-liveobjects.ts and ably-js. 3. a bundle containing `src/index-mo...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
[uncategorized] ~30-~30: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...e containing src/index-modular.ts and ably-js. - test: Using the bundles created by...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
🪛 Biome (1.9.4)
test/package/browser/template/src/index-liveobjects.ts
[error] 27-27: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (5)
test/package/browser/template/src/index-liveobjects.ts (1)
1-3: LGTM: Imports are correctly structured
The necessary dependencies are properly imported for testing the LiveObjects plugin.
test/package/browser/template/README.md (2)
11-11: LGTM! Documentation accurately reflects the LiveObjects import.
The added line clearly documents how to import the LiveObjects plugin, which aligns with the PR objective of implementing LiveObjects plugin tests.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~11-~11: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...liveobjects.ts imports the LiveObjects ably-js plugin (import LiveObjects from 'ably/...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
Line range hint 1-30: Verify the completeness of TypeScript types documentation.
The PR objectives mention updates to ably.d.ts for LiveObjects' public API, but this README doesn't mention TypeScript type testing specifics for LiveObjects.
Let's check if there are any TypeScript-specific test files for LiveObjects:
✅ Verification successful
TypeScript types for LiveObjects are well-tested in the codebase
The index-liveobjects.ts file contains comprehensive TypeScript type testing for LiveObjects, including:
- Type checking for LiveMap operations (
root.get,root.size) - Exhaustive type checking for subscription callbacks
- Type verification for LiveCounter operations and updates
- Type assertions to ensure correct typing of values and updates
The README's statement about TypeScript type testing is accurate, as the implementation demonstrates thorough type checking through practical usage patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for TypeScript test files related to LiveObjects
fd -e ts -e tsx . | rg -i "liveobjects"
# Check if there are any type-specific tests
rg -i "typescript.*test.*liveobjects|liveobjects.*typescript.*test"
Length of output: 573
Script:
#!/bin/bash
# Let's check the content of index-liveobjects.ts to see how it tests TypeScript types
cat test/package/browser/template/src/index-liveobjects.ts
# Also check if there are any type assertions or type-related tests in the file
rg -A 5 "type|interface|declare" test/package/browser/template/src/index-liveobjects.ts
# Let's also check the content of liveobjects.d.ts to understand the type definitions
cat liveobjects.d.ts
Length of output: 3826
🧰 Tools
🪛 LanguageTool
[uncategorized] ~8-~8: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...tains three files, each of which import ably-js in different manners, and provide a way...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
[uncategorized] ~10-~10: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...c/index-default.ts imports the default ably-js package (import { Realtime } from 'abl...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
[uncategorized] ~11-~11: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...liveobjects.ts imports the LiveObjects ably-js plugin (import LiveObjects from 'ably/...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
[style] ~12-~12: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...iveobjects'). - src/index-modular.ts imports the tree-shakable ably-js package (imp...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~12-~12: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...x-modular.ts imports the tree-shakable ably-js package (import { BaseRealtime, WebSoc...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
[style] ~13-~13: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... 'ably/modular'). - src/ReactApp.tsx imports React hooks from the ably-js package (...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~13-~13: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...ctApp.tsx imports React hooks from the ably-js package (import { useChannel } from 'a...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
ably.d.ts (2)
2046-2046:
Replace Buffer with ArrayBuffer in StateValue type
The use of Buffer in browser environments could cause compatibility issues.
This issue was previously identified. Apply the suggested fix:
-export type StateValue = string | number | boolean | Buffer | Uint8Array;
+export type StateValue = string | number | boolean | ArrayBuffer | Uint8Array;2062-2064:
Make LiveObjectUpdate generic to improve type safety
Using any type defeats TypeScript's type safety. Consider making the interface generic.
This issue was previously identified. Apply the suggested fix:
-export declare interface LiveObjectUpdate {
- update: any;
+export declare interface LiveObjectUpdate<T = any> {
+ update: T;
}| globalThis.testAblyPackage = async function () { | ||
| const key = await createSandboxAblyAPIKey({ featureFlags: ['enableChannelState'] }); | ||
|
|
||
| const realtime = new Ably.Realtime({ key, environment: 'sandbox', plugins: { LiveObjects } }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and cleanup for Realtime instance
The test initialization should include proper error handling and cleanup to prevent resource leaks.
globalThis.testAblyPackage = async function () {
- const key = await createSandboxAblyAPIKey({ featureFlags: ['enableChannelState'] });
+ let realtime: Ably.Realtime | undefined;
+ try {
+ const key = await createSandboxAblyAPIKey({ featureFlags: ['enableChannelState'] });
+ realtime = new Ably.Realtime({ key, environment: 'sandbox', plugins: { LiveObjects } });
- const realtime = new Ably.Realtime({ key, environment: 'sandbox', plugins: { LiveObjects } });
+ } catch (error) {
+ console.error('Failed to initialize Ably:', error);
+ throw error;
+ } finally {
+ await realtime?.close();
+ }Committable suggestion skipped: line range outside the PR's diff.
| // check LiveCounter types also behave as expected | ||
| const counter = root.get('randomKey') as Ably.LiveCounter | undefined; | ||
| // use nullish coalescing as we didn't actually create a counter object on the root, | ||
| // so the next calls would fail. we only need to check that TypeScript types work | ||
| counter?.value(); | ||
| const counterSubscribeResponse = counter?.subscribe(({ update }) => { | ||
| const shouldBeANumber: number = update.inc; | ||
| }); | ||
| counterSubscribeResponse?.unsubscribe(); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add actual LiveCounter testing
The current implementation only tests TypeScript types without creating and testing an actual counter.
// check LiveCounter types also behave as expected
- const counter = root.get('randomKey') as Ably.LiveCounter | undefined;
- // use nullish coalescing as we didn't actually create a counter object on the root,
- // so the next calls would fail. we only need to check that TypeScript types work
- counter?.value();
- const counterSubscribeResponse = counter?.subscribe(({ update }) => {
- const shouldBeANumber: number = update.inc;
- });
- counterSubscribeResponse?.unsubscribe();
+ try {
+ const counterKey = 'testCounter';
+ const counter = await root.createCounter(counterKey, { initial: 0 });
+
+ // Test counter operations
+ await counter.increment(5);
+ console.assert(counter.value() === 5, 'Counter value should be 5');
+
+ // Test counter subscription
+ const counterSubscribeResponse = counter.subscribe(({ update }) => {
+ console.assert(typeof update.inc === 'number', 'Increment value should be a number');
+ });
+
+ await counter.increment(3);
+ counterSubscribeResponse.unsubscribe();
+
+ // Cleanup
+ await root.delete(counterKey);
+ } catch (error) {
+ console.error('Failed to test counter:', error);
+ throw error;
+ }Committable suggestion skipped: line range outside the PR's diff.
| const channel = realtime.channels.get('channel', { modes: ['STATE_SUBSCRIBE', 'STATE_PUBLISH'] }); | ||
| // check liveObjects can be accessed | ||
| const liveObjects = channel.liveObjects; | ||
| const root = await liveObjects.getRoot(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add channel connection handling and error handling
The channel setup should include connection state handling and proper error handling for root access.
const channel = realtime.channels.get('channel', { modes: ['STATE_SUBSCRIBE', 'STATE_PUBLISH'] });
+
+ // Wait for channel to be attached
+ await new Promise<void>((resolve, reject) => {
+ channel.once('attached', () => resolve());
+ channel.once('failed', (err) => reject(err));
+ });
+
// check liveObjects can be accessed
const liveObjects = channel.liveObjects;
- const root = await liveObjects.getRoot();
+ let root;
+ try {
+ root = await liveObjects.getRoot();
+ } catch (error) {
+ console.error('Failed to get root:', error);
+ throw error;
+ }📝 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 channel = realtime.channels.get('channel', { modes: ['STATE_SUBSCRIBE', 'STATE_PUBLISH'] }); | |
| // check liveObjects can be accessed | |
| const liveObjects = channel.liveObjects; | |
| const root = await liveObjects.getRoot(); | |
| const channel = realtime.channels.get('channel', { modes: ['STATE_SUBSCRIBE', 'STATE_PUBLISH'] }); | |
| // Wait for channel to be attached | |
| await new Promise<void>((resolve, reject) => { | |
| channel.once('attached', () => resolve()); | |
| channel.once('failed', (err) => reject(err)); | |
| }); | |
| // check liveObjects can be accessed | |
| const liveObjects = channel.liveObjects; | |
| let root; | |
| try { | |
| root = await liveObjects.getRoot(); | |
| } catch (error) { | |
| console.error('Failed to get root:', error); | |
| throw error; | |
| } |
VeskeR
force-pushed
the
liveobjects/isolated-create-ops-and-vector-timeserials
branch
from
059e10b
to
245b6a2
Compare
This is barebones public API definitions (missing documentation) to enable LiveObjects plugin package tests and TypeScript checks.
Test LiveObjects plugin can be imported and provided to the Ably client, and that TypeScript types for LiveObjects work as expected.
VeskeR
force-pushed
the
liveobjects/package-test
branch
from
9da78a6
to
426fd8d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (12)
test/package/browser/template/src/index-liveobjects.ts (2)
19-30: Good use of exhaustiveness checking, but fix switch statement scopingThe use of a
nevertype for exhaustiveness checking is a great practice as it ensures all possible update types are handled. However, the switch statement needs proper scoping as flagged by the static analysis.Apply this diff to fix the scoping while preserving the exhaustiveness check:
const { unsubscribe } = root.subscribe(({ update }) => { switch (update.someKey) { case 'removed': case 'updated': break; default: { // check all possible types are exhausted const shouldExhaustAllTypes: never = update.someKey; break; } } });🧰 Tools
🪛 Biome (1.9.4)
[error] 27-27: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
33-33: Consider using type guard instead of type assertionWhile the type assertion works for testing types, it's safer to use a type guard function to validate the LiveCounter type at runtime.
- const counter = root.get('randomKey') as Ably.LiveCounter | undefined; + const counter = root.get('randomKey'); + const isLiveCounter = (value: unknown): value is Ably.LiveCounter => { + return value !== null && + typeof value === 'object' && + 'value' in value && + 'increment' in value; + }; + if (counter && isLiveCounter(counter)) { + // Now TypeScript knows counter is LiveCounter + counter.value(); + }test/package/browser/template/README.md (2)
29-30: Consider enhancing the build script documentation.While the additions correctly document the new bundles, consider these improvements for better clarity:
- Use consistent list formatting
- Add brief descriptions of each bundle's purpose, especially for the LiveObjects and modular bundles
1. a bundle containing `src/index-default.ts` and ably-js; - 2. a bundle containing `src/index-liveobjects.ts` and ably-js. - 3. a bundle containing `src/index-modular.ts` and ably-js. + 2. a bundle containing `src/index-liveobjects.ts` and ably-js for testing LiveObjects plugin integration; + 3. a bundle containing `src/index-modular.ts` and ably-js for testing tree-shakable imports;🧰 Tools
🪛 LanguageTool
[uncategorized] ~29-~29: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...ntainingsrc/index-liveobjects.tsand ably-js. 3. a bundle containing `src/index-mo...(HYPHENATED_LY_ADVERB_ADJECTIVE)
[uncategorized] ~30-~30: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...e containingsrc/index-modular.tsand ably-js. -test: Using the bundles created by...(HYPHENATED_LY_ADVERB_ADJECTIVE)
Line range hint
1-30: Consider adding TypeScript types testing information.The PR objectives mention validating TypeScript types for LiveObjects, but this isn't explicitly covered in the documentation. Consider adding a note about TypeScript types testing under the
typecheckscript section.- `typecheck`: Type-checks the code that imports ably-js. + `typecheck`: Type-checks the code that imports ably-js, including validation of LiveObjects TypeScript types and interfaces.🧰 Tools
🪛 LanguageTool
[uncategorized] ~8-~8: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...tains three files, each of which import ably-js in different manners, and provide a way...(HYPHENATED_LY_ADVERB_ADJECTIVE)
[uncategorized] ~10-~10: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...c/index-default.tsimports the default ably-js package (import { Realtime } from 'abl...(HYPHENATED_LY_ADVERB_ADJECTIVE)
[uncategorized] ~11-~11: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...liveobjects.tsimports the LiveObjects ably-js plugin (import LiveObjects from 'ably/...(HYPHENATED_LY_ADVERB_ADJECTIVE)
[style] ~12-~12: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...iveobjects'). -src/index-modular.tsimports the tree-shakable ably-js package (imp...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~12-~12: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...x-modular.tsimports the tree-shakable ably-js package (import { BaseRealtime, WebSoc...(HYPHENATED_LY_ADVERB_ADJECTIVE)
[style] ~13-~13: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... 'ably/modular'). -src/ReactApp.tsximports React hooks from the ably-js package (...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~13-~13: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...ctApp.tsximports React hooks from the ably-js package (import { useChannel } from 'a...(HYPHENATED_LY_ADVERB_ADJECTIVE)
ably.d.ts (8)
2031-2033: Add JSDoc documentation for LiveObjects interface and methods.The interface and its method lack proper documentation. Consider adding:
- Interface description explaining the purpose of LiveObjects
- Method documentation explaining what getRoot() does and when to use it
+/** + * Provides access to LiveObjects functionality, allowing real-time state synchronization + * between clients. + */ export declare interface LiveObjects { + /** + * Returns the root LiveMap object that serves as the entry point for LiveObjects state. + * @returns A promise that resolves with the root LiveMap instance. + */ getRoot(): Promise<LiveMap>; }
2037-2040: Enhance LiveMap interface with documentation and type safety.The interface needs documentation and could benefit from improved type safety:
- Add JSDoc documentation for interface and methods
- Consider making the interface generic to provide type-safe access to values
+/** + * Represents a synchronized map of key-value pairs that can be observed for changes. + * @template T The type of values stored in the map + */ -export declare interface LiveMap extends LiveObject<LiveMapUpdate> { +export declare interface LiveMap<T = StateValue | LiveObject> extends LiveObject<LiveMapUpdate> { + /** + * Retrieves the value associated with the specified key. + * @param key The key to look up + * @returns The value if found, undefined otherwise + */ - get(key: string): LiveObject | StateValue | undefined; + get(key: string): T | undefined; + /** + * Returns the number of key-value pairs in the map. + * @returns The size of the map + */ size(): number; }
2042-2044: Add documentation and improve type safety of LiveMapUpdate interface.The interface needs documentation and could have more specific types:
+/** + * Represents an update event for a LiveMap, indicating which keys were modified. + */ export declare interface LiveMapUpdate extends LiveObjectUpdate { + /** + * A map of key names to their update type. + * - 'updated': The value for this key was updated + * - 'removed': The key was removed from the map + */ - update: { [keyName: string]: 'updated' | 'removed' }; + update: Record<string, 'updated' | 'removed'>; }
2048-2050: Add documentation for LiveCounter interface.Add JSDoc documentation to explain the purpose and usage of the LiveCounter interface.
+/** + * Represents a synchronized counter that can be observed for changes. + * Useful for maintaining distributed counts across multiple clients. + */ export declare interface LiveCounter extends LiveObject<LiveCounterUpdate> { + /** + * Returns the current value of the counter. + * @returns The current numeric value of the counter + */ value(): number; }
2052-2054: Add documentation for LiveCounterUpdate interface.Add JSDoc documentation to explain the update structure for counter changes.
+/** + * Represents an update event for a LiveCounter, containing the increment/decrement value. + */ export declare interface LiveCounterUpdate extends LiveObjectUpdate { + /** + * The update payload containing the increment value. + * Positive values increment the counter, negative values decrement it. + */ update: { inc: number }; }
2056-2060: Enhance LiveObject interface with documentation and specific return types.The interface needs documentation and could benefit from more specific types:
+/** + * Base interface for all live objects that can be observed for changes. + * @template TUpdate The type of update events emitted by this live object + */ export declare interface LiveObject<TUpdate extends LiveObjectUpdate = LiveObjectUpdate> { + /** + * Subscribes to updates from this live object. + * @param listener The callback function that will be called with updates + * @returns A subscription response object that can be used to unsubscribe + */ subscribe(listener: (update: TUpdate) => void): SubscribeResponse; + /** + * Removes a specific update listener. + * @param listener The callback function to remove + */ unsubscribe(listener: (update: TUpdate) => void): void; + /** + * Removes all update listeners from this live object. + */ unsubscribeAll(): void; }
2066-2068: Add documentation for SubscribeResponse interface.Add JSDoc documentation to explain the purpose and usage of the subscription response.
+/** + * Represents a subscription to a live object, providing a way to unsubscribe. + */ export declare interface SubscribeResponse { + /** + * Cancels the subscription, stopping further update notifications. + */ unsubscribe(): void; }
2035-2035: Address TODO comment about documenting public API.The TODO comment indicates missing documentation for the LiveObjects public API. This should be addressed before merging, as proper documentation is crucial for the public API.
Would you like me to help generate comprehensive documentation for the LiveObjects public API?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
ably.d.ts(1 hunks)test/package/browser/template/README.md(2 hunks)test/package/browser/template/package.json(1 hunks)test/package/browser/template/server/resources/index-liveobjects.html(1 hunks)test/package/browser/template/server/server.ts(1 hunks)test/package/browser/template/src/index-liveobjects.ts(1 hunks)test/package/browser/template/src/sandbox.ts(1 hunks)test/package/browser/template/test/lib/package.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- test/package/browser/template/package.json
- test/package/browser/template/server/resources/index-liveobjects.html
- test/package/browser/template/server/server.ts
- test/package/browser/template/src/sandbox.ts
- test/package/browser/template/test/lib/package.test.ts
🧰 Additional context used
🪛 LanguageTool
test/package/browser/template/README.md
[uncategorized] ~11-~11: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...liveobjects.ts imports the LiveObjects ably-js plugin (import LiveObjects from 'ably/...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
[uncategorized] ~29-~29: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...ntaining src/index-liveobjects.ts and ably-js. 3. a bundle containing `src/index-mo...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
[uncategorized] ~30-~30: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...e containing src/index-modular.ts and ably-js. - test: Using the bundles created by...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
🪛 Biome (1.9.4)
test/package/browser/template/src/index-liveobjects.ts
[error] 27-27: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (3)
test/package/browser/template/README.md (1)
11-11: LGTM! Import example is well documented.
The added LiveObjects import example is clear, consistent with other examples, and correctly documents the import syntax for the LiveObjects plugin.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~11-~11: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...liveobjects.ts imports the LiveObjects ably-js plugin (import LiveObjects from 'ably/...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
ably.d.ts (2)
2046-2046: Reiterating past review comment about Buffer usage.
The previous review comment about replacing Buffer with ArrayBuffer is still valid and should be addressed.
2062-2064: Reiterating past review comment about 'any' type usage.
The previous review comment about avoiding the 'any' type is still valid and should be addressed.
Test that LiveObjects plugin can be imported and provided to the Ably client, and that TypeScript types for LiveObjects work as expected.
Also update
ably.d.tsto include latest public API for LiveObjects (no documentation yet, this is solely to support the tests).Summary by CodeRabbit
Release Notes
New Features
LiveObjectsfunctionality, enhancing real-time data management within the Ably SDK.LiveMap,LiveCounter, andLiveObject, allowing for improved interaction with live data structures.Documentation
Bug Fixes
createSandboxAblyAPIKeyfunction to accept additional options for improved customization.Tests