[DTP-947] Expose LiveObjects as a plugin #1880
Conversation
WalkthroughThe changes introduce a new Live Objects functionality to the Ably JavaScript client library, enhancing real-time data handling capabilities. This includes the addition of a LiveObjects plugin, new configurations in the build process, updated type definitions, and comprehensive documentation. The modifications span multiple files, integrating the LiveObjects feature into the existing framework while maintaining overall structure and functionality. Changes
Possibly related PRs
Suggested reviewers
Poem
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/1880/bundle-report
VeskeR
force-pushed
the
DTP-947/liveobjects-plugin-base
branch
from
57cbb82 to
f0bb1ba
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1880/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1880/features
VeskeR
force-pushed
the
DTP-947/liveobjects-plugin-base
branch
from
f0bb1ba to
f3490a0
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1880/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1880/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: 3
🧹 Outside diff range and nitpick comments (23)
src/plugins/liveobjects/index.ts (2)
5-7: LGTM: Default export is correctly implemented. Consider a minor style improvement.The default export of an object containing the
LiveObjectsclass is correct and provides flexibility for future expansion of the module. This approach, combined with the named export, offers maximum flexibility for module consumers.Consider using the object property shorthand for a more concise syntax:
export default { - LiveObjects, + LiveObjects };This change is purely stylistic and doesn't affect functionality.
1-7: Overall assessment: Well-structured plugin implementation.This file successfully introduces the LiveObjects plugin, aligning with the PR objectives. The implementation follows best practices for TypeScript modules and provides flexibility for consumers through both named and default exports. The structure is consistent with other plugins in the project, such as the Web Push plugin.
As the project grows, consider maintaining a consistent pattern for plugin exports across all plugins to ensure uniformity and ease of use throughout the codebase.
src/plugins/index.d.ts (2)
5-5: LGTM! Consider updating documentation.The new
LiveObjectsproperty in theStandardPluginsinterface is correctly implemented and aligns with the existing structure.Consider updating any relevant documentation or README files to reflect the addition of the LiveObjects plugin to the standard plugins.
Line range hint
1-8: Summary: LiveObjects plugin successfully integratedThe changes in this file effectively integrate the new LiveObjects plugin into the existing plugin system. The import statement and the addition to the
StandardPluginsinterface are consistent with the PR objectives and follow the established patterns in the codebase.This change maintains the modularity of the plugin system, allowing for easy addition or removal of plugins. It's a good architectural decision that promotes flexibility and extensibility in the Ably JavaScript library.
src/plugins/liveobjects/liveobjects.ts (2)
8-11: LGTM with suggestions: Constructor is functional but could be more robust.The constructor correctly initializes the private properties using the provided
channelparameter. However, consider adding null checks or default values to handle potential edge cases wherechannelorchannel.clientmight be null or undefined.Consider adding null checks or using the non-null assertion operator if you're certain these values will always be defined:
constructor(channel: RealtimeChannel) { if (!channel || !channel.client) { throw new Error('Invalid channel or client'); } this._channel = channel; this._client = channel.client; }Or, if you prefer a more concise approach and are certain of the values:
constructor(channel: RealtimeChannel) { this._channel = channel; this._client = channel.client!; }
1-12: Good start, but consider adding documentation and outlining future work.The
LiveObjectsclass provides a basic structure for the LiveObjects plugin, which aligns with the PR objective. However, the current implementation is minimal and lacks methods for interacting with LiveObjects.Consider adding the following:
- A class-level JSDoc comment explaining the purpose and intended use of the
LiveObjectsclass.- TODO comments or a separate documentation file outlining the planned methods and functionality for this class.
This additional context would be helpful for other developers and for tracking the progress of the LiveObjects plugin implementation.
liveobjects.d.ts (1)
7-25: Documentation is comprehensive, but could be more specific about LiveObjects functionality.The documentation provides clear examples for using the LiveObjects plugin with both
RealtimeClientandBaseRealtime. The import statements and client initialization examples are correct and follow best practices.Consider adding a brief description of what specific functionality the LiveObjects plugin adds to the client. This would give developers a clearer understanding of the plugin's purpose and capabilities without having to refer to external documentation.
src/common/lib/client/modularplugins.ts (1)
36-36: LGTM: LiveObjects property added to ModularPlugins interface.The
LiveObjectsproperty is correctly typed and marked as optional, consistent with other plugin properties. This addition enables the integration of the LiveObjects plugin into the modular plugin system.For consistency with other properties, consider adding a comment describing the purpose of the LiveObjects plugin, similar to the comments present for other plugins in this file.
test/realtime/live_objects.test.js (3)
1-11: Remove redundant 'use strict' directiveThe 'use strict' directive on line 1 is redundant in this context. JavaScript modules are automatically in strict mode, so this statement is not needed.
Apply this diff to remove the redundant directive:
-'use strict'; define(['ably', 'shared_helper', 'async', 'chai', 'live_objects'], function ( Ably, Helper, async, chai, LiveObjectsPlugin, ) { var expect = chai.expect; var createPM = Ably.protocolMessageFromDeserialized;🧰 Tools
🪛 Biome
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
40-56: Well-structured test for behavior without LiveObjects pluginThis test suite correctly verifies the expected behavior when the LiveObjects plugin is not provided. The use of
monitorConnectionThenCloseAndFinishensures proper cleanup after the test.Consider using a more specific assertion for the error message. Instead of just checking if an error is thrown, you could verify the exact error message:
expect(() => channel.liveObjects).to.throw('LiveObjects plugin not provided');This would make the test more robust against potential changes in the error message.
58-73: Effective test for behavior with LiveObjects pluginThis test suite effectively verifies the expected behavior when the LiveObjects plugin is provided. The use of helper functions
LiveObjectsRealtimeandmonitorConnectionThenCloseAndFinishis appropriate and ensures proper setup and cleanup.While this test covers the basic functionality, consider adding more comprehensive tests:
- Test different channel names to ensure the plugin works across various channels.
- Verify that the LiveObjects instance has the expected methods or properties.
- Test the behavior when creating multiple channels with LiveObjects.
Example:
it('works with multiple channels', async function () { const helper = this.test.helper; const client = LiveObjectsRealtime(helper); await monitorConnectionThenCloseAndFinish( helper, async () => { const channel1 = client.channels.get('channel1'); const channel2 = client.channels.get('channel2'); expect(channel1.liveObjects.constructor.name).to.equal('LiveObjects'); expect(channel2.liveObjects.constructor.name).to.equal('LiveObjects'); expect(channel1.liveObjects).to.not.equal(channel2.liveObjects); }, client, ); });These additional tests would provide more confidence in the plugin's functionality across different scenarios.
grunt/esbuild/build.js (4)
80-85: LGTM! Consider adding a comment for clarity.The
liveObjectsPluginConfigis correctly implemented, following the same pattern as other plugin configurations. It properly sets up the build for the LiveObjects plugin.Consider adding a brief comment above this configuration to explain its purpose, similar to how you might document a function. For example:
// Configuration for building the LiveObjects plugin const liveObjectsPluginConfig = { // ... (rest of the configuration) };This would improve code readability and maintainability.
87-92: LGTM! Consider adding a comment for consistency.The
liveObjectsPluginCdnConfigis correctly implemented, following the same pattern as other CDN plugin configurations. It properly sets up the build for the LiveObjects plugin CDN version.For consistency with the previous suggestion, consider adding a brief comment above this configuration as well:
// Configuration for building the LiveObjects plugin CDN version const liveObjectsPluginCdnConfig = { // ... (rest of the configuration) };This would maintain consistency in documentation across all configurations.
94-100: LGTM! Consider adding a comment for consistency.The
minifiedLiveObjectsPluginCdnConfigis correctly implemented, following the same pattern as other minified CDN plugin configurations. It properly sets up the build for the minified LiveObjects plugin CDN version.For consistency with the previous suggestions, consider adding a brief comment above this configuration as well:
// Configuration for building the minified LiveObjects plugin CDN version const minifiedLiveObjectsPluginCdnConfig = { // ... (rest of the configuration) };This would complete the documentation for all LiveObjects plugin configurations.
Line range hint
80-112: Overall, the LiveObjects plugin configurations are well-implemented.The additions to this file correctly set up the build configurations for the new LiveObjects plugin. The implementations follow the existing patterns in the file, ensuring consistency with other plugins. The configurations cover the standard, CDN, and minified CDN versions of the plugin, which is in line with the project's build strategy.
These changes will enable the building of the LiveObjects plugin in various formats, making it accessible for different use cases (e.g., standard usage, CDN distribution, and minified CDN distribution for production environments).
To further improve the maintainability of this file, consider:
- Grouping related configurations together (e.g., all LiveObjects configurations in one block).
- Adding a comment block at the top of the file explaining the purpose of these configurations and how they relate to the build process.
These suggestions would enhance the overall structure and readability of the build configuration file.
package.json (1)
146-146: LGTM: Build scripts updated for LiveObjects.The changes to the scripts section are appropriate:
- The "test:node" script now includes building LiveObjects before running tests.
- A new "build:liveobjects" script has been added for dedicated LiveObjects building.
These modifications align with the PR objectives and follow the existing pattern for other plugins.
Consider updating the "build" script to include building LiveObjects as well, ensuring it's built when running a full build:
- "build": "grunt build:all && npm run build:react", + "build": "grunt build:all && npm run build:react && npm run build:liveobjects",Also applies to: 160-160
Gruntfile.js (1)
148-162: LGTM: 'build:liveobjects' task implemented correctlyThe new 'build:liveobjects' task is well-structured and consistent with other build tasks in the file. It correctly uses esbuild to build the necessary configurations for the LiveObjects plugin.
For consistency with other build tasks, consider adding a console log message on success:
.then(() => { + console.log('LiveObjects build succeeded'); done(true); })README.md (3)
589-590: Consider rephrasing the section header for consistency.The current header "Live Objects functionality" could be more consistent with other sections in the README. Consider changing it to "Live Objects" or "Using Live Objects" to match the style of other feature sections.
-### Live Objects functionality +### Using Live Objects
591-592: Improve wording for clarity.The current wording can be simplified for better readability.
-Live Objects functionality is supported for Realtime clients via the LiveObjects plugin. In order to use Live Objects, you must pass in the plugin via client options. +Live Objects is supported for Realtime clients via the LiveObjects plugin. To use Live Objects, you must pass the plugin via client options.🧰 Tools
🪛 LanguageTool
[style] ~591-~591: Consider a shorter alternative to avoid wordiness.
Context: ...ime clients via the LiveObjects plugin. In order to use Live Objects, you must pass in the ...(IN_ORDER_TO_PREMIUM)
620-621: Add missing comma and fix formatting.There's a missing comma in the sentence, and the formatting of the version numbers can be improved for consistency.
-The LiveObjects plugin is developed as part of the Ably client library, so it is available for the same versions as the Ably client library itself. It also means that it follows the same semantic versioning rules as they were defined for [the Ably client library](#for-browsers). For example, to lock into a major or minor version of the LiveObjects plugin, you can specify a specific version number such as https://cdn.ably.com/lib/liveobjects.umd.min-2.js for all v2._ versions, or https://cdn.ably.com/lib/liveobjects.umd.min-2.4.js for all v2.4._ versions, or you can lock into a single release with https://cdn.ably.com/lib/liveobjects.umd.min-2.4.0.js. Note you can load the non-minified version by omitting `.min` from the URL such as https://cdn.ably.com/lib/liveobjects.umd-2.js. +The LiveObjects plugin is developed as part of the Ably client library, so it is available for the same versions as the Ably client library itself. It also means that it follows the same semantic versioning rules as defined for [the Ably client library](#for-browsers). For example, to lock into a major or minor version of the LiveObjects plugin, you can specify a specific version number such as `https://cdn.ably.com/lib/liveobjects.umd.min-2.js` for all v2._ versions, or `https://cdn.ably.com/lib/liveobjects.umd.min-2.4.js` for all v2.4._ versions, or you can lock into a single release with `https://cdn.ably.com/lib/liveobjects.umd.min-2.4.0.js`. Note, you can load the non-minified version by omitting `.min` from the URL, such as `https://cdn.ably.com/lib/liveobjects.umd-2.js`.🧰 Tools
🪛 LanguageTool
[uncategorized] ~620-~620: Possible missing comma found.
Context: ...y.com/lib/liveobjects.umd.min-2.4.0.js. Note you can load the non-minified version b...(AI_HYDRA_LEO_MISSING_COMMA)
🪛 Markdownlint
620-620: null
Bare URL used(MD034, no-bare-urls)
620-620: null
Bare URL used(MD034, no-bare-urls)
620-620: null
Bare URL used(MD034, no-bare-urls)
620-620: null
Bare URL used(MD034, no-bare-urls)
620-620: null
Spaces inside emphasis markers(MD037, no-space-in-emphasis)
ably.d.ts (1)
626-630: Approved: LiveObjects interface addedThe addition of the
LiveObjectsinterface is a good start for implementing the new LiveObjects functionality. However, as it's currently empty, it doesn't provide any actual functionality yet.Consider adding a TODO comment within the interface to indicate future implementation plans, for example:
export declare interface LiveObjects { // TODO: Implement LiveObjects functionality }scripts/moduleReport.ts (1)
186-208: Enhance Unit Test Coverage for New FunctionsThe newly introduced functions
calculatePluginSize(),calculatePushPluginSize(), andcalculateLiveObjectsPluginSize()are important for plugin size reporting. Consider adding unit tests to cover these functions, ensuring they handle various scenarios correctly.Would you like assistance in drafting unit tests for these functions?
src/common/lib/client/realtimechannel.ts (1)
155-160: Consistency in error handling within 'liveObjects' getterThe
liveObjectsgetter throws an error usingUtils.throwMissingPluginError('LiveObjects')if_liveObjectsis not initialized. Confirm that this approach is consistent with error handling for other optional plugins and that the error message provided is clear and helpful to the developer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
- Gruntfile.js (2 hunks)
- README.md (1 hunks)
- ably.d.ts (3 hunks)
- grunt/esbuild/build.js (2 hunks)
- liveobjects.d.ts (1 hunks)
- package.json (3 hunks)
- scripts/cdn_deploy.js (1 hunks)
- scripts/moduleReport.ts (4 hunks)
- src/common/lib/client/modularplugins.ts (2 hunks)
- src/common/lib/client/realtimechannel.ts (4 hunks)
- src/plugins/index.d.ts (1 hunks)
- src/plugins/liveobjects/index.ts (1 hunks)
- src/plugins/liveobjects/liveobjects.ts (1 hunks)
- test/common/globals/named_dependencies.js (1 hunks)
- test/realtime/live_objects.test.js (1 hunks)
- test/support/browser_file_list.js (1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~591-~591: Consider a shorter alternative to avoid wordiness.
Context: ...ime clients via the LiveObjects plugin. In order to use Live Objects, you must pass in the ...(IN_ORDER_TO_PREMIUM)
[uncategorized] ~620-~620: Possible missing comma found.
Context: ...y.com/lib/liveobjects.umd.min-2.4.0.js. Note you can load the non-minified version b...(AI_HYDRA_LEO_MISSING_COMMA)
🪛 Markdownlint
README.md
620-620: null
Bare URL used(MD034, no-bare-urls)
620-620: null
Bare URL used(MD034, no-bare-urls)
620-620: null
Bare URL used(MD034, no-bare-urls)
620-620: null
Bare URL used(MD034, no-bare-urls)
620-620: null
Spaces inside emphasis markers(MD037, no-space-in-emphasis)
593-593: Expected: indented; Actual: fenced
Code block style(MD046, code-block-style)
607-607: Expected: indented; Actual: fenced
Code block style(MD046, code-block-style)
613-613: Expected: indented; Actual: fenced
Code block style(MD046, code-block-style)
🪛 Biome
test/realtime/live_objects.test.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
🔇 Additional comments (35)
src/plugins/liveobjects/index.ts (2)
1-1: LGTM: Import statement is correct and follows best practices.The import statement correctly imports the
LiveObjectsclass from a local file. The use of a relative path for local imports is a good practice.
3-3: LGTM: Named export is correctly implemented.The named export of the
LiveObjectsclass is correctly implemented. This allows consumers of the module to selectively import theLiveObjectsclass if needed.src/plugins/index.d.ts (1)
1-1: LGTM! Verify the existence of the imported file.The import statement for
LiveObjectsis correctly formatted and aligns with the PR objectives to introduce a new LiveObjects plugin.Let's verify the existence of the imported file:
src/plugins/liveobjects/liveobjects.ts (2)
1-2: LGTM: Import statements are correct and follow best practices.The import statements are properly using the
typekeyword for TypeScript type imports, which is a good practice. The imported types match the class properties, ensuring type safety.
4-6: LGTM: Class definition and properties are well-structured.The
LiveObjectsclass is properly defined with two private properties,_clientand_channel. The use of theprivatekeyword and underscore prefix for private members follows TypeScript best practices. The property types correctly match the imported types, ensuring type safety.test/common/globals/named_dependencies.js (1)
14-17: LGTM! Verify build setup for LiveObjects plugin.The addition of the
live_objectsentry is correct and consistent with the file's structure and purpose. It aligns with the PR objective of introducing a LiveObjects plugin.To ensure the build setup is correct, please run the following script:
This script will help verify that the necessary build files and configurations for the LiveObjects plugin are in place.
✅ Verification successful
Build setup for LiveObjects plugin verified.
The
builddirectory is correctly ignored, and all necessary build scripts for the LiveObjects plugin are properly configured.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of LiveObjects build files and related configurations. # Test 1: Check if the LiveObjects build files exist echo "Checking LiveObjects build files..." fd -t f "liveobjects" build # Test 2: Verify if LiveObjects is mentioned in build configuration echo "Checking build configuration..." fd -t f "rollup.config.js" | xargs rg "liveobjects" # Test 3: Look for LiveObjects-related npm scripts echo "Checking npm scripts..." rg '"build:.*liveobjects"' package.jsonLength of output: 3528
Script:
#!/bin/bash # Description: Check if the 'build' directory is ignored and verify build script configurations. # Test 4: Check if 'build' is listed in .gitignore echo "Checking if 'build' is ignored in .gitignore..." rg "^build(/|$)" .gitignore # Test 5: Verify if build scripts are set to generate 'build/liveobjects' echo "Verifying build scripts for LiveObjects..." rg "build:liveobjects" package.json rg "grunt.registerTask\('build:liveobjects'" Gruntfile.jsLength of output: 551
liveobjects.d.ts (2)
1-5: LGTM: ESLint directives and imports are correctly implemented.The ESLint directives are appropriately used to suppress warnings for unused variables, as these types are only referenced in documentation comments. The imports for
RealtimeClientandBaseRealtimeare correctly placed and necessary for the documentation.
28-28: LGTM: Export statement is correct.The
export = LiveObjects;statement correctly exports theLiveObjectsconstant as the default export. This aligns with the import syntax shown in the documentation examples, allowing users to import the plugin as demonstrated.src/common/lib/client/modularplugins.ts (3)
14-14: LGTM: Import statement for LiveObjectsPlugin added.The import statement for
LiveObjectsPluginis correctly formatted and aligns with the PR objective of introducing a new LiveObjects plugin.
Line range hint
1-39: Summary: LiveObjects plugin successfully integrated into modular plugin systemThe changes in this file successfully introduce the LiveObjects plugin to the modular plugin system. The implementation follows existing patterns and is consistent with other plugins. The modifications are minimal and focused, aligning well with the PR objectives.
Key points:
- Import statement for LiveObjects plugin added.
- LiveObjects property added to ModularPlugins interface.
- No changes made to allCommonModularPlugins constant.
These changes lay the groundwork for using the LiveObjects plugin in the Ably JavaScript library.
39-39: Verify: LiveObjects not initialized in allCommonModularPluginsThe
LiveObjectsplugin is not initialized in theallCommonModularPluginsconstant. This might be intentional, following the pattern of other plugins like Push. However, it's worth confirming if this is the desired behavior or if it should be initialized here.To check if other plugins follow a similar pattern, we can run the following script:
test/realtime/live_objects.test.js (2)
13-23: Well-structured helper functionsThe helper functions
LiveObjectsRealtimeandmonitorConnectionThenCloseAndFinishare well-defined and serve clear purposes. They make good use of modern JavaScript features like object spread and async/await, which enhances readability and maintainability.
25-38: Appropriate test suite setupThe test suite setup is well-structured:
- The 60-second timeout is reasonable for realtime tests.
- The app setup is efficiently done once before all tests.
- Error handling in the setup is appropriate.
test/support/browser_file_list.js (1)
42-42: LGTM: New test file added for LiveObjects functionality.The addition of
'test/realtime/live_objects.test.js': true,is consistent with the existing pattern in this file and aligns with the PR objectives of introducing a new LiveObjects plugin.To ensure the new test file exists and is properly integrated, run the following script:
✅ Verification successful
Verified: New test file is properly integrated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the new test file and its integration in the test suite. # Test 1: Check if the file exists if [ -f "test/realtime/live_objects.test.js" ]; then echo "Test file 'live_objects.test.js' exists." else echo "Error: Test file 'live_objects.test.js' does not exist." exit 1 fi # Test 2: Check if the file is imported or required in any test runner files rg --type js "live_objects\.test\.js" test/Length of output: 270
grunt/esbuild/build.js (1)
110-112: LGTM! Exports are correctly updated.The module exports have been properly updated to include the new LiveObjects plugin configurations. The additions are consistent with the existing pattern and maintain the correct order.
scripts/cdn_deploy.js (2)
Line range hint
1-138: Ensure comprehensive testing of the deployment process.While the change to include the LiveObjects plugin file is minimal and focused, it's crucial to verify that the entire deployment process works correctly with this new file type.
Please perform a full test deployment to ensure:
- The new LiveObjects files are correctly identified and uploaded.
- Versioning is applied correctly to the new files.
- The S3 upload process works as expected for the new file type.
- The deployment doesn't negatively impact existing file types (ably and push.umd).
Consider adding or updating any relevant unit tests or integration tests to cover the new file type in the deployment process.
24-24: LGTM! Verify handling of new file type.The change to include 'liveobjects.umd' in the file regex is appropriate and aligns with the PR objective of introducing a new LiveObjects plugin. This modification follows the pattern established for the Web Push plugin while maintaining support for existing file types.
To ensure the new file type is correctly handled, please run the following verification script:
This script will help confirm that the new file type is present and correctly matches the updated regex pattern.
package.json (3)
33-35: LGTM: New export for LiveObjects added correctly.The new export for "./liveobjects" has been added following the existing pattern. It correctly specifies the types and import path, which aligns with the PR objective of exposing LiveObjects as a plugin.
41-41: LGTM: LiveObjects TypeScript definitions included.The addition of "liveobjects.d.ts" to the files array ensures that the TypeScript definitions for the new LiveObjects functionality are included in the package. This is consistent with the PR objectives and maintains the existing structure.
32-35: Summary: LiveObjects integration looks good overall.The changes to
package.jsonsuccessfully integrate the LiveObjects functionality:
- New export added for LiveObjects.
- TypeScript definitions included in the package.
- Build and test scripts updated appropriately.
These modifications align well with the PR objectives and follow existing patterns in the package. Consider the minor suggestion to update the main "build" script, but otherwise, the changes look good to proceed.
Also applies to: 41-41, 146-146, 160-160
Gruntfile.js (3)
76-83: LGTM: 'build:liveobjects' task added correctlyThe addition of the 'build:liveobjects' subtask to the main 'build' task is correct and aligns with the PR objective of introducing the LiveObjects plugin. The placement at the end of the task list ensures it doesn't interfere with existing build steps.
167-167: LGTM: 'build:liveobjects' added to 'test:webserver' taskThe addition of 'build:liveobjects' to the 'test:webserver' task is correct and ensures that the LiveObjects plugin is built before running the web server for testing. The placement is consistent with the order of tasks in the main 'build' task.
Line range hint
76-167: Overall assessment: LiveObjects plugin successfully integrated into the build processThe changes made to integrate the LiveObjects plugin into the build process are well-implemented and consistent with the existing structure of the Gruntfile. The new 'build:liveobjects' task is correctly added to both the main 'build' task and the 'test:webserver' task, ensuring that the plugin is built alongside other components and included in the testing process.
The implementation follows the patterns established for other plugins (e.g., the Push plugin) and maintains the overall integrity of the build system. These changes effectively support the PR objective of introducing the LiveObjects plugin to the Ably JavaScript library.
README.md (6)
593-601: LGTM: Code example for module import.The code example for importing and using the LiveObjects plugin with module imports is clear and correct.
🧰 Tools
🪛 Markdownlint
593-593: Expected: indented; Actual: fenced
Code block style(MD046, code-block-style)
603-604: LGTM: Modular variant compatibility.The information about compatibility with the modular variant is useful and consistent with other sections.
605-609: LGTM: Script tag loading example.The example for loading the LiveObjects plugin using a script tag is clear and correct.
🧰 Tools
🪛 Markdownlint
607-607: Expected: indented; Actual: fenced
Code block style(MD046, code-block-style)
611-618: LGTM: Global object usage example.The explanation and code example for using the LiveObjects plugin when loaded via a script tag are clear and correct.
🧰 Tools
🪛 Markdownlint
613-613: Expected: indented; Actual: fenced
Code block style(MD046, code-block-style)
622-623: LGTM: Documentation link.The link to the Ably Live Objects documentation is correct and provides a good resource for more information.
589-623: Overall assessment of the Live Objects section.The new Live Objects section is a valuable addition to the README.md file. It provides clear instructions for using the LiveObjects plugin with both module imports and script tag loading. The content is well-structured and consistent with other sections of the document. The minor suggestions provided will help improve clarity and maintain consistency throughout the README.
🧰 Tools
🪛 LanguageTool
[style] ~591-~591: Consider a shorter alternative to avoid wordiness.
Context: ...ime clients via the LiveObjects plugin. In order to use Live Objects, you must pass in the ...(IN_ORDER_TO_PREMIUM)
[uncategorized] ~620-~620: Possible missing comma found.
Context: ...y.com/lib/liveobjects.umd.min-2.4.0.js. Note you can load the non-minified version b...(AI_HYDRA_LEO_MISSING_COMMA)
🪛 Markdownlint
620-620: null
Bare URL used(MD034, no-bare-urls)
620-620: null
Bare URL used(MD034, no-bare-urls)
620-620: null
Bare URL used(MD034, no-bare-urls)
620-620: null
Bare URL used(MD034, no-bare-urls)
620-620: null
Spaces inside emphasis markers(MD037, no-space-in-emphasis)
593-593: Expected: indented; Actual: fenced
Code block style(MD046, code-block-style)
607-607: Expected: indented; Actual: fenced
Code block style(MD046, code-block-style)
613-613: Expected: indented; Actual: fenced
Code block style(MD046, code-block-style)
ably.d.ts (2)
2152-2155: Approved: LiveObjects property added to RealtimeChannelThe addition of the
liveObjectsproperty to theRealtimeChannelinterface is appropriate and aligns with the PR objective of exposing LiveObjects as a plugin. This will allow users to access LiveObjects functionality for a specific channel.
626-630: Approved: LiveObjects plugin added to CorePluginsThe addition of the
LiveObjectsproperty to theCorePluginsinterface is appropriate and consistent with how other plugins are defined. Usingunknownas the type allows for flexibility in the plugin's implementation.As the LiveObjects plugin development progresses, consider replacing
unknownwith a more specific type that describes the plugin's structure and functionality.scripts/moduleReport.ts (1)
311-311: Verify Completeness ofallowedFilesfor LiveObjects PluginEnsure that the
allowedFilesset includes all source files that significantly contribute to the LiveObjects plugin bundle size. If there are additional files expected to be part of the plugin, they should be added to prevent unintended errors during bundle size checks.src/common/lib/client/realtimechannel.ts (3)
32-32: Import statement for 'LiveObjects'The import of the
LiveObjectstype from'plugins/liveobjects'is correctly declared.
103-103: Declaration of the optional '_liveObjects' propertyThe optional private property
_liveObjectsof typeLiveObjectsis appropriately declared.
143-145: Verify the instantiation of the 'LiveObjects' pluginEnsure that the
LiveObjectsclass is correctly exported and that the instantiationnew client.options.plugins.LiveObjects.LiveObjects(this)is accurate. Verify thatLiveObjects.LiveObjectsis the correct path to the constructor.
VeskeR
force-pushed
the
DTP-947/liveobjects-plugin-base
branch
from
f3490a0 to
5d66652
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1880/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1880/features
VeskeR
force-pushed
the
DTP-947/liveobjects-plugin-base
branch
from
5d66652 to
fbf46a7
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1880/features
github-actions
bot
temporarily deployed
to
staging/pull/1880/bundle-report
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (9)
test/realtime/live_objects.test.js (2)
1-11: Remove redundant 'use strict' directive and approve importsThe 'use strict' directive on line 1 is redundant in modern JavaScript modules. Consider removing it as the entire contents of JavaScript modules are automatically in strict mode.
The imports look good and appropriate for the testing requirements.
Apply this diff to remove the redundant 'use strict' directive:
-'use strict'; define(['ably', 'shared_helper', 'async', 'chai', 'live_objects'], function ( Ably, Helper, async, chai, LiveObjectsPlugin, ) { var expect = chai.expect; var createPM = Ably.protocolMessageFromDeserialized;🧰 Tools
🪛 Biome
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
58-75: Second test suite is well-implemented with a minor suggestionThe "Realtime with LiveObjects plugin" test suite is well-structured and effectively tests the positive scenario:
- It correctly tests the behavior when the LiveObjects plugin is used.
- The test case properly asserts that the
liveObjectsproperty returns an instance of LiveObjects.- The use of async/await and the
monitorConnectionThenCloseAndFinishhelper function ensures proper setup and cleanup.This test suite effectively covers the positive scenario, complementing the negative scenario tested in the first suite.
Consider adding an additional assertion to check if the
liveObjectsproperty is actually defined before checking its constructor name. This would make the test more robust:expect(channel.liveObjects).to.exist; expect(channel.liveObjects.constructor.name).to.equal('LiveObjects');grunt/esbuild/build.js (1)
110-112: LGTM: Module exports updated correctly.The new LiveObjects plugin configurations are properly added to the module exports. This ensures they are accessible for use in the build process.
Consider grouping related exports together for improved readability. For example:
module.exports = { webConfig, minifiedWebConfig, modularConfig, nodeConfig, pushPluginConfig, pushPluginCdnConfig, minifiedPushPluginCdnConfig, liveObjectsPluginConfig, liveObjectsPluginCdnConfig, minifiedLiveObjectsPluginCdnConfig, };This groups the main configs, push plugin configs, and live objects plugin configs together.
Gruntfile.js (1)
148-162: LGTM: LiveObjects build task implemented correctlyThe new 'build:liveobjects' task is well-structured and consistent with other plugin build tasks in the file. It correctly uses esbuild to build the necessary configurations for the LiveObjects plugin.
For improved consistency with other tasks, consider adding a console log message on success:
.then(() => { + console.log('LiveObjects build succeeded'); done(true); })This addition would make the task output consistent with the 'build:browser' task.
README.md (2)
589-591: Simplify wording for clarityConsider simplifying the introductory sentence for conciseness.
- Live Objects functionality is supported for Realtime clients via the LiveObjects plugin. In order to use Live Objects, you must pass in the plugin via client options. + Live Objects functionality is supported for Realtime clients via the LiveObjects plugin. To use Live Objects, pass in the plugin via client options.🧰 Tools
🪛 LanguageTool
[style] ~591-~591: Consider a shorter alternative to avoid wordiness.
Context: ...ime clients via the LiveObjects plugin. In order to use Live Objects, you must pass in the ...(IN_ORDER_TO_PREMIUM)
620-621: Improve readability of version informationConsider adding a comma and formatting the URLs as proper Markdown links for better readability.
- The LiveObjects plugin is developed as part of the Ably client library, so it is available for the same versions as the Ably client library itself. It also means that it follows the same semantic versioning rules as they were defined for [the Ably client library](#for-browsers). For example, to lock into a major or minor version of the LiveObjects plugin, you can specify a specific version number such as https://cdn.ably.com/lib/liveobjects.umd.min-2.js for all v2._ versions, or https://cdn.ably.com/lib/liveobjects.umd.min-2.4.js for all v2.4._ versions, or you can lock into a single release with https://cdn.ably.com/lib/liveobjects.umd.min-2.4.0.js. Note you can load the non-minified version by omitting `.min` from the URL such as https://cdn.ably.com/lib/liveobjects.umd-2.js. + The LiveObjects plugin is developed as part of the Ably client library, so it is available for the same versions as the Ably client library itself. It also means that it follows the same semantic versioning rules as they were defined for [the Ably client library](#for-browsers). For example, to lock into a major or minor version of the LiveObjects plugin, you can specify a specific version number such as [https://cdn.ably.com/lib/liveobjects.umd.min-2.js](https://cdn.ably.com/lib/liveobjects.umd.min-2.js) for all v2._ versions, or [https://cdn.ably.com/lib/liveobjects.umd.min-2.4.js](https://cdn.ably.com/lib/liveobjects.umd.min-2.4.js) for all v2.4._ versions, or you can lock into a single release with [https://cdn.ably.com/lib/liveobjects.umd.min-2.4.0.js](https://cdn.ably.com/lib/liveobjects.umd.min-2.4.0.js). Note, you can load the non-minified version by omitting `.min` from the URL such as [https://cdn.ably.com/lib/liveobjects.umd-2.js](https://cdn.ably.com/lib/liveobjects.umd-2.js).🧰 Tools
🪛 LanguageTool
[uncategorized] ~620-~620: Possible missing comma found.
Context: ...y.com/lib/liveobjects.umd.min-2.4.0.js. Note you can load the non-minified version b...(AI_HYDRA_LEO_MISSING_COMMA)
🪛 Markdownlint
620-620: null
Bare URL used(MD034, no-bare-urls)
620-620: null
Bare URL used(MD034, no-bare-urls)
620-620: null
Bare URL used(MD034, no-bare-urls)
620-620: null
Bare URL used(MD034, no-bare-urls)
620-620: null
Spaces inside emphasis markers(MD037, no-space-in-emphasis)
src/common/lib/client/realtimechannel.ts (1)
32-32: Summary: LiveObjects plugin integration is well-implemented.The changes to integrate the LiveObjects plugin into the RealtimeChannel class are well-executed:
- The import and property declarations are correctly typed.
- The initialization in the constructor follows established patterns for plugin integration.
- The getter method is implemented with proper error handling.
These changes enhance the functionality of the RealtimeChannel class without modifying existing behavior, maintaining backwards compatibility while adding new capabilities.
Consider documenting the LiveObjects functionality in the class-level JSDoc comments to help developers understand how to use this new feature.
Also applies to: 103-103, 142-145, 155-161
ably.d.ts (2)
2018-2021: LiveObjects interface added, but lacks structure or documentation.The addition of the
LiveObjectsinterface is a good start for exposing LiveObjects as a plugin. However, the interface is currently empty. Consider adding some basic structure or documentation to provide context for its usage.Consider adding documentation or placeholder methods to the
LiveObjectsinterface. For example:export declare interface LiveObjects { /** * Subscribes to LiveObjects state for the channel. * @param callback - A function to be called when the LiveObjects state changes. */ subscribe(callback: (state: any) => void): void; // Add other relevant methods or properties }
626-630: LiveObjects plugin added to CorePlugins interface.The addition of the
LiveObjectsplugin to theCorePluginsinterface is correct and consistent with the PR objective of exposing LiveObjects as a plugin.Consider using a more specific type for the
LiveObjectsplugin if one is available or planned, instead ofunknown. This could provide better type safety and autocompletion for users of the SDK. For example:LiveObjects?: LiveObjectsPlugin;Where
LiveObjectsPlugincould be a separate interface defining the structure of the plugin.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
- Gruntfile.js (2 hunks)
- README.md (1 hunks)
- ably.d.ts (3 hunks)
- grunt/esbuild/build.js (2 hunks)
- liveobjects.d.ts (1 hunks)
- package.json (3 hunks)
- scripts/cdn_deploy.js (1 hunks)
- scripts/moduleReport.ts (5 hunks)
- src/common/lib/client/modularplugins.ts (2 hunks)
- src/common/lib/client/realtimechannel.ts (4 hunks)
- src/plugins/index.d.ts (1 hunks)
- src/plugins/liveobjects/index.ts (1 hunks)
- src/plugins/liveobjects/liveobjects.ts (1 hunks)
- test/common/globals/named_dependencies.js (1 hunks)
- test/realtime/live_objects.test.js (1 hunks)
- test/support/browser_file_list.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- liveobjects.d.ts
- package.json
- scripts/cdn_deploy.js
- scripts/moduleReport.ts
- src/common/lib/client/modularplugins.ts
- src/plugins/index.d.ts
- src/plugins/liveobjects/index.ts
- src/plugins/liveobjects/liveobjects.ts
- test/common/globals/named_dependencies.js
- test/support/browser_file_list.js
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~591-~591: Consider a shorter alternative to avoid wordiness.
Context: ...ime clients via the LiveObjects plugin. In order to use Live Objects, you must pass in the ...(IN_ORDER_TO_PREMIUM)
[uncategorized] ~620-~620: Possible missing comma found.
Context: ...y.com/lib/liveobjects.umd.min-2.4.0.js. Note you can load the non-minified version b...(AI_HYDRA_LEO_MISSING_COMMA)
🪛 Markdownlint
README.md
620-620: null
Bare URL used(MD034, no-bare-urls)
620-620: null
Bare URL used(MD034, no-bare-urls)
620-620: null
Bare URL used(MD034, no-bare-urls)
620-620: null
Bare URL used(MD034, no-bare-urls)
620-620: null
Spaces inside emphasis markers(MD037, no-space-in-emphasis)
593-593: Expected: indented; Actual: fenced
Code block style(MD046, code-block-style)
607-607: Expected: indented; Actual: fenced
Code block style(MD046, code-block-style)
613-613: Expected: indented; Actual: fenced
Code block style(MD046, code-block-style)
🪛 Biome
test/realtime/live_objects.test.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
🔇 Additional comments (20)
test/realtime/live_objects.test.js (3)
13-23: Helper functions look goodThe
LiveObjectsRealtimeandmonitorConnectionThenCloseAndFinishfunctions are well-implemented:
LiveObjectsRealtimecorrectly creates a Realtime client with the LiveObjects plugin.monitorConnectionThenCloseAndFinishproperly manages the connection lifecycle and ensures cleanup, which is crucial for maintaining test isolation.These helper functions will contribute to more readable and maintainable tests.
25-56: First test suite is well-implementedThe "Realtime without LiveObjects plugin" test suite is well-structured and comprehensive:
- It correctly tests the behavior when the LiveObjects plugin is not used.
- The test case properly asserts that an error is thrown with the expected message when accessing the
liveObjectsproperty.- The use of async/await and the
monitorConnectionThenCloseAndFinishhelper function ensures proper setup and cleanup.This test suite effectively covers the negative scenario, which is crucial for ensuring correct plugin behavior.
1-76: Overall, excellent test implementation for LiveObjects pluginThis test file is well-structured and comprehensive, effectively validating the LiveObjects plugin functionality:
- It covers both positive and negative scenarios, ensuring robust testing of the new feature.
- The use of helper functions (
LiveObjectsRealtimeandmonitorConnectionThenCloseAndFinish) promotes code reuse and maintainability.- Proper use of async/await and cleanup procedures ensures test isolation and reliability.
- The tests are clear, concise, and follow good testing practices.
The implementation aligns well with the PR objectives, providing a solid foundation for the LiveObjects plugin integration into the Ably JavaScript library.
🧰 Tools
🪛 Biome
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
grunt/esbuild/build.js (4)
80-85: LGTM: LiveObjects plugin configuration added correctly.The
liveObjectsPluginConfigis well-structured and consistent with other plugin configurations. It correctly sets up the build process for the LiveObjects plugin.
87-92: LGTM: LiveObjects plugin CDN configuration added correctly.The
liveObjectsPluginCdnConfigis properly set up for CDN distribution, following the same pattern as other plugin CDN configurations.
94-100: LGTM: Minified LiveObjects plugin CDN configuration added correctly.The
minifiedLiveObjectsPluginCdnConfigis properly set up for minified CDN distribution, following the same pattern as other minified plugin CDN configurations.
Line range hint
80-112: Summary: LiveObjects plugin configurations successfully integrated.The changes to
grunt/esbuild/build.jssuccessfully integrate the new LiveObjects plugin configurations into the build process. The additions are consistent with existing patterns and correctly set up for various distribution scenarios (standard, CDN, and minified CDN).Great job on maintaining consistency with the existing codebase structure!
Gruntfile.js (2)
76-83: LGTM: LiveObjects build task integrated correctlyThe addition of 'build:liveobjects' to the main 'build' task is appropriate and aligns with the PR objectives. This ensures that the new LiveObjects plugin is built alongside other components of the library.
163-169: LGTM: LiveObjects build integrated into test webserver setupThe addition of 'build:liveobjects' to the 'test:webserver' task is appropriate. This ensures that the latest LiveObjects plugin build is available when running the test web server, which is crucial for comprehensive testing.
README.md (6)
593-601: LGTM: Clear example for module usageThe code example for using the LiveObjects plugin with module imports is clear and follows good practices.
🧰 Tools
🪛 Markdownlint
593-593: Expected: indented; Actual: fenced
Code block style(MD046, code-block-style)
603-604: LGTM: Informative note about Modular variant compatibilityThis note provides valuable information about compatibility with the Modular variant of the library.
605-609: LGTM: Clear instructions for script tag usageThe instructions for loading the LiveObjects plugin using a script tag are clear and helpful for users who can't use a package manager.
🧰 Tools
🪛 Markdownlint
607-607: Expected: indented; Actual: fenced
Code block style(MD046, code-block-style)
611-618: LGTM: Clear example for script tag usageThe code example for using the LiveObjects plugin when loaded via a script tag is clear and follows good practices.
🧰 Tools
🪛 Markdownlint
613-613: Expected: indented; Actual: fenced
Code block style(MD046, code-block-style)
622-623: LGTM: Clear link to documentationThe link to the Ably Live Objects documentation is clear and helpful for users seeking more information.
589-623: Overall, well-documented LiveObjects functionalityThe new section on LiveObjects functionality is a valuable addition to the README. It provides clear instructions, examples, and versioning information for both module and script tag usage. The minor suggestions for improvement in wording and formatting will enhance readability and consistency with the rest of the document.
🧰 Tools
🪛 LanguageTool
[style] ~591-~591: Consider a shorter alternative to avoid wordiness.
Context: ...ime clients via the LiveObjects plugin. In order to use Live Objects, you must pass in the ...(IN_ORDER_TO_PREMIUM)
[uncategorized] ~620-~620: Possible missing comma found.
Context: ...y.com/lib/liveobjects.umd.min-2.4.0.js. Note you can load the non-minified version b...(AI_HYDRA_LEO_MISSING_COMMA)
🪛 Markdownlint
620-620: null
Bare URL used(MD034, no-bare-urls)
620-620: null
Bare URL used(MD034, no-bare-urls)
620-620: null
Bare URL used(MD034, no-bare-urls)
620-620: null
Bare URL used(MD034, no-bare-urls)
620-620: null
Spaces inside emphasis markers(MD037, no-space-in-emphasis)
593-593: Expected: indented; Actual: fenced
Code block style(MD046, code-block-style)
607-607: Expected: indented; Actual: fenced
Code block style(MD046, code-block-style)
613-613: Expected: indented; Actual: fenced
Code block style(MD046, code-block-style)
src/common/lib/client/realtimechannel.ts (4)
32-32: LGTM: LiveObjects import added correctly.The import statement for LiveObjects is properly typed and consistent with other plugin imports in the file.
103-103: LGTM: _liveObjects property added correctly.The _liveObjects property is properly typed as an optional LiveObjects instance, following TypeScript best practices for private properties.
142-145: LGTM: _liveObjects initialized correctly in the constructor.The _liveObjects property is properly initialized only when the LiveObjects plugin is present in the client options. This approach is consistent with the initialization of other plugins and ensures that LiveObjects is only created when necessary.
155-161: LGTM: liveObjects getter method implemented correctly.The liveObjects getter method is well-implemented:
- It follows the same pattern as the existing push getter.
- It includes a safeguard against accessing the property when the plugin is not initialized.
- The error handling is consistent with other plugin-related errors in the class.
This implementation ensures that the LiveObjects functionality is safely exposed and follows the established patterns in the codebase.
ably.d.ts (1)
2152-2155: LiveObjects property added to RealtimeChannel interface.The addition of the
liveObjectsproperty to theRealtimeChannelinterface is correct and aligns with the PR objective of exposing LiveObjects as a plugin.
VeskeR
force-pushed
the
DTP-947/liveobjects-plugin-base
branch
from
fbf46a7 to
d9d6408
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1880/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1880/features
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (10)
test/realtime/live_objects.test.js (4)
1-11: Remove redundant 'use strict' directive and approve importsThe 'use strict' directive on line 1 is redundant in modules, as they are automatically in strict mode. Consider removing it.
The imports look good and include all necessary dependencies for testing the LiveObjects plugin.
Apply this diff to remove the redundant 'use strict' directive:
-'use strict'; define(['ably', 'shared_helper', 'async', 'chai', 'live_objects'], function ( Ably, Helper, async, chai, LiveObjectsPlugin, ) { var expect = chai.expect; var createPM = Ably.protocolMessageFromDeserialized;🧰 Tools
🪛 Biome
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
17-23: Approve monitorConnectionThenCloseAndFinish helper with minor suggestionThe
monitorConnectionThenCloseAndFinishhelper function is well-implemented. It ensures proper monitoring of connection states and cleanup of resources, which is crucial for maintaining test isolation.However, consider adding error logging before the cleanup in the
finallyblock. This would help in debugging if an error occurs during the connection monitoring.Consider adding error logging:
async function monitorConnectionThenCloseAndFinish(helper, action, realtime, states) { try { await helper.monitorConnectionAsync(action, realtime, states); + } catch (error) { + console.error('Error during connection monitoring:', error); + throw error; } finally { await helper.closeAndFinishAsync(realtime); } }
40-48: Approve "Realtime without LiveObjects plugin" test suite with minor suggestionThe test suite correctly verifies the behavior when the LiveObjects plugin is not present. It ensures that an appropriate error is thrown when attempting to access the
liveObjectsproperty.For consistency with the other test suite, consider using an async function for the test case.
Consider updating the test case to use an async function:
describe('Realtime without LiveObjects plugin', () => { /** @nospec */ - it("throws an error when attempting to access the channel's `liveObjects` property", async function () { + it("throws an error when attempting to access the channel's `liveObjects` property", async function () { const helper = this.test.helper; const client = helper.AblyRealtime({ autoConnect: false }); const channel = client.channels.get('channel'); expect(() => channel.liveObjects).to.throw('LiveObjects plugin not provided'); }); });
50-58: Approve "Realtime with LiveObjects plugin" test suite with suggestion for improvementThe test suite correctly verifies the behavior when the LiveObjects plugin is present. It ensures that the
liveObjectsproperty returns an instance of the LiveObjects class.To make the test more robust, consider adding additional assertions to verify the structure and methods of the returned LiveObjects instance. This would provide more confidence in the correct implementation of the plugin.
Consider expanding the test case to include more detailed assertions:
describe('Realtime with LiveObjects plugin', () => { /** @nospec */ it("returns LiveObjects instance when accessing channel's `liveObjects` property", async function () { const helper = this.test.helper; const client = LiveObjectsRealtime(helper, { autoConnect: false }); const channel = client.channels.get('channel'); expect(channel.liveObjects.constructor.name).to.equal('LiveObjects'); + expect(channel.liveObjects).to.have.property('create'); + expect(channel.liveObjects).to.have.property('get'); + expect(channel.liveObjects.create).to.be.a('function'); + expect(channel.liveObjects.get).to.be.a('function'); }); });scripts/moduleReport.ts (1)
206-208: LGTM: NewcalculateLiveObjectsPluginSizefunctionThe new
calculateLiveObjectsPluginSizefunction is correctly implemented, following the same pattern ascalculatePushPluginSize. It properly utilizes the genericcalculatePluginSizefunction with appropriate parameters for the LiveObjects plugin.Consider adding a brief comment above this function to explain its purpose, similar to other functions in this file.
README.md (4)
589-591: Consider rephrasing for concisenessThe opening sentence could be more concise. Consider revising it to:
-Live Objects functionality is supported for Realtime clients via the LiveObjects plugin. In order to use Live Objects, you must pass in the plugin via client options. +Live Objects functionality is supported for Realtime clients via the LiveObjects plugin. To use Live Objects, pass the plugin via client options.This change maintains the meaning while making the text more direct and concise.
🧰 Tools
🪛 LanguageTool
[style] ~591-~591: Consider a shorter alternative to avoid wordiness.
Context: ...ime clients via the LiveObjects plugin. In order to use Live Objects, you must pass in the ...(IN_ORDER_TO_PREMIUM)
605-609: Consider using fenced code blocks for consistencyFor consistency with other code blocks in the document, consider using fenced code blocks (```) instead of indented code blocks. This also aligns with the Markdownlint suggestion.
-```html -<script src="https://cdn.ably.com/lib/liveobjects.umd.min-2.js"></script> -``` +```html +<script src="https://cdn.ably.com/lib/liveobjects.umd.min-2.js"></script> +```🧰 Tools
🪛 Markdownlint
607-607: Expected: indented; Actual: fenced
Code block style(MD046, code-block-style)
613-618: Consider using fenced code blocks for consistencySimilar to the previous comment, consider using fenced code blocks here as well:
-```javascript -const client = new Ably.Realtime({ - ...options, - plugins: { LiveObjects: AblyLiveObjectsPlugin }, -}); -``` +```javascript +const client = new Ably.Realtime({ + ...options, + plugins: { LiveObjects: AblyLiveObjectsPlugin }, +}); +```🧰 Tools
🪛 Markdownlint
613-613: Expected: indented; Actual: fenced
Code block style(MD046, code-block-style)
620-620: Consider formatting improvementsThis line could benefit from a few formatting improvements:
- Add a missing comma after "js".
- Format the bare URLs as inline links for better readability.
Here's a suggested revision:
-The LiveObjects plugin is developed as part of the Ably client library, so it is available for the same versions as the Ably client library itself. It also means that it follows the same semantic versioning rules as they were defined for [the Ably client library](#for-browsers). For example, to lock into a major or minor version of the LiveObjects plugin, you can specify a specific version number such as https://cdn.ably.com/lib/liveobjects.umd.min-2.js for all v2._ versions, or https://cdn.ably.com/lib/liveobjects.umd.min-2.4.js for all v2.4._ versions, or you can lock into a single release with https://cdn.ably.com/lib/liveobjects.umd.min-2.4.0.js. Note you can load the non-minified version by omitting `.min` from the URL such as https://cdn.ably.com/lib/liveobjects.umd-2.js. +The LiveObjects plugin is developed as part of the Ably client library, so it is available for the same versions as the Ably client library itself. It also means that it follows the same semantic versioning rules as they were defined for [the Ably client library](#for-browsers). For example, to lock into a major or minor version of the LiveObjects plugin, you can specify a specific version number such as [https://cdn.ably.com/lib/liveobjects.umd.min-2.js](https://cdn.ably.com/lib/liveobjects.umd.min-2.js) for all v2._ versions, or [https://cdn.ably.com/lib/liveobjects.umd.min-2.4.js](https://cdn.ably.com/lib/liveobjects.umd.min-2.4.js) for all v2.4._ versions, or you can lock into a single release with [https://cdn.ably.com/lib/liveobjects.umd.min-2.4.0.js](https://cdn.ably.com/lib/liveobjects.umd.min-2.4.0.js). Note, you can load the non-minified version by omitting `.min` from the URL such as [https://cdn.ably.com/lib/liveobjects.umd-2.js](https://cdn.ably.com/lib/liveobjects.umd-2.js).🧰 Tools
🪛 LanguageTool
[uncategorized] ~620-~620: Possible missing comma found.
Context: ...y.com/lib/liveobjects.umd.min-2.4.0.js. Note you can load the non-minified version b...(AI_HYDRA_LEO_MISSING_COMMA)
🪛 Markdownlint
620-620: null
Bare URL used(MD034, no-bare-urls)
620-620: null
Bare URL used(MD034, no-bare-urls)
620-620: null
Bare URL used(MD034, no-bare-urls)
620-620: null
Bare URL used(MD034, no-bare-urls)
620-620: null
Spaces inside emphasis markers(MD037, no-space-in-emphasis)
ably.d.ts (1)
626-630: Approved, but consider adding more details to the LiveObjects interface.The addition of the
LiveObjectsinterface is a good start for exposing LiveObjects as a plugin. However, the interface is currently empty. Consider adding methods or properties that will be part of the LiveObjects functionality to make the interface more meaningful and useful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
- Gruntfile.js (2 hunks)
- README.md (1 hunks)
- ably.d.ts (3 hunks)
- grunt/esbuild/build.js (2 hunks)
- liveobjects.d.ts (1 hunks)
- package.json (3 hunks)
- scripts/cdn_deploy.js (1 hunks)
- scripts/moduleReport.ts (5 hunks)
- src/common/lib/client/modularplugins.ts (2 hunks)
- src/common/lib/client/realtimechannel.ts (4 hunks)
- src/plugins/index.d.ts (1 hunks)
- src/plugins/liveobjects/index.ts (1 hunks)
- src/plugins/liveobjects/liveobjects.ts (1 hunks)
- test/common/globals/named_dependencies.js (1 hunks)
- test/realtime/live_objects.test.js (1 hunks)
- test/support/browser_file_list.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- Gruntfile.js
- liveobjects.d.ts
- package.json
- scripts/cdn_deploy.js
- src/common/lib/client/modularplugins.ts
- src/common/lib/client/realtimechannel.ts
- src/plugins/index.d.ts
- src/plugins/liveobjects/index.ts
- src/plugins/liveobjects/liveobjects.ts
- test/common/globals/named_dependencies.js
- test/support/browser_file_list.js
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~591-~591: Consider a shorter alternative to avoid wordiness.
Context: ...ime clients via the LiveObjects plugin. In order to use Live Objects, you must pass in the ...(IN_ORDER_TO_PREMIUM)
[uncategorized] ~620-~620: Possible missing comma found.
Context: ...y.com/lib/liveobjects.umd.min-2.4.0.js. Note you can load the non-minified version b...(AI_HYDRA_LEO_MISSING_COMMA)
🪛 Markdownlint
README.md
620-620: null
Bare URL used(MD034, no-bare-urls)
620-620: null
Bare URL used(MD034, no-bare-urls)
620-620: null
Bare URL used(MD034, no-bare-urls)
620-620: null
Bare URL used(MD034, no-bare-urls)
620-620: null
Spaces inside emphasis markers(MD037, no-space-in-emphasis)
593-593: Expected: indented; Actual: fenced
Code block style(MD046, code-block-style)
607-607: Expected: indented; Actual: fenced
Code block style(MD046, code-block-style)
613-613: Expected: indented; Actual: fenced
Code block style(MD046, code-block-style)
🪛 Biome
test/realtime/live_objects.test.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
🔇 Additional comments (15)
test/realtime/live_objects.test.js (2)
13-15: LGTM: LiveObjectsRealtime helper functionThe
LiveObjectsRealtimehelper function is well-implemented. It correctly creates a new Ably Realtime instance with the LiveObjects plugin included in the options. The use of the spread operator ensures that any additional options passed to the function are preserved.
1-60: Overall assessment: Well-structured and comprehensive test suiteThe test file for the LiveObjects plugin is well-structured and provides good coverage of the basic functionality. It effectively tests scenarios both with and without the plugin, ensuring proper error handling and instance creation.
Suggestions for future improvements:
- Add more detailed tests for the LiveObjects instance methods (e.g.,
create,get).- Consider adding edge cases and error scenarios to increase test coverage.
- If applicable, add tests for any specific features or behaviors of the LiveObjects plugin that are crucial to its functionality.
Great job on implementing these tests! They will help ensure the reliability of the LiveObjects plugin integration.
🧰 Tools
🪛 Biome
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
grunt/esbuild/build.js (5)
80-85: LGTM: LiveObjects plugin configuration looks goodThe
liveObjectsPluginConfigis correctly structured and follows the established pattern for plugin configurations in this file. It properly sets up the build process for the LiveObjects plugin, including the correct entry point, library name, and output file.
87-92: LGTM: LiveObjects CDN plugin configuration is correctThe
liveObjectsPluginCdnConfigis properly set up for CDN use, following the same pattern as other CDN plugin configurations. The output file is correctly set to use the 'umd' suffix, which is consistent with the CDN naming convention used in this project.
94-100: LGTM: Minified LiveObjects CDN plugin configuration is properly set upThe
minifiedLiveObjectsPluginCdnConfigcorrectly sets up a minified build for CDN use. It follows the established pattern for minified CDN configurations, including the proper output filename with the 'min' suffix and theminifyoption set totrue.
110-112: LGTM: Module exports correctly updated with new LiveObjects configurationsThe module exports have been properly updated to include the new LiveObjects plugin configurations. This ensures that the new build configurations are accessible for use in the build process.
Line range hint
80-112: Summary: LiveObjects plugin build configurations successfully implementedThe changes to this file successfully introduce the necessary build configurations for the new LiveObjects plugin. The implementations for
liveObjectsPluginConfig,liveObjectsPluginCdnConfig, andminifiedLiveObjectsPluginCdnConfigare consistent with the existing patterns in the file and provide the required build options for standard, CDN, and minified CDN versions of the plugin.The module exports have been correctly updated to include these new configurations, ensuring they are accessible for the build process. Overall, these changes are well-integrated and maintain the structure and consistency of the build configuration file.
scripts/moduleReport.ts (3)
9-9: Please clarify the reason for increasing the raw threshold.The
rawthreshold forminimalUsefulRealtimeBundleSizeThresholdsKiBhas been increased from 98 to 99 KiB. Could you provide context for this change? It's important to understand the reasoning behind adjusting bundle size thresholds.
186-195: Great job on refactoringcalculatePluginSize!The new
calculatePluginSizefunction is a well-designed, generic solution that can be reused for different plugins. It improves code maintainability and reduces duplication. The use of an options object withpathanddescriptionprovides good flexibility.
202-204: LGTM: Proper use of the newcalculatePluginSizefunctionThe
calculatePushPluginSizefunction has been correctly updated to use the new genericcalculatePluginSizefunction. This change maintains the same functionality while reducing code duplication.README.md (3)
593-601: LGTM: Clear and concise code exampleThe code example for using the LiveObjects plugin with npm is clear and follows the style of previous examples in the document.
🧰 Tools
🪛 Markdownlint
593-593: Expected: indented; Actual: fenced
Code block style(MD046, code-block-style)
603-604: LGTM: Informative note about Modular variant compatibilityThis note provides valuable information about compatibility with the Modular variant of the library.
622-623: LGTM: Clear link to documentationThe link to the Ably Live Objects documentation is clear and provides a good resource for users to find more information.
ably.d.ts (2)
2152-2155: LGTM! LiveObjects property successfully added to RealtimeChannel.The addition of the
liveObjectsproperty of typeLiveObjectsto theRealtimeChannelinterface is correct and aligns with the PR objectives. This will allow users to access LiveObjects functionality through the channel object.
626-630: LGTM! LiveObjects plugin successfully added to CorePlugins.The addition of the
LiveObjectsproperty to theCorePluginsinterface is correct and necessary for LiveObjects to be recognized as a plugin. The use of theunknowntype is appropriate here, allowing for flexibility in the plugin's implementation.
There was a problem hiding this comment.
Looks like we're using a mix of live_objects and liveobjects, I think we should pick one and stick to it (liveobjects would be my preference)
There was a problem hiding this comment.
I've used two different conventions because ably-js currrently has mixed styles.
Tests use snake case, hence using live_objects as a file name and as a global dependency name below (see modules like shared_helper and private_api_recorder, and lots of files in test/support folder using snake case).
Source code files then use what I think is called flat case? Like your connectionmanager.
So for now I kept the consistency with the codebase when naming live objects stuff. Ideally we would address naming conventions in ably-js separately.
| @@ -11,6 +11,10 @@ define(function () { | |||
| browser: 'build/push', | |||
| node: 'build/push', | |||
| }, | |||
| live_objects: { | |||
| const liveObjectsPluginConfig = { | ||
| ...createBaseConfig(), | ||
| entryPoints: ['src/plugins/liveobjects/index.ts'], | ||
| plugins: [umdWrapper.default({ libraryName: 'AblyLiveObjectsPlugin', amdNamedModule: false })], |
There was a problem hiding this comment.
this is confusing to me, we have an artefact called liveobjects.js and another called liveobjects.umd.js but they're exactly the same. i assume this one was meant to be esm
| plugins: [umdWrapper.default({ libraryName: 'AblyLiveObjectsPlugin', amdNamedModule: false })], |
There was a problem hiding this comment.
the second liveObjectsPluginCdnConfig (and third minifiedLiveObjectsPluginCdnConfig) configs are for CDN LiveObjects bundles.
the implementation is based on our CDN for Push plugin #1861.
Push plugin used *.umd.js naming convention for its CDN plugin, so I've used the same here for LiveObjects CDN plugin.
you're right that cdn and regular configs for LiveObjects are the same right now - as LiveObjects plugin doesn't have any external dependencies at this moment, but 1. that can easily change in the future, 2. we wouldn't want to change a name for a CDN script for LiveObjects in the future
test/realtime/live_objects.test.js
Outdated
| return helper.AblyRealtime({ ...options, plugins: { LiveObjects: LiveObjectsPlugin } }); | ||
| } | ||
|
|
||
| async function monitorConnectionThenCloseAndFinish(helper, action, realtime, states) { |
There was a problem hiding this comment.
this function is unused. also monitorConnection already closes the connection if the client transitions to a target state so seems redundant anyway?
There was a problem hiding this comment.
I used it in the tests in the initial commit, but then I realized those two tests don't need to open a connection so I added autoConnect: false and removed a call to monitorConnectionThenCloseAndFinish.
However, monitorConnectionThenCloseAndFinish is going to be used extensively in other integration tests that do actually interact with a sandbox server (it's already used in #1882).
also monitorConnection already closes the connection if the client transitions to a target state so seems redundant anyway?
monitorConnection without states provided is listening for failed and suspended states, and basically designed to fast fail a test in those cases, as usually you don't want to deal with those states in a test.
closeAndFinish will close the connection no matter what.
combination of those two is the usual practice in the tests, and the monitorConnectionThenCloseAndFinish is just a convenience method for async tests, see how it's used in modular.test.js
There was a problem hiding this comment.
ok that's fair, but it's a generic helper so ought to be part of the shared helpers. also seems unnecessary taking helper as an argument rather than being a method on the helper class, like helper.monitorConnectionThen...
There was a problem hiding this comment.
true, now that this function is being used in multiple tests it makes sense to move it to the helper. will do that
There was a problem hiding this comment.
moved monitorConnectionThenCloseAndFinish to the helper in a separate commit: fac8a64
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (10)
test/realtime/live_objects.test.js (4)
1-1: Remove redundant 'use strict' directiveThe 'use strict' directive is unnecessary in ES modules as they are automatically in strict mode.
Apply this diff to remove the redundant directive:
-'use strict';🧰 Tools
🪛 Biome
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
3-11: Use consistent naming convention for importsThe import for 'live_objects' uses snake_case, which is inconsistent with the camelCase used for other imports.
Consider renaming 'live_objects' to 'liveObjects' for consistency:
-define(['ably', 'shared_helper', 'async', 'chai', 'live_objects'], function ( +define(['ably', 'shared_helper', 'async', 'chai', 'liveObjects'], function ( Ably, Helper, async, chai, - LiveObjectsPlugin, + LiveObjectsPlugin, ) {
17-30: Consider reducing the timeout if possibleThe test suite setup is well-structured and includes proper error handling in the
beforehook. However, the 60-second timeout might be excessive for unit tests.If these are primarily unit tests, consider reducing the timeout. If integration tests require this duration, you may want to add a comment explaining why such a long timeout is necessary.
32-50: Approve test cases and suggest improvementsThe test cases effectively cover the basic scenarios of accessing the
liveObjectsproperty with and without the plugin. The use ofautoConnect: falseis a good practice for preventing unnecessary connections during tests.Consider adding more comprehensive test cases to cover edge cases and additional functionality of the LiveObjects plugin. For example:
- Test behavior when switching between channels
- Verify error handling for invalid operations
- Check performance with multiple LiveObjects instances
Also, could you clarify the purpose of the
@nospeccomments? Are these tests meant to be excluded from certain test runs?test/common/modules/shared_helper.js (1)
174-180: LGTM! Consider adding error handling.The new
monitorConnectionThenCloseAndFinishmethod is a well-structured addition that combines existing functionality to provide a more robust way of handling connection monitoring and cleanup in tests. The use oftry-finallyensures that resources are properly cleaned up, even in the case of errors.Consider adding error handling to log or report any errors that occur during the
monitorConnectionAsynccall. This could be done by catching the error in thetryblock and logging it before allowing it to propagate:async monitorConnectionThenCloseAndFinish(action, realtime, states) { try { await this.monitorConnectionAsync(action, realtime, states); + } catch (error) { + console.error('Error during connection monitoring:', error); + throw error; } finally { await this.closeAndFinishAsync(realtime); } }This would provide more visibility into any issues that occur during testing.
test/browser/modular.test.js (5)
454-491: Well-implemented test for encrypted message publishingThis test case effectively verifies the encryption and decryption process for published messages. The use of separate clients for receiving and transmitting is a good practice. The code structure is clear and uses async/await appropriately.
Consider adding a comment explaining the purpose of the nested
monitorConnectionThenCloseAndFinishcalls for improved readability.
628-632: Effective test for RealtimePresence plugin absenceThis test case correctly verifies that an error is thrown when attempting to access the
presenceproperty without the RealtimePresence plugin. The code structure is clear and concise.Consider standardizing the error message format across the library for consistency. For example, you might want to use "RealtimePresence plugin not provided" instead of "RealtimePresence plugin not provided" to match the format used in other error messages (e.g., "Rest plugin not provided").
642-670: Comprehensive test for handling PRESENCE ProtocolMessage without RealtimePresence pluginThis test case effectively verifies that a client without the RealtimePresence plugin can still function normally when receiving PRESENCE ProtocolMessages. The code structure, while complex, is necessary for this scenario.
To improve clarity, consider adding a comment explaining the purpose of waiting for the
receivedMessagePromiseto resolve. This would help readers understand why this step is crucial for the test.
694-720: Effective test for RealtimePresence functionalityThis test case successfully verifies the basic functionality of the RealtimePresence plugin. The code structure is consistent with previous test cases, which is good for maintainability.
To improve robustness, consider adding a timeout to the
rxPresenceMessagePromise. This would prevent the test from hanging indefinitely if the presence message is not received for some reason.
814-829: Well-implemented test for transport selectionThis test case effectively verifies the ability to use a specific transport (WebSocket or XHR polling). The approach of overriding the
tryATransportmethod to capture the first transport candidate is clever and non-intrusive.To improve clarity, consider adding a comment explaining the purpose of overriding
tryATransport. This would help readers understand the test's methodology more quickly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- test/browser/modular.test.js (11 hunks)
- test/common/modules/shared_helper.js (1 hunks)
- test/realtime/live_objects.test.js (1 hunks)
🧰 Additional context used
🪛 Biome
test/realtime/live_objects.test.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
🔇 Additional comments (5)
test/realtime/live_objects.test.js (1)
13-15: LGTM: Helper function is well-implementedThe
LiveObjectsRealtimehelper function is correctly implemented. It effectively creates a new Realtime instance with the LiveObjects plugin, using the spread operator to merge options. The function name is clear and descriptive.test/common/modules/shared_helper.js (1)
Line range hint
1-180: Good addition to enhance test robustnessThe introduction of the
monitorConnectionThenCloseAndFinishmethod is a positive addition to theSharedHelperclass. It combines existing functionality in a way that ensures proper resource cleanup, even in error scenarios. This enhancement will likely improve the reliability and consistency of tests involving connection monitoring.The new method aligns well with the existing code structure and naming conventions. It provides a higher-level abstraction that should make writing certain types of tests easier and less error-prone.
test/browser/modular.test.js (3)
197-211: Well-structured test case for publishing and subscribingThis test case is well-implemented, using async/await for better readability and proper connection management with
helper.monitorConnectionThenCloseAndFinish. It effectively tests both publishing and subscribing functionality without the Rest plugin.
572-574: Concise test for JSON format usageThis test case effectively verifies that the Realtime client uses JSON format when MsgPack is not provided. The use of
helper.monitorConnectionThenCloseAndFinishensures proper connection management.
612-614: Consistent test for MessagePack format usageThis test case effectively verifies that the Realtime client uses MessagePack format when the MsgPack plugin is provided. The structure is consistent with the previous test for JSON format, which is good for maintainability and readability.
Base code, tests and build setup for new LiveObjects plugin. Adds a new
.liveObjectsproperty for RealtimeChannel.Plugin setup is based on Web Push plugin PR, and CDN setup for Push plugin PR.
Resolves DTP-947
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests
Chores