-
-
Notifications
You must be signed in to change notification settings - Fork 17
Allow keeping Atoms alive within a scope until it's deallocation #188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for retaining atoms for the lifetime of a view scope, introducing new scope tracking structures and updating cache/subscription management to use them.
- Replace
initializedScope
withscopeValues
inAtomCache
and adapt initializer calls. - Add
ScopeValues
,ScopeState
, andscopes
inStoreState
to track active scopes and unregister atoms when scopes deallocate. - Update
StoreContext
to register/unregister scopes, usestate.subscribed
instead of the old graph field for subscriptions, and adjust related logic. - Revise tests and documentation to reflect the new scope-based keep-alive behavior.
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
Tests/AtomsTests/Utilities/Utilities.swift | Renamed initializer argument from initializedScope to scopeValues . |
Tests/AtomsTests/Core/StoreContextTests.swift | Adjusted assertions to use store.state.subscribed . |
Tests/AtomsTests/Core/ScopeKeyTests.swift | Added @MainActor to testDescription . |
Tests/AtomsTests/Core/AtomCacheTests.swift | Updated test to use scopeValues in AtomCache initializer. |
Tests/AtomsTests/Attribute/KeepAliveTests.swift | Expanded KeepAlive tests to cover scoped and overridden scopes. |
Sources/Atoms/Core/StoreState.swift | Added subscribed and scopes dictionaries. |
Sources/Atoms/Core/StoreContext.swift | Refactored to use ScopeValues , register/unregister scopes, and switch to state.subscribed . |
Sources/Atoms/Core/ScopeValues.swift | Introduced ScopeValues to capture scope parameters. |
Sources/Atoms/Core/ScopeState.swift | Added ScopeState class to automatically unregister on deinit. |
Sources/Atoms/Core/ScopeKey.swift | Made Token actor-isolated with @MainActor . |
Sources/Atoms/Core/Scope.swift | Reduced Scope to only track atom keys. |
Sources/Atoms/Core/Graph.swift | Removed deprecated subscribed field. |
Sources/Atoms/Core/AtomKey.swift | Exposed scopeKey to public, removed isScoped . |
Sources/Atoms/Core/AtomCache.swift | Replaced initializedScope with scopeValues , added shouldKeepAlive . |
Sources/Atoms/Attribute/KeepAlive.swift | Clarified documentation for scoped/override retention behavior. |
Sources/Atoms/AtomScope.swift | Updated AtomScope to register new scopes via ScopeState . |
README.md | Added scoped retention note under KeepAlive . |
@Environment(\.store) | ||
private var environmentStore | ||
private var store |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The store
property is missing its @Environment(\.store)
property wrapper, so it will never be injected and will always be nil. It should be declared as @Environment(\.store) private var store
.
Copilot uses AI. Check for mistakes.
scopedStore?.registerScope(state: state.scopeState) | ||
|
||
return content.environment(\.store, scopedStore) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Registering a scope via side effects directly in the SwiftUI View body can lead to unpredictable behaviors. Consider moving registerScope
to an onAppear
modifier or the view initializer to avoid side effects during view recomputation.
scopedStore?.registerScope(state: state.scopeState) | |
return content.environment(\.store, scopedStore) | |
return content | |
.environment(\.store, scopedStore) | |
.onAppear { | |
scopedStore?.registerScope(state: state.scopeState) | |
} |
Copilot uses AI. Check for mistakes.
Pull Request Type
Issue for this PR
Link: #165