[ECO-5384] Implement memory management pattern similar to ably-cocoa #26
Conversation
github-actions
bot
temporarily deployed
to
staging/pull/26/AblyLiveObjects
lawrence-forooghian
force-pushed
the
9-memory-management
branch
from
3c118c7 to
bc814fb
Compare
github-actions
bot
temporarily deployed
to
staging/pull/26/AblyLiveObjects
See accompanying ably-cocoa change. The additional test dependencies in Package.swift are because I was getting "Undefined symbol: _OBJC_CLASS_$_APDefaultPublicRealtimeChannelUnderlyingObjects" when compiling. Now, not entirely sure _what_ was causing this error, but given that the tests do indeed import these two packages it seems reasonable enough to declare them as dependencies.
lawrence-forooghian
force-pushed
the
9-memory-management
branch
from
bc814fb to
e6dc68b
Compare
github-actions
bot
temporarily deployed
to
staging/pull/26/AblyLiveObjects
lawrence-forooghian
force-pushed
the
9-memory-management
branch
from
e6dc68b to
1754935
Compare
github-actions
bot
temporarily deployed
to
staging/pull/26/AblyLiveObjects
…WithObjects helper Preparation for pulling this out to be used more widely.
Will use in some upcoming tests for object lifetimes.
lawrence-forooghian
changed the title
lawrence-forooghian
changed the title
lawrence-forooghian
force-pushed
the
9-memory-management
branch
from
1754935 to
e66c130
Compare
lawrence-forooghian
changed the title
github-actions
bot
temporarily deployed
to
staging/pull/26/AblyLiveObjects
We adopt a pattern whereby the LiveObjects SDK functions correctly as long as the user is holding a reference to any object vended by the public API of the SDK. We do this by copying the ably-cocoa approach of having separate public and internal versions of objects. All written by me, except for getting Cursor to help with updating the tests in response to the new way of injecting CoreSDK. Resolves #9.
lawrence-forooghian
force-pushed
the
9-memory-management
branch
from
e66c130 to
dcbe617
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis update introduces a comprehensive overhaul of memory management and object lifecycles for Ably LiveObjects in Swift. It removes weak references between core SDK and plugin components, implements strong reference chains from public API objects to internal SDK components, and introduces public proxy wrappers with a shared store to ensure stable public object identity. The change is documented, thoroughly tested (including new lifecycle tests), and reflected in both internal and public-facing APIs. Changes
Sequence Diagram(s)sequenceDiagram
participant UserApp
participant PublicRealtimeObjects
participant PublicObjectsStore
participant InternalRealtimeObjects
participant CoreSDK
participant AblyChannel
participant AblyClient
UserApp->>AblyChannel: Obtain objects property
AblyChannel->>PublicObjectsStore: getOrCreateRealtimeObjects(proxying: InternalRealtimeObjects, args)
PublicObjectsStore->>PublicRealtimeObjects: create or fetch proxy
PublicRealtimeObjects->>InternalRealtimeObjects: delegate API calls
PublicRealtimeObjects->>CoreSDK: delegate API calls
CoreSDK->>AblyChannel: hold strong reference
AblyChannel->>AblyClient: hold strong reference
Note right of PublicObjectsStore: Weak references ensure deallocation when user releases public object
sequenceDiagram
participant UserApp
participant PublicLiveMap
participant PublicObjectsStore
participant InternalLiveMap
participant CoreSDK
UserApp->>PublicRealtimeObjects: getRoot()
PublicRealtimeObjects->>InternalRealtimeObjects: getRoot(coreSDK)
InternalRealtimeObjects->>InternalLiveMap: return root map
PublicRealtimeObjects->>PublicObjectsStore: getOrCreateMap(proxying: InternalLiveMap, args)
PublicObjectsStore->>PublicLiveMap: create or fetch proxy
PublicLiveMap->>InternalLiveMap: delegate API calls
PublicLiveMap->>CoreSDK: pass coreSDK as needed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
CONTRIBUTING.md (1)
59-59: Minor: Add period after "etc" for consistency.In American English, abbreviations like "etc." should include a period.
-that is, the ably-cocoa components that hold a strong reference to these internal components (that is, the realtime client or channel); thus, they must never hold a strong reference to a `CoreSDK` object +that is, the ably-cocoa components that hold a strong reference to these internal components (that is, the realtime client or channel); thus, they must never hold a strong reference to a `CoreSDK` object.Tests/AblyLiveObjectsTests/Helpers/ClientHelper.swift (1)
36-39: Consider adding documentation for PartialClientOptions.The
PartialClientOptionsstruct would benefit from brief documentation explaining its purpose and when each optional parameter should be used.+ /// Partial client options for customizing client behavior in tests. struct PartialClientOptions: Encodable, Hashable { + /// Whether to use binary protocol instead of JSON. var useBinaryProtocol: Bool? + /// Whether the client should automatically connect on creation. var autoConnect: Bool? }Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (1)
85-106: Note: Multiple unimplemented methodsSeveral methods required by the internal API are marked as
notYetImplemented(). This is acceptable for an early implementation phase, but these will need to be implemented for the SDK to be fully functional.Would you like me to open issues to track the implementation of these methods:
increment,decrement,subscribe,unsubscribeAll,on, andoffAll?Tests/AblyLiveObjectsTests/ObjectLifetimesTests.swift (2)
138-138: Inconsistency: Missing autoConnect parameterThe first test explicitly sets
autoConnect: false, but this line doesn't specify it. According to the comment on line 139, you need the connection for this test, but it would be clearer to explicitly setautoConnect: truehere for consistency.- let realtime = try await ClientHelper.realtimeWithObjects() + let realtime = try await ClientHelper.realtimeWithObjects(options: .init(autoConnect: true))
71-71: Track TODOs for comprehensive test coverageThe TODO comments indicate important missing test coverage:
- Testing event reception on LiveObjects
- Testing with non-root LiveMap and counter objects
These are important for validating the complete memory management pattern.
Would you like me to create GitHub issues to track these TODO items for future implementation?
Also applies to: 188-188, 235-235
Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift (1)
878-879: Consider extracting repeated MockCoreSDK initialization.Multiple test methods create
MockCoreSDK(channelState: .attached)instances. Consider extracting this into a test helper to reduce duplication.+ private static func createAttachedCoreSDK() -> MockCoreSDK { + MockCoreSDK(channelState: .attached) + }Then in the tests:
- let coreSDK = MockCoreSDK(channelState: .attached) + let coreSDK = Self.createAttachedCoreSDK()Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift (1)
36-36: Document or use the unusedclientparameter.The
clientparameter is added but marked as unused with_. If this is for interface compatibility, consider adding a comment explaining why it's needed but unused.- internal func prepare(_ channel: AblyPlugin.RealtimeChannel, client _: AblyPlugin.RealtimeClient) { + // client parameter is required by the LiveObjectsInternalPluginProtocol interface + internal func prepare(_ channel: AblyPlugin.RealtimeChannel, client _: AblyPlugin.RealtimeClient) {Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicObjectsStore.swift (1)
16-17: Consider usingos_unfair_lockfor better performance.
NSLockis a good choice for thread safety, butos_unfair_lockwould provide better performance for the short critical sections in this code.- private let mutex = NSLock() + private let mutex = os_unfair_lock()Note: This would require adjusting the lock usage pattern from
mutex.withLockto the appropriateos_unfair_lockAPIs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
CONTRIBUTING.md(1 hunks)Package.swift(1 hunks)Sources/AblyLiveObjects/Internal/CoreSDK.swift(1 hunks)Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift(1 hunks)Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift(3 hunks)Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift(6 hunks)Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift(5 hunks)Sources/AblyLiveObjects/Internal/InternalLiveMapValue.swift(1 hunks)Sources/AblyLiveObjects/Internal/LiveObjectMutableState.swift(1 hunks)Sources/AblyLiveObjects/Internal/ObjectsPool.swift(6 hunks)Sources/AblyLiveObjects/Public/ARTRealtimeChannel+Objects.swift(1 hunks)Sources/AblyLiveObjects/Public/Plugin.swift(1 hunks)Sources/AblyLiveObjects/Public/Public Proxy Objects/InternalLiveMapValue+ToPublic.swift(1 hunks)Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultLiveCounter.swift(1 hunks)Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultLiveMap.swift(1 hunks)Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultRealtimeObjects.swift(1 hunks)Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicObjectsStore.swift(1 hunks)Sources/AblyLiveObjects/Utility/WeakRef.swift(0 hunks)Tests/AblyLiveObjectsTests/AblyLiveObjectsTests.swift(4 hunks)Tests/AblyLiveObjectsTests/Helpers/ClientHelper.swift(1 hunks)Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift(1 hunks)Tests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swift(19 hunks)Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift(52 hunks)Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift(40 hunks)Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift(6 hunks)Tests/AblyLiveObjectsTests/ObjectLifetimesTests.swift(1 hunks)Tests/AblyLiveObjectsTests/ObjectsPoolTests.swift(18 hunks)ably-cocoa(1 hunks)
💤 Files with no reviewable changes (1)
- Sources/AblyLiveObjects/Utility/WeakRef.swift
🧰 Additional context used
🧬 Code Graph Analysis (6)
Tests/AblyLiveObjectsTests/ObjectLifetimesTests.swift (5)
Tests/AblyLiveObjectsTests/Helpers/ClientHelper.swift (2)
realtimeWithObjects(7-27)channelOptionsWithObjects(30-34)Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (2)
get(96-118)on(194-196)Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultLiveMap.swift (2)
get(25-32)on(91-93)Tests/AblyLiveObjectsTests/Helpers/Ably+Concurrency.swift (1)
attachAsync(6-16)Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultRealtimeObjects.swift (2)
getRoot(23-32)on(54-56)
Sources/AblyLiveObjects/Internal/InternalLiveMapValue.swift (1)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (1)
value(68-83)
Tests/AblyLiveObjectsTests/Helpers/ClientHelper.swift (2)
Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift (2)
realtimeWithObjects(12-14)channelOptionsWithObjects(16-18)Tests/AblyLiveObjectsTests/Helpers/Sandbox.swift (1)
fetchSharedAPIKey(89-91)
Sources/AblyLiveObjects/Internal/ObjectsPool.swift (3)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (4)
entries(141-166)createZeroValued(80-91)replaceData(208-216)replaceData(306-328)Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (3)
createZeroValued(55-64)replaceData(112-116)replaceData(167-181)Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift (2)
objectState(189-205)objectsMap(495-503)
Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicObjectsStore.swift (1)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (1)
value(68-83)
Sources/AblyLiveObjects/Public/Public Proxy Objects/InternalLiveMapValue+ToPublic.swift (1)
Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicObjectsStore.swift (4)
getOrCreateMap(49-53)getOrCreateMap(116-127)getOrCreateCounter(37-41)getOrCreateCounter(104-114)
🪛 LanguageTool
CONTRIBUTING.md
[style] ~50-~50: Consider removing “of” to be more concise
Context: ... similar to that of ably-cocoa: we keep all of the internal components of the SDK alive as...
(ALL_OF_THE)
[style] ~52-~52: Consider removing “of” to be more concise
Context: ...pes which maintain strong references to all of the internal components of the SDK which ar...
(ALL_OF_THE)
[style] ~57-~57: Consider a more concise word here.
Context: ...ts The key rules that must be followed in order to avoid a strong reference cycle are that...
(IN_ORDER_TO_PREMIUM)
[style] ~57-~57: In American English, abbreviations like “etc.” require a period.
Context: ...asses (that is InternalDefaultLiveMap etc) _must never hold a strong reference to...
(ETC_PERIOD)
🔇 Additional comments (53)
ably-cocoa (1)
1-1: Submodule pointer looks fine – please verify the hash is pushed and CI pulls it correctlyThis file only updates the
ably-cocoasubmodule to commit4e0601a4567235a2e5afd36eb19d363057b642da.
Double-check that:
- The referenced commit exists on the remote (not a local‐only WIP).
.gitmoduleshasn’t changed path or branch inadvertently.- CI workflows include
git submodule update --init --recursiveso builds pick up the new revision.No other issues spotted here.
Sources/AblyLiveObjects/Internal/LiveObjectMutableState.swift (1)
5-5: Documentation accurately reflects the internal class renaming.The update correctly changes the class references from
DefaultLiveCounterandDefaultLiveMaptoInternalDefaultLiveCounterandInternalDefaultLiveMap, aligning with the memory management refactor that separates internal and public API classes.Package.swift (1)
59-66: Test dependencies correctly added for the refactored API.Adding "Ably" and "AblyPlugin" dependencies to the test target is appropriate given the introduction of public wrapper classes and internal API changes that the tests need to verify directly.
Sources/AblyLiveObjects/Public/Plugin.swift (2)
26-26: Good centralization of plugin API instance.Introducing the
defaultPluginAPIconstant centralizes plugin API creation, improving testability and making it easier to inject different implementations if needed.
30-30: Consistent usage of centralized plugin API.Using the centralized
defaultPluginAPIconstant instead of direct instantiation maintains consistency and supports the dependency injection pattern.Tests/AblyLiveObjectsTests/AblyLiveObjectsTests.swift (2)
23-23: Type check correctly updated for public wrapper class.The test now correctly expects
PublicDefaultRealtimeObjectsinstead of the internalDefaultRealtimeObjects, reflecting the new public API wrapper pattern.
82-82: Test access properties correctly updated for new architecture.The changes from
testsOnly_internallyTypedObjectstotestsOnly_nonTypeErasedObjectsappropriately reflect the new memory management pattern where public objects wrap internal implementations. The test functionality remains intact while working through the updated public API.Also applies to: 85-85, 93-93, 104-104, 113-113
Tests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swift (2)
13-14: LGTM! Correct adaptation to the new memory management pattern.The test correctly creates the
coreSDKinstance separately and passes it as a parameter to thevalue()method, following the new pattern where internal objects don't hold references to their dependencies.Also applies to: 17-17
272-272: LGTM! Simplified ObjectsPool initialization.The removal of
rootDelegateandrootCoreSDKparameters aligns with the new memory management approach where dependencies are passed explicitly during method calls rather than stored.Also applies to: 273-273
Sources/AblyLiveObjects/Internal/InternalLiveMapValue.swift (2)
4-8: Well-designed internal value representation.The enum correctly encapsulates the different types of values that can be stored in a live map, with appropriate use of
Sendablefor thread safety. This aligns well with the new memory management pattern where internal types don't hold references to dependencies.
36-53: Clean and idiomatic convenience API.The convenience getters provide an elegant way to access specific value types, properly delegating to
PrimitiveObjectValuefor primitive type extraction. The consistent pattern of returningnilfor non-matching cases is appropriate.CONTRIBUTING.md (1)
48-69: Clear and comprehensive memory management documentation.The documentation effectively explains the SDK's memory management approach, including:
- The rationale for keeping internal components alive through strong references
- Clear rules to avoid reference cycles
- The distinction between internal and public types
- References to relevant code and tests
This will be valuable for contributors understanding the architecture.
Tests/AblyLiveObjectsTests/ObjectsPoolTests.swift (2)
12-18: Correct usage of the new internal types.The test properly uses
InternalDefaultLiveMap.createZeroValuedand correctly demonstrates that existing objects in the pool are returned by reference.
251-328: Excellent comprehensive sync scenario test.This test effectively covers the complete sync behavior including:
- Updating existing objects
- Creating new objects
- Removing objects not in sync
- Preserving the root object
The explicit dependency passing pattern is correctly applied throughout.
Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift (3)
13-24: Proper error handling test with explicit dependency injection.The test correctly validates that operations throw appropriate errors when the channel is in DETACHED or FAILED state, properly passing the
MockCoreSDKinstance with the test state.
540-546: Good test of zero-value object creation.The test properly verifies that MAP_SET operations create zero-value objects in the pool when referencing object IDs, which is crucial for the lazy initialization pattern.
579-615: Excellent test verifying RTO6a behavior.This test effectively validates that existing objects in the pool are not replaced when referenced by MAP_SET operations, which is crucial for maintaining object identity and preventing unnecessary recreations.
Tests/AblyLiveObjectsTests/Helpers/ClientHelper.swift (1)
1-40: Well-implemented test helper utility.This new
ClientHelperprovides a clean, reusable interface for creating LiveObjects-enabled clients in integration tests. The implementation correctly handles all necessary configuration including plugin setup, sandbox environment, and optional parameters.Sources/AblyLiveObjects/Internal/CoreSDK.swift (1)
15-27: No strong reference cycles introduced.The switch to strong references in
DefaultCoreSDKis correctly applied and does not create retain cycles:
DefaultCoreSDKnow holds strong references tochannel,client, andpluginAPI(Sources/AblyLiveObjects/Internal/CoreSDK.swift).- Public proxies (e.g.
PublicDefaultLiveMap,PublicDefaultLiveCounter) holdCoreSDKstrongly, with no back-references fromDefaultCoreSDK.- Searches (rg/ast-grep) confirmed no internal component retains
DefaultCoreSDK.All good to merge.
Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultLiveCounter.swift (2)
1-52: Well-implemented public proxy wrapper.This
PublicDefaultLiveCountercorrectly implements the proxy pattern, holding strong references to both the internal implementation andCoreSDKwhile delegating allLiveCounterprotocol methods. The design ensures proper memory management and provides a stable public API.
23-35: No change needed for CoreSDK parameter passingThe internal
InternalDefaultLiveCounterdefines:
func value(coreSDK: CoreSDK) throws -> Doublefunc increment(amount: Double) async throwsfunc decrement(amount: Double) async throwsThe public proxies correctly match those signatures:
proxied.value(coreSDK: coreSDK)proxied.increment(amount: amount)proxied.decrement(amount: amount)There is no inconsistency—increment and decrement intentionally don’t take a
CoreSDKparameter. Please disregard the original suggestion.Likely an incorrect or invalid review comment.
Sources/AblyLiveObjects/Public/ARTRealtimeChannel+Objects.swift (2)
29-32: Good test accessibility with clear naming.The
testsOnly_nonTypeErasedObjectsproperty provides appropriate test access while clearly indicating its test-only purpose through naming conventions.
10-27: Thread safety of PublicObjectsStore confirmedThe
PublicObjectsStoresafely synchronizes access to its mutable state using anNSLockand themutex.withLock { … }pattern in eachgetOrCreate…method. No further changes are needed.
- Internal
mutex = NSLock()withmutableStateprotected bymutex.withLock { … }- Class is marked
Sendableand uses a private lock for all mutationsSources/AblyLiveObjects/Public/Public Proxy Objects/InternalLiveMapValue+ToPublic.swift (2)
1-38: Excellent conversion layer implementation.This extension provides a clean and well-structured mechanism for converting internal live map values to their public counterparts. The implementation correctly:
- Uses
PublicObjectsStorefor consistent proxy object identity- Handles all enum cases appropriately (primitives pass-through, live objects get proxied)
- Encapsulates creation dependencies in a clear
PublicValueCreationArgsstructure- Maintains the memory management guarantees of the new architecture
4-15: Well-designed creation arguments pattern.The
PublicValueCreationArgsstruct with computed properties for specific creation args provides a clean way to pass dependencies while maintaining type safety and avoiding parameter proliferation.Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (2)
5-6: LGTM! Clean separation of internal implementation from public APIThe renaming to
InternalDefaultLiveCounterand change toSendableconformance clearly indicates this is an internal implementation class. This aligns well with the memory management pattern where public proxy objects wrap these internal implementations.
68-83: Good architectural change: Explicit dependency passingConverting from a stored
coreSDKproperty to an explicit parameter improves testability and reduces coupling. The error handling for invalid channel states is preserved correctly.Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultLiveMap.swift (3)
1-22: Well-structured proxy implementation with proper lifecycle managementThe implementation correctly maintains strong references to dependencies (
coreSDKanddelegate) as documented in the comments. This ensures the internal components remain alive while the public object exists, which is essential for the memory management pattern.
6-6: Verify: Should this bepublicinstead ofinternal?The class is in the "Public Proxy Objects" directory but is marked as
internal. Is this intentional for the current implementation phase, or should it bepublicto match its role as a public-facing API?
25-32: Consistent and correct delegation patternAll methods properly delegate to the internal implementation and handle type conversion using
toPublic. The error propagation withthrows(ARTErrorInfo)is handled correctly throughout.Also applies to: 40-54, 63-72
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (3)
4-5: Consistent refactoring: Internal class with explicit dependenciesThe renaming and protocol changes align with the broader refactoring pattern. The class now clearly indicates its internal nature and properly implements
Sendablefor thread safety.
72-78: Simplified initialization without stored dependenciesThe constructor no longer requires
coreSDKor delegate parameters, which simplifies object creation and reduces coupling. TheObjectsPoolis initialized with just the logger.
90-99: Clean parameter passing for channel state validationThe
getRootmethod now receivescoreSDKas a parameter, allowing it to check the channel state without maintaining a stored reference. The error handling for invalid channel states is preserved correctly.Sources/AblyLiveObjects/Internal/ObjectsPool.swift (2)
9-10: Consistent type updates to use internal classesAll type references have been correctly updated from
DefaultLiveMap/DefaultLiveCounterto theirInternalcounterparts. The convenience getters maintain the same functionality with the new types.Also applies to: 13-23, 91-91
85-85: Simplified object creation without dependency injectionThe removal of
delegateandcoreSDKparameters fromcreateZeroValuedcalls aligns with the architectural change where these dependencies are passed explicitly to methods rather than stored in objects. This simplifies the object creation logic.Also applies to: 130-130, 132-132, 177-186
Tests/AblyLiveObjectsTests/ObjectLifetimesTests.swift (2)
6-99: Excellent test coverage for memory management with public objects propertyThis test thoroughly validates the memory management pattern by:
- Using weak references to track object lifetimes
- Ensuring internal dependencies remain alive while public references exist
- Properly handling async deallocation via dispatch queues
- Verifying cleanup after releasing references
The test structure is clear and well-documented.
220-236: Good validation of object identity stabilityThis test confirms that the public objects maintain stable pointer identity across multiple accesses, which is crucial for the caching mechanism in
PublicObjectsStore.Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift (2)
10-14: LGTM! Simplified test helper aligns with the refactor.The removal of the
channelStateparameter is consistent with the architectural change whereInternalDefaultRealtimeObjectsno longer stores channel state internally.
511-514: Explicit dependency injection pattern correctly implemented.The pattern of creating a
MockCoreSDKinstance and passing it explicitly to methods is consistently applied throughout the tests. This aligns well with the documented memory management approach where internal objects don't hold strong references.Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift (2)
12-14: Good consolidation of client helper functions.Delegating to
ClientHelpermethods reduces duplication and centralizes client setup logic.
179-180: Simplified test case initialization with boolean flag.Using the boolean directly instead of constructing intermediate option instances improves readability.
Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift (1)
23-23: Appropriate visibility change for internal architecture.Changing the visibility from private to internal for
realtimeObjectsmethod allows proper access for the new public proxy pattern while maintaining encapsulation.Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultRealtimeObjects.swift (1)
64-65: Good separation of test-only APIs.The clear documentation and naming convention (
testsOnly_prefix) for test-only properties helps prevent accidental usage in production code.Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicObjectsStore.swift (2)
85-89: Efficient cleanup of deallocated entries.The
removeDeallocatedEntriesmethod correctly filters out entries with nil weak references. This prevents memory leaks from accumulating stale dictionary entries.
65-83: Well-implemented get-or-create pattern with proper cleanup.The implementation correctly:
- Cleans up deallocated entries before checking
- Returns existing proxies when available
- Creates and stores new proxies only when needed
- Uses weak references to avoid retain cycles
This ensures stable object identity while respecting user ownership.
Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (8)
10-11: LGTM! Clear documentation and appropriate architectural changes.The rename to
InternalDefaultLiveMapand conformance change toSendableproperly reflect this class's new role as an internal implementation backing the public API.
93-93: Good documentation structure.The section header clearly delineates the internal methods that implement the LiveMap functionality.
95-96: Consistent implementation of explicit dependency injection.The method signature change properly implements the new pattern of passing dependencies explicitly rather than storing them as properties.
Also applies to: 117-117
120-138: Proper conversion from computed property to method.The
sizemethod correctly implements the explicit dependency pattern while preserving all the required business logic and error handling.
141-165: Well-implemented conversion with proper delegate passing.The
entriesmethod correctly implements the new signature pattern and properly passes the delegate parameter toconvertEntryToLiveMapValuefor each entry.
168-176: Efficient delegation pattern for keys and values methods.Both methods correctly leverage the
entriesimplementation, avoiding code duplication while maintaining the proper parameter passing pattern.
455-455: Consistent with simplified object creation pattern.The removal of
mapDelegateandcoreSDKparameters fromcreateZeroValueObjectaligns with the overall memory management improvements.
553-553: Proper explicit dependency injection in helper method.The
convertEntryToLiveMapValuemethod correctly receives and uses the delegate parameter, maintaining functionality while improving memory management.Also applies to: 591-591
Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultRealtimeObjects.swift
Show resolved
Hide resolved
Note: This PR is based on top of #25; please review that one first.
We adopt a pattern whereby the LiveObjects SDK functions correctly as long as the user is holding a reference to any object vended by the public API of the SDK. We do this by copying the ably-cocoa approach of having separate public and internal versions of objects.
See commit messages for more details.
Resolves #9.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Tests
Chores