[ECO-5658] Switch to using ably-swift's EventEmitter#131
Draft
lawrence-forooghian wants to merge 3 commits into
Draft
[ECO-5658] Switch to using ably-swift's EventEmitter#131lawrence-forooghian wants to merge 3 commits into
EventEmitter#131lawrence-forooghian wants to merge 3 commits into
Conversation
Copy EventEmitter.swift, InternalEventEmitter.swift, DefaultInternalEventEmitter.swift and its tests from ably-swift commit 98996f1, making the EventEmitter protocol internal. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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/131/AblyLiveObjects
The liveobjects plugin uses DispatchQueue-based concurrency, not @mainactor. Adapt the EventEmitter (copied from ably-swift) to match: - Remove @mainactor from all protocols and classes - Add Sendable conformance throughout - Store mutable state in DispatchQueueMutex - Prefix all methods with nosync_ (must be called on internal queue) - Rename typealiases: MainActorEventListener → EventListener, MainActorNamedEventListener → NamedEventListener (now @sendable) - SubscriptionController takes internalQueue in init, uses DispatchQueueMutex for its registrations - DefaultInternalEventEmitter snapshots listeners inside withoutSync, calls them outside to avoid exclusivity violations on re-entry - Update tests to use ably_syncNoDeadlock and nosync_ methods Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
SubscriptionStorage was a struct that manually managed subscription bookkeeping. Because it was a value type, unsubscription required a verbose updateSelfLater closure-threading pattern at every call site. Replace it with a class wrapping DefaultInternalEventEmitter. A private EmitData struct bundles the update with the dispatch queue so nosync_emit can still accept a queue parameter. Each nosync_subscribe creates a SubscriptionController whose signal is passed to the emitter; the returned SubscribeResponse captures the controller and synchronously dispatches nosync_off() on unsubscribe. Because SubscriptionStorage is now a reference type, the updateSelfLater closures throughout LiveObjectMutableState, InternalDefaultLiveCounter, InternalDefaultLiveMap, and InternalDefaultRealtimeObjects are no longer needed and are removed. The subscription/emit methods on LiveObjectMutableState drop their mutating qualifier and gain a nosync_ prefix for consistency. TODO: Lawrence check that this resolves #102 TODO: There's still a mention of updateSelfLater Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lawrence-forooghian
force-pushed
the
102-simplify-subscription-storage
branch
from
83ea597 to
9e48ef3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Removes the complicated
updateSelfLatermutable state pattern and just wraps aEventEmitterclass instead. No public API changes.Resolves #102.