[PUB-1197] LiveObjects REST client #2109
Conversation
… with the naming convention for other realtime features in the SDK This updates ably-js implementation to match the spec [1] [1] ably/specification#332
Change the name for the `Objects` class to `RealtimeObjects`
…RealtimeObjects` to `RealtimeObject` Resolves https://ably.atlassian.net/browse/PUB-2059
[PUB-2059] Change LiveObjects entry point to `channel.object.get()`
Update Objects docstrings to not use "root" terminology
Adds new path-based types to ably.d.ts. The previous LiveObject types are temporarily marked as *Deprecated in ably.d.ts until they are fully converted to the new type system in the following PRs. The `RealtimeObject` entrypoint temporarily adds a `getPathObject` method to get a PathObject instance for a root and preserves the existing `.get()` method which returns a LiveMap instance directly so that corresponding tests do not fail yet. Resolves PUB-2057
[PUB-2057] Implement base PathObject class with access and mutation methods
Resolves PUB-2063
This makes LiveObjects Instance methods to be in line with PathObject methods handling of wrong underlying type, as discussed in [1]. Type/behavior overview: - id(): may return `undefined` (when Instance is from .get() call that returned a primitive instance) - compact(): is defined for all Instance types so can be non-nullable. See method implementation in [2] - get(): may return `undefined` when not a map - entries()/keys()/values()/size(): may return `undefined` when not a map - value(): may return `undefined` when not a counter or a primitive [1] #2091 (comment) [2] #2098
Exposes LiveMap.create() and LiveCounter.create() static methods at ably/objects export. Updates all objects tests to use these new methods to create LiveMap and LiveCounter instead of realtimeObject.createMap()/.createCounter() calls Resolves PUB-2060
They are replaced by object creation via value types from previous commit
[PUB-2063] Implement Instance API for LiveObjects PathObject
[PUB-2060] Implement LiveMap and LiveCounter creation via value types
Implements: - LiveObject parent tracking - Path-based LiveObject event emission - PathObject subscriptions path matching with deep subscriptions - Path event emission for LiveMap key updates - Full ObjectMessage (user-facing type) argument for subscription callbacks Resolves PUB-2061
Stricter typing removes `ArrayBufferView` from the union type to align with the publicly exposed API, which only uses a union of `Buffer` and `ArrayBuffer`. This change enables the internal `ObjectMessage.operation` object to be exposed as the public `ObjectOperation` type in the following commit without buffer type mismatches for underlying values.
…iveObject subscriptions LiveObject subscriptions (via PathObject and Instance APIs) now expose the full operation object that caused the change in the `event.message.operation` field.
This was replaced by PathObject/Instance subscription API from previous commits
…eObjects Resolves PUB-2064
…s for binary values
[PUB-2061] Add path based subscriptions support for LiveObjects
[PUB-2064] Implement `.compact()` method for PathObject and Instance types for LiveObjects
Resolves PUB-2062
[PUB-2062] Implement async iterator subscriptions API for LiveObjects
WalkthroughAdds REST-based Objects support: new TypeScript types, a RestObject class with get/publish on RestChannel, BaseClient plugin wiring, RestChannel object getter and annotations getter, plugin export updates, a RestObject implementation, and new/updated tests and helpers for REST and realtime object operations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RestChannel
participant RestObject
participant AblyAPI
Note over Client,RestObject: RestObject.get() flow
Client->>RestChannel: access channel.object
RestChannel->>RestObject: lazy-init (new RestObject(channel))
Client->>RestObject: get(params)
RestObject->>RestObject: _basePath(params)
RestObject->>AblyAPI: HTTP GET /objects{path|id}
AblyAPI-->>RestObject: 200/404 + payload
RestObject->>RestObject: decode (msgpack/json)
RestObject-->>Client: RestCompactObjectData | RestLiveObject | undefined
Note over Client,RestObject: RestObject.publish() flow
Client->>RestObject: publish(op or [ops])
RestObject->>RestObject: _constructPublishBody(ops)
RestObject->>RestObject: _encodePrimitive(...) for values
RestObject->>AblyAPI: HTTP POST /objects{path|id} with encoded body
AblyAPI-->>RestObject: 200 + publish result
RestObject-->>Client: RestObjectPublishResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks 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
|
Something I noticed when testing this branch for Msgpack requests. The SDK currently isn't setting the correct content type header of |
Remove non-path-based API from LiveObjects
[PUB-3439] Remove AblyObjectsTypes and expect the types for the object structure to be provided explicitly
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
Adds REST support for Objects REST API endpoints:
- GET /channel/{channel}/object/{id?}?compact={boolean}&path={string}
- POST /channels/{channel}/object
Resolves PUB-1197
VeskeR
force-pushed
the
PUB-1197/liveobjects-rest-api
branch
from
3ac5641 to
0fc6cd1
Compare
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
| /** | ||
| * Base interface for all REST object operations. Contains common fields shared across all operation types. | ||
| */ | ||
| export interface RestObjectOperationBase { | ||
| /** | ||
| * An ID associated with the message. Clients may set this field explicitly when publishing an operation to enable | ||
| * idempotent publishing. If not set, this will be generated by the server. | ||
| */ | ||
| id?: string; | ||
| /** | ||
| * A JSON object of arbitrary key-value pairs that may contain metadata, and/or ancillary payloads. Valid payloads include `headers`. | ||
| */ | ||
| extras?: { | ||
| /** | ||
| * A set of key–value pair headers included with this object message. | ||
| */ | ||
| headers?: Record<string, string>; | ||
| [key: string]: any; | ||
| }; | ||
| } | ||
|
|
||
| // Enforce exactly one target at compile time using a union of the types below. | ||
| /** | ||
| * Targets an object by its object ID. | ||
| */ | ||
| type TargetByObjectId = { objectId: string; 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 }; | ||
|
|
||
| /** | ||
| * Base type for operations that can target objects either by object ID or by path. | ||
| * Ensures that exactly one targeting method is specified. | ||
| */ | ||
| export type AnyTargetOperationBase = RestObjectOperationBase & (TargetByObjectId | TargetByPath); | ||
|
|
||
| /** | ||
| * Operation to create a new map object at the specified path with initial entries. | ||
| */ | ||
| export type OperationMapCreate = RestObjectOperationBase & | ||
| TargetByPath & { | ||
| operation: ObjectOperationActions.MAP_CREATE; | ||
| entries: Record<string, PrimitiveOrObjectReference>; | ||
| }; | ||
|
|
||
| /** | ||
| * Operation to set a key to a specified value in an existing map object. | ||
| * Can target the map by either object ID or path. | ||
| */ | ||
| export type OperationMapSet = AnyTargetOperationBase & { | ||
| operation: ObjectOperationActions.MAP_SET; | ||
| key: string; | ||
| value: PrimitiveOrObjectReference; | ||
| encoding?: string; | ||
| }; | ||
|
|
||
| /** | ||
| * Operation to remove a key from an existing map object. | ||
| * Can target the map by either object ID or path. | ||
| */ | ||
| export type OperationMapRemove = AnyTargetOperationBase & { | ||
| operation: ObjectOperationActions.MAP_REMOVE; | ||
| key: string; | ||
| }; | ||
|
|
||
| /** | ||
| * Operation to create a new counter object at the specified path with an initial count value. | ||
| */ | ||
| export type OperationCounterCreate = RestObjectOperationBase & | ||
| TargetByPath & { | ||
| operation: ObjectOperationActions.COUNTER_CREATE; | ||
| count: number; | ||
| }; | ||
|
|
||
| /** | ||
| * Operation to increment (or decrement with negative values) an existing counter object. | ||
| * Can target the counter by either object ID or path. | ||
| */ | ||
| export type OperationCounterInc = AnyTargetOperationBase & { | ||
| operation: ObjectOperationActions.COUNTER_INC; | ||
| amount: number; | ||
| }; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "ably.d.ts" -type fRepository: ably/ably-js
Length of output: 93
🏁 Script executed:
wc -l ./ably.d.tsRepository: ably/ably-js
Length of output: 72
🏁 Script executed:
sed -n '2291,2376p' ./ably.d.tsRepository: ably/ably-js
Length of output: 2815
🏁 Script executed:
sed -n '2376,2450p' ./ably.d.tsRepository: ably/ably-js
Length of output: 2323
🏁 Script executed:
rg "TargetByObjectId\.__type" . 2>/dev/null | head -20Repository: ably/ably-js
Length of output: 38
🏁 Script executed:
rg "type.*=\s*\{.*:.*;" ./ably.d.ts | head -30Repository: ably/ably-js
Length of output: 289
🏁 Script executed:
sed -n '2320,2340p' ./ably.d.ts | cat -ARepository: ably/ably-js
Length of output: 862
Add property-level JSDoc to TargetByObjectId, TargetByPath, and Operation types to document inline object properties*
The API Reference job warns about undocumented fields like ably.TargetByObjectId.__type.objectId. This occurs because these inline object type aliases lack JSDoc on individual properties:
type TargetByObjectId = { objectId: string; path?: never };
type TargetByPath = { path: string; objectId?: never };The same issue affects OperationMapCreate, OperationMapSet, OperationMapRemove, OperationCounterCreate, and OperationCounterInc.
Document each property to satisfy TypeDoc and improve clarity:
-/**
- * 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. */
+ objectId: string;
+ /**
+ * Must not be set when targeting by object ID. Present only to enforce
+ * compile‑time exclusivity with {@link TargetByPath}.
+ */
+ 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 object relative to the channel's root object, expressed using
+ * the compact‑view object tree syntax.
+ */
+ path: string;
+ /**
+ * Must not be set when targeting by path. Present only to enforce
+ * compile‑time exclusivity with {@link TargetByObjectId}.
+ */
+ objectId?: never;
+};
@@
-export type OperationMapCreate = RestObjectOperationBase &
- TargetByPath & {
- operation: ObjectOperationActions.MAP_CREATE;
- entries: Record<string, PrimitiveOrObjectReference>;
- };
+export type OperationMapCreate = RestObjectOperationBase &
+ TargetByPath & {
+ /** The operation action, must be {@link ObjectOperationActions.MAP_CREATE}. */
+ operation: ObjectOperationActions.MAP_CREATE;
+ /** Initial map entries to create at the target path. */
+ entries: Record<string, PrimitiveOrObjectReference>;
+ };
@@
-export type OperationMapSet = AnyTargetOperationBase & {
- operation: ObjectOperationActions.MAP_SET;
- key: string;
- value: PrimitiveOrObjectReference;
- encoding?: string;
-};
+export type OperationMapSet = AnyTargetOperationBase & {
+ /** The operation action, must be {@link ObjectOperationActions.MAP_SET}. */
+ operation: ObjectOperationActions.MAP_SET;
+ /** The key within the map to set. */
+ key: string;
+ /** The value to assign at the given key. */
+ value: PrimitiveOrObjectReference;
+ /**
+ * Optional encoding for the value (for example a custom content‑type or
+ * transformation hint).
+ */
+ encoding?: string;
+};
@@
-export type OperationMapRemove = AnyTargetOperationBase & {
- operation: ObjectOperationActions.MAP_REMOVE;
- key: string;
-};
+export type OperationMapRemove = AnyTargetOperationBase & {
+ /** The operation action, must be {@link ObjectOperationActions.MAP_REMOVE}. */
+ operation: ObjectOperationActions.MAP_REMOVE;
+ /** The key to remove from the map. */
+ key: string;
+};
@@
-export type OperationCounterCreate = RestObjectOperationBase &
- TargetByPath & {
- operation: ObjectOperationActions.COUNTER_CREATE;
- count: number;
- };
+export type OperationCounterCreate = RestObjectOperationBase &
+ TargetByPath & {
+ /** The operation action, must be {@link ObjectOperationActions.COUNTER_CREATE}. */
+ operation: ObjectOperationActions.COUNTER_CREATE;
+ /** Initial value to assign to the new counter. */
+ count: number;
+ };
@@
-export type OperationCounterInc = AnyTargetOperationBase & {
- operation: ObjectOperationActions.COUNTER_INC;
- amount: number;
-};
+export type OperationCounterInc = AnyTargetOperationBase & {
+ /** The operation action, must be {@link ObjectOperationActions.COUNTER_INC}. */
+ operation: ObjectOperationActions.COUNTER_INC;
+ /**
+ * Amount by which to change the counter. Negative values decrement the
+ * counter.
+ */
+ amount: number;
+};After applying these changes, re-run the API Reference workflow to confirm the TypeDoc warnings are resolved.
| 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
Resolves PUB-1197, AIT-27
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.