Conversation
WalkthroughMoves the LiveObjects plugin field from realtime-only to the common client, adds a new RestObject class implementing REST get/publish with wire-format encode/decode, updates channels to use the relocated plugin field, extends typings/exports for REST objects, and adds comprehensive tests and test helpers for REST Objects. Changes
Sequence DiagramsequenceDiagram
participant Client as RestChannel
participant RestObj as RestObject
participant Transport as HTTP Transport
participant API as Objects API
rect rgba(70, 130, 180, 0.5)
Note over Client,API: GET / get() flow
Client->>RestObj: get(params)
RestObj->>Transport: HTTP GET /objects/{id or root}
Transport->>API: HTTP request
API-->>Transport: WireRestLiveObject / 404
Transport-->>RestObj: response
RestObj->>RestObj: _decodeWireLiveObject/_decodeWireObjectData
RestObj-->>Client: RestLiveObject | RestCompactObjectData | undefined
end
rect rgba(34, 139, 34, 0.5)
Note over Client,API: POST / publish() flow
Client->>RestObj: publish(operations[])
RestObj->>RestObj: _constructPublishBody() / _encodePrimitive()
RestObj->>Transport: HTTP POST /objects/publish (binary/JSON)
Transport->>API: HTTP request
API-->>Transport: RestObjectPublishResult
Transport-->>RestObj: response
RestObj->>RestObj: decode response
RestObj-->>Client: RestObjectPublishResult
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.40.5)test/realtime/liveobjects.test.jsThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
temporarily deployed
to
staging/pull/2109/features
github-actions
bot
temporarily deployed
to
staging/pull/2109/bundle-report
VeskeR
force-pushed
the
liveobjects/remove-non-path-based-api
branch
from
e7d734e to
70540ff
Compare
VeskeR
force-pushed
the
PUB-1197/liveobjects-rest-api
branch
2 times, most recently
from
15f2c4a to
5f7807f
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2109/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/2109/features
VeskeR
force-pushed
the
liveobjects/remove-non-path-based-api
branch
from
70540ff to
9c9d510
Compare
mschristensen
left a comment
mschristensen
left a comment
There was a problem hiding this comment.
LGTM, although I dont think these tests run with the useBinaryProtocol option set to true, given
Can we try running these tests with msgpack to see if it works? I did find this ticket which may indicate an issue with msgpack over rest currently: https://ably.atlassian.net/browse/PUB-1835
| {} as Record<string, any>, | ||
| ), | ||
| }; | ||
| case 'map.set': |
There was a problem hiding this comment.
Hello, These operation keys in their current format won't pass the rest validation. The current format can be found here in the docs https://ably.com/docs/api/liveobjects-rest#/paths/~1channels~1%7BchannelName%7D~1objects/post
For example map.set would have to be MAP_SET.
There was a problem hiding this comment.
test/rest/objects.test.js
Outdated
| const channel = client.channels.get('publish-map-set'); | ||
|
|
||
| const operation = { | ||
| operation: 'map.set', |
There was a problem hiding this comment.
This request will fail because the map set operation requires a key. If the intention of this request is to set a key of testKey on the root then it needs to be:
const operation = {
operation: 'map.set',
path: '',
key: 'testKey',
value: 'testValue',
};
|
Something I noticed when testing this branch for Msgpack requests. The SDK currently isn't setting the correct content type header of |
VeskeR
force-pushed
the
PUB-1197/liveobjects-rest-api
branch
from
5f7807f to
568d0a0
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2109/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/2109/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: 4
♻️ Duplicate comments (3)
test/rest/objects.test.js (2)
214-362:MAP_SETtest operations still missing requiredkeyfieldIn several publish tests, the
map.setoperations are constructed without akey:const operation = { operation: 'map.set', path: 'testKey', value: 'testValue', };and in the batch test:
{ operation: 'map.set', path: 'key1', value: 'value1', }, { operation: 'map.set', path: 'key2', value: 42, },Given the earlier review comment that the REST API requires a
keyfor map set and, for setting a key on the root, expectspath: ''andkey: 'testKey', these operations are still malformed and likely to fail REST validation. The current_constructPublishBodyalso expectsop.keyto be present when buildingdata.key.Recommend updating all root-level
map.setoperations in these tests along the lines of:-const operation = { - operation: 'map.set', - path: 'testKey', - value: 'testValue', -}; +const operation = { + operation: 'map.set', + path: '', + key: 'testKey', + value: 'testValue', +};and similarly for the batch:
-{ - operation: 'map.set', - path: 'key1', - value: 'value1', -}, +{ + operation: 'map.set', + path: '', + key: 'key1', + value: 'value1', +}, -{ - operation: 'map.set', - path: 'key2', - value: 42, -}, +{ + operation: 'map.set', + path: '', + key: 'key2', + value: 42, +},
366-412: Fix undefinedresultand alignMAP_SEToperation shape with APIIn the “publish multiple operations and verify state changes” test:
const operation = { operation: 'map.set', path: 'integrationKey', value: 'integrationValue', }; await channel.object.publish(operation); // ... expect(result).to.exist; expect(result.integrationKey).to.equal('integrationValue');Issues:
resultis never defined in this scope; this will throw aReferenceError. You probably meant to assert againstrootResult(retrieved viachannel.object.get()).- The
map.setoperation again omits thekeyfield and usespathas if it were the key, conflicting with the REST API shape and the earlier review comment.Suggested fix:
- const operation = { - operation: 'map.set', - path: 'integrationKey', - value: 'integrationValue', - }; + const operation = { + operation: 'map.set', + path: '', + key: 'integrationKey', + value: 'integrationValue', + }; @@ - // Get root object to verify counter reference + // Get root object to verify counter reference and map value const rootResult = await channel.object.get(); expect(rootResult).to.exist; expect(rootResult.testCounter).to.exist; expect(rootResult.testCounter.objectId).to.equal(counterObjectId); - expect(result).to.exist; - expect(result.integrationKey).to.equal('integrationValue'); + expect(rootResult.integrationKey).to.equal('integrationValue');src/plugins/objects/restobject.ts (1)
100-163: Map/counter publish body construction: fix operation names and rely on wire-level enumsTwo issues here:
- Operation name casing / format
The switch uses operation values like
'map.create','map.set','map.remove','counter.create','counter.inc', and then forwardsop.operationdirectly to the wire:const operation = op.operation; switch (operation) { case 'map.create': return { operation: op.operation, // ... }; // ... }Per the earlier review comment and REST docs, the wire format expects
MAP_CREATE,MAP_SET,MAP_REMOVE,COUNTER_CREATE,COUNTER_INC, etc. If you send the dotted lowercase strings, REST validation will fail.A robust approach is to map the SDK-level operations to the wire-level enums:
- private _constructPublishBody(op: RestObjectOperation, format: Utils.Format): any { - const operation = op.operation; + private _constructPublishBody(op: RestObjectOperation, format: Utils.Format): any { + const wireOperationMap: Record<string, string> = { + 'map.create': 'MAP_CREATE', + 'map.set': 'MAP_SET', + 'map.remove': 'MAP_REMOVE', + 'counter.create': 'COUNTER_CREATE', + 'counter.inc': 'COUNTER_INC', + }; + const operation = wireOperationMap[op.operation]; @@ - case 'map.create': + case 'MAP_CREATE': return { - operation: op.operation, + operation, @@ - case 'map.set': + case 'MAP_SET': return { - operation: op.operation, + operation, @@ - case 'map.remove': + case 'MAP_REMOVE': @@ - case 'counter.create': + case 'COUNTER_CREATE': @@ - case 'counter.inc': + case 'COUNTER_INC': @@ - default: + default: throw new this.channel.client.ErrorInfo('Unsupported publish operation action: ' + operation, 40003, 400);
- Assumptions about
map.setshapeThe
map.setcase builds:data: { key: op.key, value: { ...this._encodePrimitive(op.value, format), ...(op.encoding ? { encoding: op.encoding } : {}), }, },This is consistent with the test suite’s more complex usages (e.g.
objectId+key), but several tests still construct operations withoutkeyand try to overloadpath. Those tests should be fixed (see comments intest/rest/objects.test.js), rather than loosening this method.
🧹 Nitpick comments (2)
ably.d.ts (1)
2291-2538: REST Objects typings look consistent with runtime, but add property JSDoc to fix Typedoc warningsThe new REST object operation and data types (
RestObjectOperation*,RestObject*Data,GetObjectParams,PrimitiveOrObjectReference, and the updatedObjectOperationAction) are coherent and line up with theRestObjectimplementation (ids, extras, map/counter payloads, and operation action strings).The remaining issue is just documentation noise from Typedoc for the inline
TargetByObjectId/TargetByPathproperty types. You can keep the current aliases but document the properties explicitly so the API Reference job stops warning:-/** - * Targets an object by its object ID. - */ -type TargetByObjectId = { objectId: string; path?: never }; +/** + * Targets an object by its object ID. + */ +type TargetByObjectId = { + /** The ID of the object to target. Mutually exclusive with `path`. */ + objectId: string; + /** Must not be specified when targeting by `objectId`. */ + path?: never; +}; @@ -/** - * Targets an object by its location using the path. - * Paths are expressed relative to the structure of the object as defined by the compact view of the object tree. - */ -type TargetByPath = { path: string; objectId?: never }; +/** + * Targets an object by its location using the path. + * Paths are expressed relative to the structure of the object as defined by the compact view of the object tree. + */ +type TargetByPath = { + /** Path to the target, evaluated relative to the entrypoint object. Mutually exclusive with `objectId`. */ + path: string; + /** Must not be specified when targeting by `path`. */ + objectId?: never; +};This should resolve the API Reference warnings without changing the public surface.
Also applies to: 2626-2632, 3952-3960
test/common/modules/objects_helper.js (1)
42-127: Harden transport interception helpers to always restore original handler and add light guardsThe new helpers are handy, but the
onProtocolMessageinterception is a bit fragile:
- In both
waitForObjectOperationandwaitForObjectSync, ifonProtocolMessageOriginalthrows, thecatchrejects the promise but leavestransport.onProtocolMessagepatched, which can cascade into later tests.- Both helpers assume
client.connection.connectionManager.activeProtocolandgetTransport()are always available; if a test uses them before the connection is fully established, this will throw in a slightly opaque way.waitFixtureChannelIsReadyassumesentryPathObject.instance()is always non‑undefined; ifobject.get()ever returns a PathObject that doesn’t yet resolve to an instance,entryInstance.get(key)will throw.Consider a small defensive refactor that keeps behavior but makes these helpers safer in failing tests, for example:
- const transport = client.connection.connectionManager.activeProtocol.getTransport(); - const onProtocolMessageOriginal = transport.onProtocolMessage; + const activeProtocol = client.connection.connectionManager.activeProtocol; + if (!activeProtocol) { + return reject(new Error('No active protocol when waiting for object operation')); + } + const transport = activeProtocol.getTransport(); + const onProtocolMessageOriginal = transport.onProtocolMessage; @@ - transport.onProtocolMessage = function (message) { - try { - helper.recordPrivateApi('call.transport.onProtocolMessage'); - onProtocolMessageOriginal.call(transport, message); - - if (message.action === 19 && message.state[0]?.operation?.action === waitForAction) { - helper.recordPrivateApi('replace.transport.onProtocolMessage'); - transport.onProtocolMessage = onProtocolMessageOriginal; - resolve(); - } - } catch (err) { - reject(err); - } - }; + transport.onProtocolMessage = function (message) { + try { + helper.recordPrivateApi('call.transport.onProtocolMessage'); + onProtocolMessageOriginal.call(transport, message); + + if (message.action === 19 && message.state[0]?.operation?.action === waitForAction) { + resolve(); + } + } catch (err) { + reject(err); + } finally { + // Always restore the original handler once this helper completes + helper.recordPrivateApi('replace.transport.onProtocolMessage'); + transport.onProtocolMessage = onProtocolMessageOriginal; + } + };and similarly for
waitForObjectSync. Optionally, you could also guardentryInstanceinwaitFixtureChannelIsReadyand fail fast with a descriptive error if it can’t be resolved.These changes would make the tests more robust without changing their intended behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
ably.d.ts(3 hunks)objects.d.ts(2 hunks)src/common/lib/client/baseclient.ts(3 hunks)src/common/lib/client/baserealtime.ts(0 hunks)src/common/lib/client/realtimechannel.ts(1 hunks)src/common/lib/client/restchannel.ts(4 hunks)src/plugins/objects/index.ts(2 hunks)src/plugins/objects/restobject.ts(1 hunks)test/common/modules/objects_helper.js(1 hunks)test/realtime/objects.test.js(1 hunks)test/rest/objects.test.js(1 hunks)
💤 Files with no reviewable changes (1)
- src/common/lib/client/baserealtime.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/common/lib/client/realtimechannel.ts
- src/common/lib/client/baseclient.ts
- src/common/lib/client/restchannel.ts
🧰 Additional context used
🧬 Code graph analysis (4)
test/realtime/objects.test.js (1)
test/rest/objects.test.js (1)
waitFixtureChannelIsReady(13-13)
test/rest/objects.test.js (2)
src/common/lib/client/realtimechannel.ts (1)
object(153-158)src/common/lib/client/restchannel.ts (1)
object(71-76)
src/plugins/objects/restobject.ts (1)
ably.d.ts (6)
GetObjectParams(2493-2500)RestCompactObjectData(2407-2415)RestLiveObject(2421-2421)RestObjectOperation(2380-2385)RestObjectPublishResult(2391-2401)PrimitiveOrObjectReference(2632-2632)
ably.d.ts (1)
src/plugins/objects/restobject.ts (1)
RestObject(12-184)
🪛 GitHub Actions: API Reference
ably.d.ts
[warning] 1-1: Typedoc: ably.TargetByObjectId.__type.objectId does not have any documentation.
[warning] 1-1: Typedoc: ably.TargetByObjectId.__type.path does not have any documentation.
[warning] 1-1: Typedoc: ably.TargetByPath.__type.path does not have any documentation.
[warning] 1-1: Typedoc: ably.TargetByPath.__type.objectId does not have any documentation.
🪛 GitHub Actions: Bundle size report
src/plugins/objects/restobject.ts
[error] 1-1: Unexpected file src/plugins/objects/restobject.ts, contributes 2332B to bundle, more than allowed 100B
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (16.x)
- GitHub Check: test-node (18.x)
🔇 Additional comments (8)
objects.d.ts (1)
3-12: Objects plugin docs align with new REST supportThe updated imports and JSDoc correctly describe usage with both
Realtime/Restand modularBaseRealtime/BaseRest(including theRestplugin requirement). No issues from a typings or behavior perspective.Also applies to: 46-63
src/plugins/objects/index.ts (1)
5-22: RestObject export wiring looks correctImporting
RestObjectfrom./restobjectand exposing it in both the named and default exports matches how the other Objects types are surfaced; nothing further needed here.test/realtime/objects.test.js (1)
20-24: Reusing shared wait helpers fromObjectsHelperlooks goodAliasing these helpers from
ObjectsHelperkeeps this test file DRY and consistent with the REST tests. As long as the underlying helpers don’t depend on athiscontext (which appears true given their other usages), this change is sound and should be behavior‑preserving.test/rest/objects.test.js (3)
47-55: Channel.objecterror test looks goodThe “Rest without Objects plugin” test correctly asserts that accessing
channel.objectwithout the plugin throws with the expected message, matching theUtils.throwMissingPluginError('Objects')behavior fromrestchannel.ts.
66-210:RestObject.get()test coverage is comprehensive and consistentThe
RestObject.get()suite (non-existent IDs/paths, root default, compact true/false behaviors, objectId and path variants, primitive type handling, and binary checks viaBufferUtils) is thorough and matches the intended API semantics.These tests should give good confidence that
RestObject.getand the server responses behave correctly once the fixtures are in place.
415-477: Complex workflow test is solid, but depends on correct object-reference encodingThis test does a good job of exercising a realistic workflow (map.create, map.set, counter.create, then wiring a counter reference into the map) and verifying both compact and expanded views.
Note that the expectation:
expect(mapContent.map.entries.counter.data.objectId).to.equal(counterObjectId);assumes that
value: { objectId: counterObjectId }in themap.setoperation is preserved as an object reference on the wire, not JSON-stringified. This needsRestObject._encodePrimitiveto special‑case{ objectId: string }so that the server sees it as a reference rather than generic JSON.src/plugins/objects/restobject.ts (2)
19-56:get()flow looks reasonable; ensure headers / format match existing REST patternsThe
getimplementation follows the existing client patterns (choosingformatfromuseBinaryProtocol, usingenvelopebased onsupportsLinkHeaders, and decoding viaUtils.decodeBody). The 404 handling viaisErrorInfoOrPartialErrorInfoanderror.code === 40400is a good way to surface “not found” asundefined.Given the msgpack-related concerns on this branch, it’s worth double‑checking that
Defaults.defaultGetHeaders(client.options)sets an appropriateAcceptheader (and any other necessary flags) whenuseBinaryProtocolis true, in the same way as other REST GETs in the SDK, so that the server returns msgpack rather than JSON.
1-184: Address bundlesize failure forrestobject.tsThe bundlesize check reports:
Unexpected file
src/plugins/objects/restobject.ts, contributes 2332B to bundle, more than allowed 100BThis new file is clearly larger than the default allowance. You’ll need to either:
- Update the bundlesize configuration to include this file with an appropriate budget, or
- Restructure / split code (e.g. moving shared helpers or types) so that the per‑file contribution is within the configured threshold.
Given this is core new functionality, adjusting the bundlesize config to account for it is likely the pragmatic fix.
VeskeR
force-pushed
the
PUB-1197/liveobjects-rest-api
branch
from
568d0a0 to
879347f
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2109/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/2109/features
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
objects.d.ts (1)
67-67: Fix TypeScript export incompatibility.TypeScript does not allow
export =(CommonJS-style export assignment) to coexist with named exports like theLiveMapandLiveCounterclasses defined earlier in this file. This is causing the pipeline failure.Consider one of these solutions:
Solution 1 (recommended): Use default export
-declare const Objects: any; - -export = Objects; +export default Objects; +declare const Objects: any;Solution 2: Remove named exports and export everything through the default
MoveLiveMapandLiveCounterclass declarations to be properties of theObjectsobject instead of separate named exports.src/common/lib/client/realtimechannel.ts (1)
136-142: Consider caching the Push plugin as an internal field for consistency.The Objects plugin is now cached as
client._objectsPlugin, following the pattern established for other core plugins (Crypto, Annotations). However, Push continues to be accessed directly viaclient.options.plugins?.Pushthroughout the codebase. For consistency and potential performance benefits, consider caching Push similarly—e.g., asclient._pushPluginin BaseClient's constructor, similar to line 108.
♻️ Duplicate comments (3)
test/rest/objects.test.js (1)
15-21: Add default parameter foroptionsto prevent runtime errors.Both helper functions spread
optionsdirectly, which will throw aTypeErrorif called without the second argument. Several call sites (e.g., lines 108, 142, 208, 257) pass only thehelperargument viaRealtimeWithObjects(helper, options)whereoptionscomes from the test callback - this works, but if ever called without options it would fail.-function RealtimeWithObjects(helper, options) { - return helper.AblyRealtime({ ...options, plugins: { Objects: ObjectsPlugin } }); -} - -function RestWithObjects(helper, options) { - return helper.AblyRest({ ...options, plugins: { Objects: ObjectsPlugin } }); -} +function RealtimeWithObjects(helper, options = {}) { + return helper.AblyRealtime({ ...options, plugins: { Objects: ObjectsPlugin } }); +} + +function RestWithObjects(helper, options = {}) { + return helper.AblyRest({ ...options, plugins: { Objects: ObjectsPlugin } }); +}src/plugins/objects/restobject.ts (2)
125-125: Passformatparameter todefaultPostHeaders()to fix Content-Type for MsgPack.The
publish()method callsdefaultPostHeaders(client.options)without passing theformatparameter. This causes the Content-Type header to default to JSON regardless of theuseBinaryProtocolsetting, while the body is encoded as msgpack on line 133. This is the root cause of the MsgPack Content-Type issue reported in the PR comments.- const headers = client.Defaults.defaultPostHeaders(client.options); + const headers = client.Defaults.defaultPostHeaders(client.options, { format });
152-158: Change path segment from/objectto/objects.The REST API endpoint for LiveObjects is
/channels/{channelName}/objects(plural), but_basePathconstructs paths with/object(singular). This mismatch will cause all REST API calls to fail with 404 errors.return ( this._channel.client.rest.channelMixin.basePath(this._channel) + - '/object' + + '/objects' + (objectId ? '/' + encodeURIComponent(objectId) : '') );
🧹 Nitpick comments (8)
test/rest/request.test.js (1)
35-57: Consider adding a comment to explain the promise coordination pattern.The manual promise creation (lines 35-40) and the non-settling promise return (line 50) form a coordination pattern that ensures assertions complete before the test finishes. While this works correctly, a brief comment would help future maintainers understand why this approach is necessary.
Consider adding a comment like:
let savedResolve; let savedReject; + // Manual promise coordination: testRequestHandler returns a non-settling promise + // to keep the request "in progress" while we control test completion via savedResolve let promise = new Promise((res, rej) => {test/realtime/objects.test.js (1)
20-24: Centralising wait helpers viaObjectsHelperlooks goodPulling
waitFixtureChannelIsReady/waitFor*fromObjectsHelperkeeps waiter behaviour consistent across test suites and removes local duplication. The alias pattern is fine here as long as those helpers don’t rely onthis; if they ever do, consider binding them (ObjectsHelper.waitForMapKeyUpdate.bind(ObjectsHelper), etc.) as a small hardening step.test/rest/objects.test.js (2)
108-108: Close Realtime client after use to prevent resource leaks.The
RealtimeWithObjectsclient created here is used only to wait for the fixture channel to be ready but is never closed. This pattern repeats at lines 142, 208, and 257. Consider storing the client and closing it after the wait completes.- await waitFixtureChannelIsReady(RealtimeWithObjects(helper, options), objectsFixturesChannel); + const realtimeClient = RealtimeWithObjects(helper, options); + await waitFixtureChannelIsReady(realtimeClient, objectsFixturesChannel); + realtimeClient.close();
279-292: Consider movingprimitiveKeyDatadefinition before its first usage.The
primitiveKeyDataarray is used at line 262 inside theRestObject.get()tests but defined here after those tests. While this works due to hoisting, moving the definition before its first usage (before line 250) would improve readability.test/common/modules/objects_helper.js (2)
93-93: Replace magic numbers with named constants for clarity.The action values
19and20are protocol message action codes. Consider defining constants for these (e.g.,OBJECT_ACTION = 19,OBJECT_SYNC_ACTION = 20) to improve readability and maintainability.+ static OBJECT_ACTION = 19; + static OBJECT_SYNC_ACTION = 20; + static async waitForObjectOperation(helper, client, waitForAction) { // ... - if (message.action === 19 && message.state[0]?.operation?.action === waitForAction) { + if (message.action === ObjectsHelper.OBJECT_ACTION && message.state[0]?.operation?.action === waitForAction) {Also applies to: 117-117
81-127: Consider extracting shared transport interception logic.Both
waitForObjectOperationandwaitForObjectSyncfollow the same pattern: get transport, replaceonProtocolMessage, restore on match. The only difference is the condition. A shared helper could reduce duplication, though this is minor for test utilities.src/plugins/objects/restobject.ts (2)
86-91: Simplify redundant condition check.The
formatvariable is always truthy (either'msgpack'or'json'from line 70), so theif (format)condition at line 87 will always be true. The else branch is unreachable.- let decodedBody: RestCompactObjectData | WireRestLiveObject | undefined; - if (format) { - decodedBody = client.Utils.decodeBody(response.body, client._MsgPack, format); - } else { - decodedBody = response.body; - } + const decodedBody = client.Utils.decodeBody(response.body, client._MsgPack, format);
145-149: Simplify redundant condition check in publish().Similar to the
get()method,formatis always truthy here, making the condition unnecessary.- if (format) { - return client.Utils.decodeBody(response.body, client._MsgPack, format); - } - - return response.body!; + return client.Utils.decodeBody(response.body, client._MsgPack, format);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
ably.d.ts(3 hunks)objects.d.ts(2 hunks)src/common/lib/client/baseclient.ts(3 hunks)src/common/lib/client/baserealtime.ts(0 hunks)src/common/lib/client/realtimechannel.ts(1 hunks)src/common/lib/client/restchannel.ts(4 hunks)src/plugins/objects/index.ts(2 hunks)src/plugins/objects/restobject.ts(1 hunks)test/common/modules/objects_helper.js(1 hunks)test/common/modules/shared_helper.js(1 hunks)test/realtime/objects.test.js(4 hunks)test/rest/objects.test.js(1 hunks)test/rest/request.test.js(2 hunks)
💤 Files with no reviewable changes (1)
- src/common/lib/client/baserealtime.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/common/lib/client/baseclient.ts
🧰 Additional context used
🧬 Code graph analysis (5)
test/rest/request.test.js (2)
test/rest/http.test.js (3)
helper(11-11)helper(27-27)helper(72-72)test/rest/status.test.js (2)
helper(11-11)rest(38-38)
src/plugins/objects/restobject.ts (1)
ably.d.ts (8)
ObjectsMapSemantics(3975-3975)GetObjectParams(2493-2500)RestLiveObject(2421-2421)RestObjectOperation(2380-2385)RestObjectPublishResult(2391-2401)PrimitiveOrObjectReference(2632-2632)RestObjectMapEntry(2441-2444)RestObjectData(2450-2463)
test/rest/objects.test.js (2)
src/common/lib/client/realtimechannel.ts (1)
object(153-158)src/common/lib/client/restchannel.ts (1)
object(71-76)
src/common/lib/client/restchannel.ts (2)
src/plugins/objects/restobject.ts (1)
RestObject(59-354)src/plugins/objects/index.ts (1)
RestObject(12-12)
ably.d.ts (1)
src/plugins/objects/restobject.ts (1)
RestObject(59-354)
🪛 GitHub Actions: API Reference
ably.d.ts
[warning] 1-1: Typedoc warning: ably.TargetByObjectId.__type.objectId does not have any documentation.
[warning] 1-1: Typedoc warning: ably.TargetByObjectId.__type.path does not have any documentation.
[warning] 1-1: Typedoc warning: ably.TargetByPath.__type.path does not have any documentation.
[warning] 1-1: Typedoc warning: ably.TargetByPath.__type.objectId does not have any documentation.
[warning] 1-1: Typedoc warning: ably.OperationMapCreate.__type.operation does not have any documentation.
[warning] 1-1: Typedoc warning: ably.OperationMapCreate.__type.entries does not have any documentation.
[warning] 1-1: Typedoc warning: ably.OperationMapSet.__type.operation does not have any documentation.
[warning] 1-1: Typedoc warning: ably.OperationMapSet.__type.key does not have any documentation.
[warning] 1-1: Typedoc warning: ably.OperationMapSet.__type.value does not have any documentation.
[warning] 1-1: Typedoc warning: ably.OperationMapSet.__type.encoding does not have any documentation.
[warning] 1-1: Typedoc warning: ably.OperationMapRemove.__type.operation does not have any documentation.
[warning] 1-1: Typedoc warning: ably.OperationMapRemove.__type.key does not have any documentation.
[warning] 1-1: Typedoc warning: ably.OperationCounterCreate.__type.operation does not have any documentation.
[warning] 1-1: Typedoc warning: ably.OperationCounterCreate.__type.count does not have any documentation.
[warning] 1-1: Typedoc warning: ably.OperationCounterInc.__type.operation does not have any documentation.
[warning] 1-1: Typedoc warning: ably.OperationCounterInc.__type.amount does not have any documentation.
[warning] 1-1: Typedoc warning: ably.RestCompactObjectData.__type.objectId does not have any documentation.
[warning] 1-1: Typedoc warning: ably.RestObject.get.params.__type.compact does not have any documentation.
[warning] 1-1: Typedoc warning: ably.PrimitiveOrObjectReference.__type.objectId does not have any documentation.
🪛 GitHub Actions: Bundle size report
src/plugins/objects/restobject.ts
[error] 1-1: Unexpected file src/plugins/objects/restobject.ts, contributes 4040B to bundle, more than allowed 100B. Command failed during 'npm run modulereport' (exit code 1).
🪛 GitHub Actions: Test NPM package
objects.d.ts
[error] 67-67: TS2309: An export assignment cannot be used in a module with other exported elements.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (16.x)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (18.x)
🔇 Additional comments (6)
objects.d.ts (1)
8-8: Documentation updates look good.The additions of
RestClient,BaseRest, andRestto the imports and documentation accurately reflect the expanded REST support for Objects functionality.Also applies to: 11-11, 47-63
test/common/modules/shared_helper.js (1)
385-393: LGTM! Clean refactoring for async test support.The
createTesthelper unifies test invocation and enables passing thehelperas a third argument to test functions. The async-only approach is appropriate fortestOnJsonMsgpack, which differs fromtestOnAllTransportsAndProtocolsthat supports both callback and promise styles.src/plugins/objects/index.ts (1)
5-5: LGTM! RestObject export added correctly.The
RestObjectimport and export follow the same pattern as existing exports in this file.Also applies to: 12-12, 21-21
test/realtime/objects.test.js (1)
433-433: ReusingwaitFixtureChannelIsReadyfromObjectsHelperis an appropriate refactorSwitching these scenarios to
await waitFixtureChannelIsReady(client, objectsFixturesChannel);keeps fixture-channel readiness logic in one place and preserves the existing call shape, so the tests remain easy to follow.Also applies to: 605-605, 631-631
src/common/lib/client/restchannel.ts (1)
21-21: LGTM!The
RestObjectintegration follows the established plugin patterns used for Push and Annotations. The lazy initialization in the constructor and the defensive getter that throws when the plugin is missing are consistent with the existing codebase conventions.Also applies to: 38-38, 52-54, 71-76
src/plugins/objects/restobject.ts (1)
250-256: LGTM on object reference handling.The
_encodePrimitivemethod now correctly handles object references by checking for theobjectIdproperty before falling back to JSON stringification. This addresses the previously flagged issue where object references were incorrectly being JSON-encoded.
VeskeR
force-pushed
the
PUB-1197/liveobjects-rest-api
branch
from
879347f to
3ac5641
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2109/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/2109/features
VeskeR
force-pushed
the
PUB-1197/liveobjects-rest-api
branch
from
3ac5641 to
0fc6cd1
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2109/features
Fixed in https://github.com/ably/ably-js/compare/879347fb8c1ce499a871d718619a6f84987d6c62..3ac564136007a5f3b9d53bf7f30136d425a9cdba, now sends correct headers based on client format. |
Updated all Objects REST tests to run with msgpack and json, see 173b03c |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
test/rest/objects.test.js (1)
15-21: Guard againstoptionsbeingundefined.Both helpers spread
optionsdirectly, but call sites may pass onlyhelper. This was flagged in a previous review and should be addressed.-function RealtimeWithObjects(helper, options) { +function RealtimeWithObjects(helper, options = {}) { return helper.AblyRealtime({ ...options, plugins: { Objects: ObjectsPlugin } }); } -function RestWithObjects(helper, options) { +function RestWithObjects(helper, options = {}) { return helper.AblyRest({ ...options, plugins: { Objects: ObjectsPlugin } }); }src/plugins/objects/restobject.ts (1)
152-158: REST path segment must be/objects, not/object
_basePath()currently builds URLs using/object:this._channel.client.rest.channelMixin.basePath(this._channel) + '/object' + ...The LiveObjects REST API expects
/channels/{channelName}/objects(plural). Using/objectwill cause all REST object requests (get/publish) to hit a non‑existent endpoint.Recommend:
- '/object' + + '/objects' +This was already flagged previously and is still outstanding here.
🧹 Nitpick comments (5)
test/common/modules/objects_helper.js (2)
61-79: Consider adding timeouts to prevent indefinite hangs.Both
waitForMapKeyUpdateandwaitForCounterUpdatecreate promises that never reject. If the expected update never arrives (e.g., due to a bug or network issue), tests will hang until the global timeout.Consider adding optional timeout parameters:
-static async waitForMapKeyUpdate(mapInstance, key) { +static async waitForMapKeyUpdate(mapInstance, key, timeoutMs = 30000) { return new Promise((resolve, reject) => { + const timer = setTimeout(() => { + unsubscribe(); + reject(new Error(`Timeout waiting for map key update: ${key}`)); + }, timeoutMs); const { unsubscribe } = mapInstance.subscribe(({ message }) => { if (message?.operation?.mapOp?.key === key) { + clearTimeout(timer); unsubscribe(); resolve(); } }); }); }
81-127: Magic numbers for protocol message actions reduce readability.The action values
19(OBJECT) and20(OBJECT_SYNC) are used as magic numbers. Consider defining these as constants at the top of the file alongside the existingACTIONSobject for better maintainability.+const PROTOCOL_MESSAGE_ACTIONS = { + OBJECT: 19, + OBJECT_SYNC: 20, +}; + // In waitForObjectOperation: -if (message.action === 19 && message.state[0]?.operation?.action === waitForAction) { +if (message.action === PROTOCOL_MESSAGE_ACTIONS.OBJECT && message.state[0]?.operation?.action === waitForAction) { // In waitForObjectSync: -if (message.action === 20) { +if (message.action === PROTOCOL_MESSAGE_ACTIONS.OBJECT_SYNC) {test/rest/objects.test.js (1)
279-292: Consider movingprimitiveKeyDatadefinition before its first usage.
primitiveKeyDatais defined at line 279 but first referenced at line 262. While JavaScript hoisting makes this work, placing the constant before its first use improves readability.src/plugins/objects/restobject.ts (2)
238-259: Object reference vs JSON encoding handled appropriately in_encodePrimitiveBuffers go through
MessageEncoding.encodeDataForWire, primitives are wrapped in the corresponding discriminator field, and{ objectId: string }references are preserved instead of being JSON‑stringified, which is what the REST API expects.One minor thing to double‑check: top‑level
nullvalues and any unsupported shapes currently fall through toreturn value;and will be sent as raw JSON. If the REST API ever requires them to go via thejsonwrapper instead, you may want to normalize those through the{ json: JSON.stringify(value) }branch as well.
271-293: Live map decode logic is sound; consider reflecting'unknown'semantics in the typeDecoding live maps by transforming
WireObjectsMapSemanticstoObjectsMapSemanticsand recursively decodingentries.datainto eitherRestObjectDataor nestedRestLiveObjectmatches the runtime model. Since_decodeWireRestLiveMapcan emit'unknown'when it encounters an unsupported semantics value, you might want to extendObjectsMapSemanticsinably.d.ts(e.g.'lww' | 'unknown') so the type system reflects that possible runtime value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
ably.d.ts(3 hunks)objects.d.ts(2 hunks)scripts/moduleReport.ts(1 hunks)src/common/lib/client/baseclient.ts(3 hunks)src/common/lib/client/baserealtime.ts(0 hunks)src/common/lib/client/realtimechannel.ts(1 hunks)src/common/lib/client/restchannel.ts(4 hunks)src/plugins/objects/index.ts(2 hunks)src/plugins/objects/restobject.ts(1 hunks)test/common/modules/objects_helper.js(1 hunks)test/common/modules/shared_helper.js(1 hunks)test/realtime/objects.test.js(4 hunks)test/rest/objects.test.js(1 hunks)test/rest/request.test.js(2 hunks)
💤 Files with no reviewable changes (1)
- src/common/lib/client/baserealtime.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/common/lib/client/baseclient.ts
- src/common/lib/client/restchannel.ts
- src/common/lib/client/realtimechannel.ts
- objects.d.ts
🧰 Additional context used
🧬 Code graph analysis (6)
test/rest/request.test.js (2)
test/rest/http.test.js (3)
helper(11-11)helper(27-27)helper(72-72)test/rest/status.test.js (2)
helper(11-11)rest(38-38)
test/common/modules/shared_helper.js (1)
test/realtime/objects.test.js (1)
itFn(59-59)
test/realtime/objects.test.js (1)
test/rest/objects.test.js (2)
waitFixtureChannelIsReady(13-13)objectsFixturesChannel(12-12)
test/rest/objects.test.js (2)
src/common/lib/client/realtimechannel.ts (1)
object(153-158)src/common/lib/client/restchannel.ts (1)
object(71-76)
src/plugins/objects/restobject.ts (1)
ably.d.ts (9)
ObjectsMapSemantics(3975-3975)GetObjectParams(2493-2500)RestCompactObjectData(2407-2415)RestLiveObject(2421-2421)RestObjectOperation(2380-2385)RestObjectPublishResult(2391-2401)PrimitiveOrObjectReference(2632-2632)RestObjectMapEntry(2441-2444)RestObjectData(2450-2463)
ably.d.ts (1)
src/plugins/objects/restobject.ts (1)
RestObject(59-354)
🪛 GitHub Actions: API Reference
ably.d.ts
[warning] 1-1: Typedoc warning: definitions referenced in ably.d.ts do not have any documentation (e.g., ably.TargetByObjectId.__type.objectId).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test-node (16.x)
- GitHub Check: test-node (20.x)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (chromium)
🔇 Additional comments (14)
test/common/modules/objects_helper.js (1)
42-59: LGTM - Clear helper for fixture readiness.The implementation correctly waits for all expected fixture keys by checking if they already exist before subscribing for updates. Good use of
Promise.allfor parallel waiting.test/common/modules/shared_helper.js (1)
385-393: Clean refactor of test wrapper.The new
createTesthelper factory simplifies async test handling by returning the Promise directly, allowing Mocha to handle it natively. The addition ofhelperas a third parameter totestFnimproves ergonomics for tests that need helper access.test/rest/request.test.js (1)
31-57: LGTM - Proper async coordination for header verification.The manual Promise pattern correctly coordinates the asynchronous assertion in the mocked request handler. The test now properly awaits completion before the test function returns.
scripts/moduleReport.ts (1)
344-344: LGTM - RestObject added to allowed files for bundle analysis.Correctly includes the new
restobject.tsfile in the Objects plugin bundle size allowlist.src/plugins/objects/index.ts (1)
5-23: LGTM - RestObject properly exported.The RestObject is correctly added to both named and default exports, following the existing pattern for other plugin exports.
test/rest/objects.test.js (2)
228-246: Good handling of binary vs text protocol differences for bytes.The test correctly distinguishes between text protocol (base64 strings) and binary protocol (Buffer objects) when verifying byte data. This aligns with the PR objective comment about Content-Type header behavior with MsgPack.
632-729: Comprehensive workflow test validates complex object operations.The test effectively validates the full lifecycle of creating maps, counters, setting references, and verifying both compact and expanded representations. Good coverage of the REST Objects API.
test/realtime/objects.test.js (2)
20-24: Centralising wait helpers viaObjectsHelperis a good refactorReferencing the shared static helpers through local aliases removes in-file duplication and keeps the tests aligned with
objects_helper. No issues spotted with these bindings.
433-433: UpdatedwaitFixtureChannelIsReadycalls look correct and consistentPassing
objectsFixturesChannelexplicitly intowaitFixtureChannelIsReady(client, objectsFixturesChannel)matches the new helper signature and mirrors the pattern in the REST objects tests. This should keep the fixture channel setup consistent across scenarios.Also applies to: 605-605, 631-631
src/plugins/objects/restobject.ts (4)
23-57: Wire/live object wire types and semantics mapping look consistent with public typesThe
WireRest*wire shapes andmapSemanticsmapping (LWW → 'lww') align with the publicRestLive*andObjectsMapSemanticstypes, and the fallback handling viaWireAnyRestLiveObjectgives you a reasonable forward‑compatibility story.
66-116:get()encoding/decoding and 404 handling look correctUsing
formatto drivedecodeBody, falling back to primitives / JSON objects when not a live map, and treating 40400 “object not found” as a softundefinedall match the intended API surface forRestObject.get.
121-150: MsgPackContent-Typeheader now matches encoded body inpublish()Passing
{ format }intoclient.Defaults.defaultPostHeadersand then encoding the body withencodeBody(wireOps, client._MsgPack, format)ensures theContent-Typematches the actual wire format (JSON vs msgpack), which addresses the earlier bug reported for msgpack publishes.
160-236: Publish body construction matches REST operation contractThe mapping from
RestObjectOperationactions (map.create,map.set,map.remove,counter.create,counter.inc) to the uppercase wireoperationvalues (MAP_CREATE, etc.), along with the runtime validation for requiredpathon*.create, is consistent with the REST API and past review feedback.ably.d.ts (1)
2403-2538: REST object typings and operation/action wiring align with the implementation
RestObjectPublishResult,RestCompactObjectData,RestLiveObject/RestLiveMap/RestLiveCounter/AnyRestLiveObject, andGetObjectParamsare consistent with howRestObjectis implemented insrc/plugins/objects/restobject.ts.- The overloaded
RestObject.getandpublishsignatures mirror the runtime method overloads, including the compact vs live shapes.PrimitiveOrObjectReferencematches the shapes handled in_encodePrimitive(primitive values plus{ objectId: string }references).- Updating
ObjectOperationAction’s docstring to cover bothObjectOperationandRestObjectOperationkeeps the action value set in sync across realtime and REST.These declarations look correct and coherent with the rest of the Objects API surface.
Also applies to: 2626-2632, 3952-3960
| Helper.testOnJsonMsgpack('should get root object by default', async function (options, channelName, helper) { | ||
| const client = RestWithObjects(helper, options); | ||
| const channel = client.channels.get(objectsFixturesChannel); | ||
|
|
||
| await waitFixtureChannelIsReady(RealtimeWithObjects(helper, options), objectsFixturesChannel); | ||
|
|
||
| const root = await channel.object.get(); | ||
| expect(root).to.exist; | ||
| expect(root.initialValueCounter).to.equal(10); | ||
| }); |
There was a problem hiding this comment.
Realtime client created for fixture readiness may leak resources.
RealtimeWithObjects creates a Realtime client at line 108 to wait for fixture readiness, but it's never closed. This pattern repeats at lines 142, 208, and 257. Consider closing these clients after use to avoid resource leaks:
-await waitFixtureChannelIsReady(RealtimeWithObjects(helper, options), objectsFixturesChannel);
+const realtimeClient = RealtimeWithObjects(helper, options);
+await waitFixtureChannelIsReady(realtimeClient, objectsFixturesChannel);
+realtimeClient.close();Alternatively, create a shared Realtime client in a before hook and close it in after.
📝 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.
| Helper.testOnJsonMsgpack('should get root object by default', async function (options, channelName, helper) { | |
| const client = RestWithObjects(helper, options); | |
| const channel = client.channels.get(objectsFixturesChannel); | |
| await waitFixtureChannelIsReady(RealtimeWithObjects(helper, options), objectsFixturesChannel); | |
| const root = await channel.object.get(); | |
| expect(root).to.exist; | |
| expect(root.initialValueCounter).to.equal(10); | |
| }); | |
| Helper.testOnJsonMsgpack('should get root object by default', async function (options, channelName, helper) { | |
| const client = RestWithObjects(helper, options); | |
| const channel = client.channels.get(objectsFixturesChannel); | |
| const realtimeClient = RealtimeWithObjects(helper, options); | |
| await waitFixtureChannelIsReady(realtimeClient, objectsFixturesChannel); | |
| realtimeClient.close(); | |
| const root = await channel.object.get(); | |
| expect(root).to.exist; | |
| expect(root.initialValueCounter).to.equal(10); | |
| }); |
🤖 Prompt for AI Agents
In test/rest/objects.test.js around lines 104 to 113, the Realtime client
created via RealtimeWithObjects to wait for fixture readiness is never closed,
leaking resources; either call close()/disconnect() (or the appropriate cleanup
method) on the client returned by RealtimeWithObjects immediately after
waitFixtureChannelIsReady completes (ensure await completion before closing), or
refactor tests to create a single shared Realtime client in a before hook and
close it in an after hook so all waits reuse that client and no per-test clients
are leaked.
6f46f9d to
f9c7d38
Compare
VeskeR
force-pushed
the
PUB-1197/liveobjects-rest-api
branch
from
0fc6cd1 to
892ccc8
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2109/features
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/common/lib/client/baseclient.ts`:
- Around line 108-109: There is a stray nullish-coalescing operator causing a
syntax error in the assignment to this._liveObjectsPlugin; remove the extra "??
null" so the property assignment uses the intended expression
(options.plugins?.LiveObjects ?? null) or simply (options.plugins?.LiveObjects)
as appropriate; update the assignment in the constructor where
this._liveObjectsPlugin is set to reference the correct expression and ensure no
duplicated "?? null" remains.
In `@src/plugins/liveobjects/restobject.ts`:
- Around line 1-13: Update the type-only import in restobject.ts to use the
correct module path: replace the import source '../../../ably' with
'../../../liveobjects' for the listed types (GetObjectParams,
ObjectsMapSemantics, PrimitiveOrObjectReference, RestCompactObjectData,
RestLiveObject, RestObjectData, RestObjectMapEntry, RestObjectOperation,
RestObjectPublishResult) so the file imports the types from liveobjects.d.ts
like the rest of the plugin; keep the existing RestChannel and Utils imports
unchanged.
In `@test/common/modules/liveobjects_helper.js`:
- Around line 81-126: Both waitForObjectOperation and waitForObjectSync replace
transport.onProtocolMessage but don’t restore the original handler if the
wrapper throws; modify both wrappers (in waitForObjectOperation and
waitForObjectSync) to always restore transport.onProtocolMessage to
onProtocolMessageOriginal in a finally-style path: call the original handler
inside a try block, perform the action check and resolve if matched, and in a
finally block (or before every reject/resolve) reassign
transport.onProtocolMessage = onProtocolMessageOriginal so the original handler
is restored on success or error to avoid leaking the wrapper across tests.
In `@test/realtime/liveobjects.test.js`:
- Around line 461-462: Replace the undefined objectsFixturesChannel with the
existing liveobjectsFixturesChannel wherever used (examples: calls to
waitFixtureChannelIsReady and any other references at the noted spots) to avoid
ReferenceError; update calls like waitFixtureChannelIsReady(client,
objectsFixturesChannel) to use waitFixtureChannelIsReady(client,
liveobjectsFixturesChannel) and ensure any other uses of objectsFixturesChannel
in this test file are similarly switched to liveobjectsFixturesChannel.
| this._liveObjectsPlugin = options.plugins?.LiveObjects ?? null; ?? null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/common/lib/client/baseclient.ts | sed -n '100,115p'Repository: ably/ably-js
Length of output: 750
Remove the stray ?? null to fix the syntax error.
The extra ?? null; makes this file unparsable (TS1109).
🐛 Fix syntax error
- this._liveObjectsPlugin = options.plugins?.LiveObjects ?? null; ?? null;
+ this._liveObjectsPlugin = options.plugins?.LiveObjects ?? null;📝 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.
| this._liveObjectsPlugin = options.plugins?.LiveObjects ?? null; ?? null; | |
| } | |
| this._liveObjectsPlugin = options.plugins?.LiveObjects ?? null; | |
| } |
🧰 Tools
🪛 Biome (2.3.13)
[error] 108-109: Expected a statement but instead found '?? null'.
Expected a statement here.
(parse)
🪛 GitHub Actions: API Reference
[error] 108-108: TS1109: Expression expected.
🪛 GitHub Actions: Bundle size report
[error] 108-108: TS1109: Expression expected.
🪛 GitHub Actions: Lint
[error] 108-108: TS1109: Expression expected.
🪛 GitHub Actions: Spec coverage report
[error] 108-108: TS1109: Expression expected.
🪛 GitHub Actions: Test (react hooks)
[error] 108-108: TS1109: Expression expected.
🪛 GitHub Actions: Test browser
[error] 108-108: TS1109: Expression expected.
🪛 GitHub Actions: Test NodeJS
[error] 108-108: TS1109: Expression expected.
🪛 GitHub Actions: Test NPM package
[error] 108-110: TS1109: Expression expected.
🤖 Prompt for AI Agents
In `@src/common/lib/client/baseclient.ts` around lines 108 - 109, There is a stray
nullish-coalescing operator causing a syntax error in the assignment to
this._liveObjectsPlugin; remove the extra "?? null" so the property assignment
uses the intended expression (options.plugins?.LiveObjects ?? null) or simply
(options.plugins?.LiveObjects) as appropriate; update the assignment in the
constructor where this._liveObjectsPlugin is set to reference the correct
expression and ensure no duplicated "?? null" remains.
| static async waitForObjectOperation(helper, client, waitForAction) { | ||
| return new Promise((resolve, reject) => { | ||
| helper.recordPrivateApi('call.connectionManager.activeProtocol.getTransport'); | ||
| const transport = client.connection.connectionManager.activeProtocol.getTransport(); | ||
| const onProtocolMessageOriginal = transport.onProtocolMessage; | ||
|
|
||
| helper.recordPrivateApi('replace.transport.onProtocolMessage'); | ||
| transport.onProtocolMessage = function (message) { | ||
| try { | ||
| helper.recordPrivateApi('call.transport.onProtocolMessage'); | ||
| onProtocolMessageOriginal.call(transport, message); | ||
|
|
||
| if (message.action === 19 && message.state[0]?.operation?.action === waitForAction) { | ||
| helper.recordPrivateApi('replace.transport.onProtocolMessage'); | ||
| transport.onProtocolMessage = onProtocolMessageOriginal; | ||
| resolve(); | ||
| } | ||
| } catch (err) { | ||
| reject(err); | ||
| } | ||
| }; | ||
| }); | ||
| } | ||
|
|
||
| static async waitForObjectSync(helper, client) { | ||
| return new Promise((resolve, reject) => { | ||
| helper.recordPrivateApi('call.connectionManager.activeProtocol.getTransport'); | ||
| const transport = client.connection.connectionManager.activeProtocol.getTransport(); | ||
| const onProtocolMessageOriginal = transport.onProtocolMessage; | ||
|
|
||
| helper.recordPrivateApi('replace.transport.onProtocolMessage'); | ||
| transport.onProtocolMessage = function (message) { | ||
| try { | ||
| helper.recordPrivateApi('call.transport.onProtocolMessage'); | ||
| onProtocolMessageOriginal.call(transport, message); | ||
|
|
||
| if (message.action === 20) { | ||
| helper.recordPrivateApi('replace.transport.onProtocolMessage'); | ||
| transport.onProtocolMessage = onProtocolMessageOriginal; | ||
| resolve(); | ||
| } | ||
| } catch (err) { | ||
| reject(err); | ||
| } | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Restore transport.onProtocolMessage on error to avoid leaking the wrapper.
If the wrapper throws, the original handler is never restored, which can bleed into later tests. Consider restoring in a finally‑style path.
Proposed fix
static async waitForObjectOperation(helper, client, waitForAction) {
return new Promise((resolve, reject) => {
helper.recordPrivateApi('call.connectionManager.activeProtocol.getTransport');
const transport = client.connection.connectionManager.activeProtocol.getTransport();
const onProtocolMessageOriginal = transport.onProtocolMessage;
+ const restore = () => {
+ helper.recordPrivateApi('replace.transport.onProtocolMessage');
+ transport.onProtocolMessage = onProtocolMessageOriginal;
+ };
helper.recordPrivateApi('replace.transport.onProtocolMessage');
transport.onProtocolMessage = function (message) {
try {
helper.recordPrivateApi('call.transport.onProtocolMessage');
onProtocolMessageOriginal.call(transport, message);
if (message.action === 19 && message.state[0]?.operation?.action === waitForAction) {
- helper.recordPrivateApi('replace.transport.onProtocolMessage');
- transport.onProtocolMessage = onProtocolMessageOriginal;
+ restore();
resolve();
}
} catch (err) {
+ restore();
reject(err);
}
};
});
}
static async waitForObjectSync(helper, client) {
return new Promise((resolve, reject) => {
helper.recordPrivateApi('call.connectionManager.activeProtocol.getTransport');
const transport = client.connection.connectionManager.activeProtocol.getTransport();
const onProtocolMessageOriginal = transport.onProtocolMessage;
+ const restore = () => {
+ helper.recordPrivateApi('replace.transport.onProtocolMessage');
+ transport.onProtocolMessage = onProtocolMessageOriginal;
+ };
helper.recordPrivateApi('replace.transport.onProtocolMessage');
transport.onProtocolMessage = function (message) {
try {
helper.recordPrivateApi('call.transport.onProtocolMessage');
onProtocolMessageOriginal.call(transport, message);
if (message.action === 20) {
- helper.recordPrivateApi('replace.transport.onProtocolMessage');
- transport.onProtocolMessage = onProtocolMessageOriginal;
+ restore();
resolve();
}
} catch (err) {
+ restore();
reject(err);
}
};
});
}🤖 Prompt for AI Agents
In `@test/common/modules/liveobjects_helper.js` around lines 81 - 126, Both
waitForObjectOperation and waitForObjectSync replace transport.onProtocolMessage
but don’t restore the original handler if the wrapper throws; modify both
wrappers (in waitForObjectOperation and waitForObjectSync) to always restore
transport.onProtocolMessage to onProtocolMessageOriginal in a finally-style
path: call the original handler inside a try block, perform the action check and
resolve if matched, and in a finally block (or before every reject/resolve)
reassign transport.onProtocolMessage = onProtocolMessageOriginal so the original
handler is restored on success or error to avoid leaking the wrapper across
tests.
| await waitFixtureChannelIsReady(client, objectsFixturesChannel); | ||
|
|
There was a problem hiding this comment.
Fix undefined objectsFixturesChannel to avoid ReferenceError.
objectsFixturesChannel isn’t defined in this file, so these calls will fail at runtime. Either define it or use the existing liveobjectsFixturesChannel (which is already used to initialize fixtures).
🔧 Suggested fix (use existing fixture channel)
- await waitFixtureChannelIsReady(client, objectsFixturesChannel);
+ await waitFixtureChannelIsReady(client, liveobjectsFixturesChannel);- await waitFixtureChannelIsReady(client, objectsFixturesChannel);
+ await waitFixtureChannelIsReady(client, liveobjectsFixturesChannel);- await waitFixtureChannelIsReady(client, objectsFixturesChannel);
+ await waitFixtureChannelIsReady(client, liveobjectsFixturesChannel);Also applies to: 633-634, 659-660
🤖 Prompt for AI Agents
In `@test/realtime/liveobjects.test.js` around lines 461 - 462, Replace the
undefined objectsFixturesChannel with the existing liveobjectsFixturesChannel
wherever used (examples: calls to waitFixtureChannelIsReady and any other
references at the noted spots) to avoid ReferenceError; update calls like
waitFixtureChannelIsReady(client, objectsFixturesChannel) to use
waitFixtureChannelIsReady(client, liveobjectsFixturesChannel) and ensure any
other uses of objectsFixturesChannel in this test file are similarly switched to
liveobjectsFixturesChannel.
Adds REST support for Objects REST API endpoints:
- GET /channel/{channel}/object/{id?}?compact={boolean}&path={string}
- POST /channels/{channel}/object
Resolves PUB-1197
Test actually didn't work properly before this fix. Helper.testOnJsonMsgpack expects an async function test, but the test was callback based on tried to use done() callback - which wasn't even provided.
VeskeR
force-pushed
the
PUB-1197/liveobjects-rest-api
branch
from
892ccc8 to
1919291
Compare
3 participants
Resolves PUB-1197, AIT-27
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.