[DTP-1034] Emit Live Objects lifecycle events #1958
Conversation
|
Warning Rate limit exceeded@VeskeR has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 7 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe pull request introduces significant improvements to the event handling mechanism in the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
temporarily deployed
to
staging/pull/1958/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1958/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1958/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: 0
🧹 Nitpick comments (4)
src/plugins/liveobjects/liveobject.ts (2)
32-36: Consider adding event payload type.The lifecycle event callback and response interfaces are well-defined but could be enhanced to support future event types that might need payload data.
Consider this enhancement:
-export type LiveObjectLifecycleEventCallback = () => void; +export type LiveObjectLifecycleEventCallback<T = void> = (data?: T) => void; export interface OnLiveObjectLifecycleEventResponse { off(): void; }
166-166: Consider adding a deleted timestamp to the event payload.The
deletedevent could benefit from including the_tombstonedAttimestamp in its payload for better tracking of when the deletion occurred.- this._lifecycleEvents.emit(LiveObjectLifecycleEvent.deleted); + this._lifecycleEvents.emit(LiveObjectLifecycleEvent.deleted, { deletedAt: this._tombstonedAt });src/plugins/liveobjects/liveobjects.ts (2)
167-170: Consider adding type guard for event parameter.Similar to the
LiveObjectclass, there's a defensive check for nil arguments. Consider adding a type guard for the event parameter.- if (this._client.Utils.isNil(event) && this._client.Utils.isNil(callback)) { + if (!Object.values(LiveObjectsEvent).includes(event) || this._client.Utils.isNil(callback)) { return; }
314-315: Consider adding sync metadata to event payload.The sync events could provide valuable metadata about the sync operation to help with debugging and monitoring.
- this._eventEmitterInternal.emit(LiveObjectsEvent.sync_start); - this._eventEmitterPublic.emit(LiveObjectsEvent.sync_start); + const syncStartMetadata = { syncId: this._currentSyncId, cursor: this._currentSyncCursor }; + this._eventEmitterInternal.emit(LiveObjectsEvent.sync_start, syncStartMetadata); + this._eventEmitterPublic.emit(LiveObjectsEvent.sync_start, syncStartMetadata);- this._eventEmitterInternal.emit(LiveObjectsEvent.sync_end); - this._eventEmitterPublic.emit(LiveObjectsEvent.sync_end); + const syncEndMetadata = { + syncId: this._currentSyncId, + bufferedOperationsCount: this._bufferedStateOperations.length + }; + this._eventEmitterInternal.emit(LiveObjectsEvent.sync_end, syncEndMetadata); + this._eventEmitterPublic.emit(LiveObjectsEvent.sync_end, syncEndMetadata);Also applies to: 329-330
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/plugins/liveobjects/liveobject.ts(8 hunks)src/plugins/liveobjects/liveobjects.ts(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (20.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (16.x)
- GitHub Check: test-browser (chromium)
🔇 Additional comments (5)
src/plugins/liveobjects/liveobject.ts (3)
6-7: LGTM! Consistent event naming convention.The event names follow the lowercase snake_case convention, which is consistent across both
LiveObjectSubscriptionEventandLiveObjectLifecycleEventenums.Also applies to: 28-30
43-44: LGTM! Good separation of event emitters.The separation of subscription and lifecycle events into distinct event emitters improves the code organization and maintainability.
Also applies to: 69-70
123-126: LGTM! Defensive programming.Good defensive check to prevent accidental removal of all event listeners when both arguments are nil.
src/plugins/liveobjects/liveobjects.ts (2)
14-16: LGTM! Clear sync event naming.The sync events follow a consistent naming pattern and clearly indicate the start and end of sync operations.
32-34: LGTM! Good documentation of event emitter separation.The comment referencing RTC10 provides clear context for why there are separate internal and public event emitters.
| sync_start = 'sync_start', | ||
| sync_end = 'sync_end', |
There was a problem hiding this comment.
I think these events should reflect the current "state" (in the state-machine sense) of the liveobjects instance, similar to how we do for channels and connections
So I would suggest initialized, syncing, and synced, analogous to initialized, connecting and connected for connection state or initialized, attaching and attached for channels.
There was a problem hiding this comment.
sounds good, I think for initialized I will need to add another flag (or more likely an explicit state) to the LiveObjects class to distinguish between sync is in progress (we received an ATTACHED with HAS_STATE flag) and when LiveObjects instance was just created. Currently there is only _syncInProgress flag that only covers syncing and synced.
Also, would you expect to always get a syncing event before getting a synced event? This is important because with the current implementation, we can be in a synced state and then reattach to a channel that doesn't have HAS_STATE flag - this essentially removes the state immediately and emits the synced event again, without emitting syncing event before that.
see here:
ably-js/src/plugins/liveobjects/liveobjects.ts
Lines 114 to 131 in 210af2d
There was a problem hiding this comment.
this._endSync(); call will just emit synced event again in this case, but we never entered the syncing state so never emitted that
There was a problem hiding this comment.
What do we do for channels/connections? Do we enforce that you always get e.g. connecting before connected?
There was a problem hiding this comment.
Small note from the standup: HAS_STATE flag on channel attachment is always true if state_subscribe mode is set, so this guarantees we can have syncing -> synced order, given HAS_STATE flag is set (state_subscribe mode is set for a channel)
What do we do for channels/connections? Do we enforce that you always get e.g. connecting before connected?
I couldn't find an explicit mention of connecting -> connected order, but I think it is guaranteed implicitly based on the next:
The RealtimeChannel will (potentially) emit a separate update event when it gets an ATTACHED message while being in an attached state:
ably-js/src/common/lib/client/realtimechannel.ts
Lines 524 to 526 in 471cdef
And more generally, if there is no special logic for an event, then
notifyEvent method won't emit an event if current state of the channel matches the one it's attempting to notify: ably-js/src/common/lib/client/realtimechannel.ts
Lines 718 to 720 in 471cdef
Spec point matches the behavior described above: https://sdk.ably.com/builds/ably/specification/main/features/#RTL12
The Connection (more precisely ConnectionManager) has pretty much the same behavior, where there is sometimes special logic to emit update event when connection configuration changes without an explicit reconnection:
ably-js/src/common/lib/transport/connectionmanager.ts
Lines 700 to 717 in 471cdef
ably-js/src/common/lib/transport/connectionmanager.ts
Lines 1217 to 1218 in 471cdef
And relevant spec point: https://sdk.ably.com/builds/ably/specification/main/features/#RTN24
based on this we can do the following:
- have
initialized,syncing,syncedandupdateevents/states. - we won't (generally) emit a state event if we're currently in it (can't receive two
syncingevents in a row). - if we're currently in
syncedand: -
- receive a
STATE_SYNCmessage that can be immediately applied (sync cursor is empty), we will emit anupdateevent and stay insynced. otherwise we entersyncingand wait for sync sequence to complete as usual.
- receive a
-
- we reattach to a channel with no
HAS_STATEflag - we emitupdateevent and stay insynced
- we reattach to a channel with no
- also, I suggest we still allow to subscribe to events even if
state_subscribemode wasn't set (meaning we won't receive aHAS_STATEflag on attachment) to improve DX (say you attach to a channel without the mode, get liveobjects instance and subscribe to lifecycle events now, and only at some time in the future you reattach with correct modes. if we don't allow subscribing unlessstate_subscribemode is set this no longer becomes possible). To maintain a consistentsyncing->syncedorder, when a channel is attached while in theinitializedstate 1. it will transition tosyncing. 2. on the next event loop, it will emitsyncedsince it is immediately synchronized due to the absence of aHAS_STATEflag.
There was a problem hiding this comment.
updated implementation in:
98d66e2 - adds state to LiveObjects just like channel/connection
cf6f5fd - emits state change events
as discussed on the standup yesterday, I removed the update event as it will have different meaning here than in channel/connection (update here would simply mean that underlying data has changed, and we have live object subscriptions for that).
There was a problem hiding this comment.
also should point out that it is not technically possible to receive a initialized event as it is set on channel/connection object construction at which point we can't event subscribe to events yet.
and no other state can transition back to initialized, so it's impossible to get it later too.
thus we don't emit initialized in liveobjects either, we just start from this state
VeskeR
force-pushed
the
DTP-948/error-wrong-state-mode-attach
branch
from
5d78643 to
a75d500
Compare
VeskeR
force-pushed
the
DTP-1034/liveobjects-lifecycle-events
branch
from
eedbe01 to
1f062f0
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1958/features
github-actions
bot
temporarily deployed
to
staging/pull/1958/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1958/bundle-report
VeskeR
force-pushed
the
DTP-948/error-wrong-state-mode-attach
branch
from
a75d500 to
53d07ff
Compare
VeskeR
force-pushed
the
DTP-1034/liveobjects-lifecycle-events
branch
from
1f062f0 to
6824c38
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1958/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1958/features
github-actions
bot
temporarily deployed
to
staging/pull/1958/typedoc
VeskeR
force-pushed
the
DTP-1034/liveobjects-lifecycle-events
branch
from
6824c38 to
8c2a8de
Compare
VeskeR
changed the base branch from
integration/liveobjects
to
DTP-1147/fix-flaky-liveobjects-tests
github-actions
bot
temporarily deployed
to
staging/pull/1958/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1958/features
github-actions
bot
temporarily deployed
to
staging/pull/1958/typedoc
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/plugins/liveobjects/liveobject.ts(8 hunks)src/plugins/liveobjects/liveobjects.ts(6 hunks)src/plugins/liveobjects/objectid.ts(0 hunks)test/common/modules/live_objects_helper.js(2 hunks)test/realtime/live_objects.test.js(78 hunks)
💤 Files with no reviewable changes (1)
- src/plugins/liveobjects/objectid.ts
👮 Files not reviewed due to content moderation or server errors (1)
- test/realtime/live_objects.test.js
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (20.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-node (16.x)
🔇 Additional comments (12)
src/plugins/liveobjects/liveobject.ts (6)
Line range hint
6-36: LGTM! Clear separation of subscription and lifecycle events.The enums and types are well-structured with a clear separation between subscription and lifecycle events. The naming convention follows a consistent lowercase pattern.
43-44: LGTM! Good separation of event emitters.The split into separate event emitters for subscriptions and lifecycle events improves separation of concerns and maintainability.
69-70: LGTM! Consistent event emitter initialization.Both event emitters are correctly initialized with the client logger.
109-133: LGTM! Well-implemented lifecycle event methods.The lifecycle event methods are well-implemented with:
- Consistent pattern matching subscription methods
- Proper null checks to prevent accidental removal of all callbacks
- Clear and concise method signatures
Line range hint
144-154: LGTM! Clean update to use new event enum.The method correctly uses the new subscription event enum while maintaining the existing noop check logic.
166-166: LGTM! Proper lifecycle event emission.The deleted event is correctly emitted after setting the tombstone state.
src/plugins/liveobjects/liveobjects.ts (3)
32-34: LGTM! Good separation of internal and public event emitters.The split into separate event emitters for internal and public events improves encapsulation and aligns with RTC10 spec.
48-49: LGTM! Consistent event emitter initialization.Both event emitters are correctly initialized with the client logger.
154-179: LGTM! Well-implemented event methods.The event methods are well-implemented with:
- Consistent pattern across methods
- Proper null checks to prevent accidental removal of all callbacks
- Clear and concise method signatures
test/common/modules/live_objects_helper.js (3)
28-29: LGTM! Exposing ACTIONS as static property improves test maintainability.Making ACTIONS accessible as a static property allows for better test assertions and reduces duplication.
30-32: LGTM! Centralizing fixture root keys improves maintainability.The static fixtureRootKeys method provides a single source of truth for fixture keys, making it easier to maintain and update test data.
301-301: LGTM! Adding timestamp to object IDs ensures uniqueness.Appending
Date.now()to object IDs helps prevent collisions in tests, especially when running tests in rapid succession.Also applies to: 305-305
Currently emits `deleted` upon receiving `OBJECT_DELETE` for the object DTP-1034
VeskeR
force-pushed
the
DTP-1034/liveobjects-lifecycle-events
branch
from
8c2a8de to
3c40056
Compare
Functionally complete but not tests yet as some of them depend on DTP-1147. Worth revieweing this PR API side while I complete tests setup.
Resolves DTP-1034
Summary by CodeRabbit
New Features
on,off, andoffAllmethods.Improvements
sync_startandsync_end.