-
Notifications
You must be signed in to change notification settings - Fork 59
LiveObjects integration branch fixes #2017
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis set of changes updates the global TypeScript interface for LiveObjects typings from Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Test as Test Code
participant Channel as Channel
participant Transport as Transport
User->>Test: Initiate test for channelSerial on ATTACH
Test->>Channel: Publish message
Channel->>Transport: Send MESSAGE ProtocolMessage
Transport-->>Channel: Receive MESSAGE, capture channelSerial
Test->>Transport: Disconnect transport
Transport-->>Test: Triggers reconnect
Test->>Transport: Spy on send (for ATTACH)
Channel->>Transport: Send ATTACH ProtocolMessage (with channelSerial)
Test->>Transport: Assert ATTACH includes correct channelSerial
Possibly related PRs
Suggested reviewers
Poem
🪧 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 (
|
8b44d6d
to
6a27d76
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/common/lib/util/utils.ts (1)
282-290
: Comprehensive JSDoc addition
The new comment clearly documents each branch ofdataSizeBytes
(TM6a, TM6c, OD3c, OD3d) and the expected input types. Consider adding a@throws
tag to document the error thrown when an unsupported type is passed.
🛑 Comments failed to post (3)
test/package/browser/template/src/index-default.ts (1)
6-8: 🛠️ Refactor suggestion
Replace deprecated
module
withnamespace
The Biome lint rule flagsdeclare module globalThis {}
as outdated. Usedeclare namespace globalThis {}
to augment the global object:-declare module globalThis { +declare namespace globalThis {📝 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.declare namespace globalThis { var testAblyPackage: () => Promise<void>; }
🧰 Tools
🪛 Biome (1.9.4)
[error] 6-6: Use the namespace keyword instead of the outdated module keyword.
The module keyword is deprecated to avoid any confusion with the ECMAScript modules which are often called modules.
Safe fix: Use namespace instead.(lint/suspicious/useNamespaceKeyword)
🤖 Prompt for AI Agents (early access)
In test/package/browser/template/src/index-default.ts around lines 6 to 8, replace the deprecated `declare module globalThis` with `declare namespace globalThis` to properly augment the global object and comply with the Biome lint rule.
test/package/browser/template/src/index-modular.ts (1)
7-9: 🛠️ Refactor suggestion
Use
namespace
for global augmentation
To avoid confusion with ECMAScript modules, replacedeclare module globalThis {}
with:-declare module globalThis { +declare namespace globalThis {📝 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.declare namespace globalThis { var testAblyPackage: () => Promise<void>; }
🧰 Tools
🪛 Biome (1.9.4)
[error] 7-7: Use the namespace keyword instead of the outdated module keyword.
The module keyword is deprecated to avoid any confusion with the ECMAScript modules which are often called modules.
Safe fix: Use namespace instead.(lint/suspicious/useNamespaceKeyword)
🤖 Prompt for AI Agents (early access)
In test/package/browser/template/src/index-modular.ts around lines 7 to 9, replace the use of `declare module globalThis {}` with `declare namespace globalThis {}` to properly augment the global scope and avoid confusion with ECMAScript modules. Change the declaration to use `namespace` for global augmentation.
test/package/browser/template/src/index-objects.ts (1)
5-9: 🛠️ Refactor suggestion
Use
namespace
instead ofmodule
for TypeScript declarations.The
declare module
syntax is deprecated in TypeScript. Usenamespace
instead to avoid confusion with ECMAScript modules.// Fix for "type 'typeof globalThis' has no index signature" error: // https://stackoverflow.com/questions/68481686/type-typeof-globalthis-has-no-index-signature -declare module globalThis { +declare namespace globalThis { var testAblyPackage: () => Promise<void>; }📝 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.// Fix for "type 'typeof globalThis' has no index signature" error: // https://stackoverflow.com/questions/68481686/type-typeof-globalthis-has-no-index-signature declare namespace globalThis { var testAblyPackage: () => Promise<void>; }
🧰 Tools
🪛 Biome (1.9.4)
[error] 7-7: Use the namespace keyword instead of the outdated module keyword.
The module keyword is deprecated to avoid any confusion with the ECMAScript modules which are often called modules.
Safe fix: Use namespace instead.(lint/suspicious/useNamespaceKeyword)
🤖 Prompt for AI Agents (early access)
In test/package/browser/template/src/index-objects.ts around lines 5 to 9, replace the deprecated `declare module globalThis` syntax with `declare namespace globalThis` to correctly declare the global augmentation and avoid confusion with ECMAScript modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/package/browser/template/src/index-default.ts (1)
4-8
: Usenamespace
instead ofmodule
for TypeScript declaration mergingThe declaration to fix the TypeScript error correctly adds the global variable to
globalThis
, but modern TypeScript best practices recommend using thenamespace
keyword rather than the outdatedmodule
keyword.-declare module globalThis { +declare namespace globalThis { var testAblyPackage: () => Promise<void>; }🧰 Tools
🪛 Biome (1.9.4)
[error] 6-6: Use the namespace keyword instead of the outdated module keyword.
The module keyword is deprecated to avoid any confusion with the ECMAScript modules which are often called modules.
Safe fix: Use namespace instead.(lint/suspicious/useNamespaceKeyword)
test/package/browser/template/src/index-modular.ts (1)
5-9
: Usenamespace
instead ofmodule
for TypeScript declaration mergingThe declaration to fix the TypeScript error correctly adds the global variable to
globalThis
, but modern TypeScript best practices recommend using thenamespace
keyword rather than the outdatedmodule
keyword.-declare module globalThis { +declare namespace globalThis { var testAblyPackage: () => Promise<void>; }🧰 Tools
🪛 Biome (1.9.4)
[error] 7-7: Use the namespace keyword instead of the outdated module keyword.
The module keyword is deprecated to avoid any confusion with the ECMAScript modules which are often called modules.
Safe fix: Use namespace instead.(lint/suspicious/useNamespaceKeyword)
test/package/browser/template/src/index-objects.ts (1)
5-9
: Usenamespace
instead ofmodule
for TypeScript declaration mergingThe declaration to fix the TypeScript error correctly adds the global variable to
globalThis
, but modern TypeScript best practices recommend using thenamespace
keyword rather than the outdatedmodule
keyword.-declare module globalThis { +declare namespace globalThis { var testAblyPackage: () => Promise<void>; }🧰 Tools
🪛 Biome (1.9.4)
[error] 7-7: Use the namespace keyword instead of the outdated module keyword.
The module keyword is deprecated to avoid any confusion with the ECMAScript modules which are often called modules.
Safe fix: Use namespace instead.(lint/suspicious/useNamespaceKeyword)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
README.md
(2 hunks)ably.d.ts
(6 hunks)src/common/lib/util/utils.ts
(1 hunks)test/common/modules/private_api_recorder.js
(1 hunks)test/package/browser/template/src/ably.config.d.ts
(0 hunks)test/package/browser/template/src/index-default.ts
(1 hunks)test/package/browser/template/src/index-modular.ts
(1 hunks)test/package/browser/template/src/index-objects.ts
(4 hunks)test/package/browser/template/src/tsconfig.json
(1 hunks)test/realtime/channel.test.js
(4 hunks)typedoc.json
(1 hunks)
💤 Files with no reviewable changes (1)
- test/package/browser/template/src/ably.config.d.ts
✅ Files skipped from review due to trivial changes (2)
- typedoc.json
- test/common/modules/private_api_recorder.js
🚧 Files skipped from review as they are similar to previous changes (4)
- test/package/browser/template/src/tsconfig.json
- src/common/lib/util/utils.ts
- test/realtime/channel.test.js
- ably.d.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/package/browser/template/src/index-objects.ts (4)
src/plugins/objects/livemap.ts (2)
LiveMap
(54-843)size
(289-303)ably.d.ts (1)
AblyObjectsTypes
(2370-2372)test/realtime/objects.test.js (2)
root
(148-148)objects
(144-144)src/common/lib/client/realtimechannel.ts (1)
objects
(157-162)
🪛 markdownlint-cli2 (0.17.2)
README.md
891-891: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
🪛 Biome (1.9.4)
test/package/browser/template/src/index-default.ts
[error] 6-6: Use the namespace keyword instead of the outdated module keyword.
The module keyword is deprecated to avoid any confusion with the ECMAScript modules which are often called modules.
Safe fix: Use namespace instead.
(lint/suspicious/useNamespaceKeyword)
test/package/browser/template/src/index-modular.ts
[error] 7-7: Use the namespace keyword instead of the outdated module keyword.
The module keyword is deprecated to avoid any confusion with the ECMAScript modules which are often called modules.
Safe fix: Use namespace instead.
(lint/suspicious/useNamespaceKeyword)
test/package/browser/template/src/index-objects.ts
[error] 7-7: Use the namespace keyword instead of the outdated module keyword.
The module keyword is deprecated to avoid any confusion with the ECMAScript modules which are often called modules.
Safe fix: Use namespace instead.
(lint/suspicious/useNamespaceKeyword)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-node (20.x)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (16.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (chromium)
🔇 Additional comments (2)
README.md (1)
869-870
: LGTM: Interface renaming is consistent with type declaration changesThe renamed interface from
ObjectsTypes
toAblyObjectsTypes
is consistently applied throughout the documentation, keeping it in sync with the actual implementation in the type declarations.Also applies to: 883-883, 892-892
test/package/browser/template/src/index-objects.ts (1)
11-29
: Good job renaming the interface and moving type definitions inlineMoving the
CustomRoot
type definition inline and renaming the global interface fromObjectsTypes
toAblyObjectsTypes
improves the organization of the code. All references to the interface have been correctly updated throughout the file, ensuring type safety and consistency.Also applies to: 44-44, 51-51, 61-61, 92-92
…ge tests" This reverts commit d59e167. The `moduleResolution` option should be `bundler`, just like it was set in the original fecf2df commit. The supposed TypeScript error "Option '--resolveJsonModule' cannot be specified without 'node' module resolution strategy." is actually coming from the root folder TypeScript installation, not the one used for the `package` test folder. The `--moduleResolution bundler` option was added in TypeScript 5.0 [1] and in the root we have TypeScript ^4.9 installed, compared to TypeScript ^5.2 used for `package`` folder. See PR review comment for more context [2]. [1] https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-0.html#--moduleresolution-bundler [2] #2007 (comment)
020efdc
to
5106329
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/package/browser/template/src/index-modular.ts (1)
5-9
: Usenamespace
instead ofmodule
for TypeScript declarations.The TypeScript declaration correctly adds a type for the global
testAblyPackage
function, fixing the "no index signature" error. However, themodule
keyword is deprecated in TypeScript declarations.-declare module globalThis { +declare namespace globalThis { var testAblyPackage: () => Promise<void>; }🧰 Tools
🪛 Biome (1.9.4)
[error] 7-7: Use the namespace keyword instead of the outdated module keyword.
The module keyword is deprecated to avoid any confusion with the ECMAScript modules which are often called modules.
Safe fix: Use namespace instead.(lint/suspicious/useNamespaceKeyword)
test/package/browser/template/src/index-default.ts (1)
4-8
: Usenamespace
instead ofmodule
for TypeScript declarations.The TypeScript declaration correctly adds a type for the global
testAblyPackage
function, fixing the "no index signature" error. However, themodule
keyword is deprecated in TypeScript declarations.-declare module globalThis { +declare namespace globalThis { var testAblyPackage: () => Promise<void>; }🧰 Tools
🪛 Biome (1.9.4)
[error] 6-6: Use the namespace keyword instead of the outdated module keyword.
The module keyword is deprecated to avoid any confusion with the ECMAScript modules which are often called modules.
Safe fix: Use namespace instead.(lint/suspicious/useNamespaceKeyword)
test/package/browser/template/src/index-objects.ts (1)
5-9
: Usenamespace
instead ofmodule
for TypeScript declarations.The TypeScript declaration correctly adds a type for the global
testAblyPackage
function, fixing the "no index signature" error. However, themodule
keyword is deprecated in TypeScript declarations.-declare module globalThis { +declare namespace globalThis { var testAblyPackage: () => Promise<void>; }🧰 Tools
🪛 Biome (1.9.4)
[error] 7-7: Use the namespace keyword instead of the outdated module keyword.
The module keyword is deprecated to avoid any confusion with the ECMAScript modules which are often called modules.
Safe fix: Use namespace instead.(lint/suspicious/useNamespaceKeyword)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (11)
README.md
(2 hunks)ably.d.ts
(6 hunks)src/common/lib/util/utils.ts
(1 hunks)test/common/modules/private_api_recorder.js
(1 hunks)test/package/browser/template/src/ably.config.d.ts
(0 hunks)test/package/browser/template/src/index-default.ts
(1 hunks)test/package/browser/template/src/index-modular.ts
(1 hunks)test/package/browser/template/src/index-objects.ts
(4 hunks)test/package/browser/template/src/tsconfig.json
(1 hunks)test/realtime/channel.test.js
(4 hunks)typedoc.json
(1 hunks)
💤 Files with no reviewable changes (1)
- test/package/browser/template/src/ably.config.d.ts
✅ Files skipped from review due to trivial changes (1)
- ably.d.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- typedoc.json
- test/package/browser/template/src/tsconfig.json
- test/common/modules/private_api_recorder.js
- src/common/lib/util/utils.ts
- test/realtime/channel.test.js
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
891-891: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
🪛 Biome (1.9.4)
test/package/browser/template/src/index-default.ts
[error] 6-6: Use the namespace keyword instead of the outdated module keyword.
The module keyword is deprecated to avoid any confusion with the ECMAScript modules which are often called modules.
Safe fix: Use namespace instead.
(lint/suspicious/useNamespaceKeyword)
test/package/browser/template/src/index-modular.ts
[error] 7-7: Use the namespace keyword instead of the outdated module keyword.
The module keyword is deprecated to avoid any confusion with the ECMAScript modules which are often called modules.
Safe fix: Use namespace instead.
(lint/suspicious/useNamespaceKeyword)
test/package/browser/template/src/index-objects.ts
[error] 7-7: Use the namespace keyword instead of the outdated module keyword.
The module keyword is deprecated to avoid any confusion with the ECMAScript modules which are often called modules.
Safe fix: Use namespace instead.
(lint/suspicious/useNamespaceKeyword)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test-node (20.x)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-npm-package
- GitHub Check: test-node (16.x)
🔇 Additional comments (3)
README.md (1)
869-870
: Interface renaming in documentation looks good.The documentation has been properly updated to refer to the renamed interface
AblyObjectsTypes
instead ofObjectsTypes
, maintaining consistency with the code changes.Also applies to: 883-884, 892-893
test/package/browser/template/src/index-objects.ts (2)
11-29
: Inline type definition looks good.The inline definition of
CustomRoot
type and the globalAblyObjectsTypes
interface properly replaces the previous approach of importing from an external file. This change aligns with the interface renaming across the codebase.
44-45
: Type references and comments are consistently updated.All references to the previous interface name have been properly updated to use
AblyObjectsTypes
in both comments and code. This maintains consistency with the rest of the codebase.Also applies to: 51-52, 61-61, 92-93
Minor fixes/changes based on the review from #2007.
Changes can be easily reviewed on a per-commit basis.
Summary by CodeRabbit
Documentation
ObjectsTypes
toAblyObjectsTypes
and clarified the scope of TypeScript typings.Tests
AblyObjectsTypes
.Chores