-
Notifications
You must be signed in to change notification settings - Fork 47
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
feat: nested observable state stores #333
Conversation
@@ -113,6 +113,20 @@ public func _$isIdentityEqual<T: ObservableState>( | |||
lhs._$id == rhs._$id | |||
} | |||
|
|||
@inlinable | |||
public func _$isIdentityEqual<T: ObservableState>( |
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.
This addition is not strictly needed for scoping, but makes the optional case much more efficient when going from nil to nil.
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.
Is it worth moving this to a file outside of the Derived
folder? It'd probably make diffs between this and the TCA ObservableState easier.
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.
🤷 previously I was asked to put it in this folder for the same reason.
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.
I just mean this function specifically (if it's not from the upstream repo)
|
||
withPerceptionTracking { | ||
_ = state.optional | ||
} onChange: { | ||
optionalDidChange.fulfill() | ||
XCTFail("Optional should not change") |
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.
result of the new _$isIdentityEqual
overload
Package.resolved
Outdated
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 Tuist setup means this file at best redundant and at worst completely out of sync. (I can pull this into a separate PR if it's controversial.)
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.
Nice addition, surprisingly easy.
I was wondering if it would be possible to create a purple warning when folks try to do this the wrong way. Since we've already had people try to reach for ForeEach(store.children)
substates, we'll definitely want to encourage them to use scoped stores (and aren't likely to catch 100% of people who don't!). A runtime warning (like you get when you forget WithPerceptionTracking
) seems like the easiest way to go (or an assert
if you're feeling especially punitive, I suppose)
It might be, but it's a little tricky. I'm not sure we can do it reliably, and there are cases where it's still OK to access nested state without a store. I think I want to wait and see how usage develops first. |
WithPerceptionTracking { | ||
VStack { | ||
ForEach(store.scope(collection: \.counters)) { counter in | ||
@Perception.Bindable var counter = counter |
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.
I'm a little surprised this works – maybe just stable because counters isn't changing?
Is it possible to have the scope collection method vend a bindable variable, or does that not work?
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.
I'm a little surprised this works – maybe just stable because counters isn't changing?
You mean the shadowing, or the fact that we can wrap it at all? The pattern for a local @Bindable
actually comes straight from Apple.
Is it possible to have the scope collection method vend a bindable variable, or does that not work?
That won't work, or more accurately, it's uglier, because you'd lose the property wrapper magic and have to call into counter.wrappedValue
or counter.projectedValue
manually.
ForEach(store.scope(collection: \.counters)) { counter in | ||
@Perception.Bindable var counter = counter | ||
|
||
WithPerceptionTracking { |
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.
Does the nested WithPerceptionTracking
here do something?
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.
Trying this out locally, removing WithPerceptionTracking
triggers those Perception runtime warnings. I think this is because of the binding to the child store being used inside the ForEach
escaping closure.
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.
Yep, ForEach
is a case that requires nested WithPerceptionTracking
.
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.
It’s great to have a working solution to this one. The sample app provides good context, too.
@@ -0,0 +1,23 @@ | |||
import SwiftUI |
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.
commenting here primarily for threading purposes, but wondering if this issue is specific to our setup, or can something analogous be replicated with 'vanilla' SwiftUI?
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.
Specific to our setup. There may be some assumptions built around the state being a reference type that we're running into.
public func scope<Substate: ObservableState>( | ||
keyPath: WritableKeyPath<Model.State, Substate> | ||
) -> Store<StateAccessor<Substate>> { |
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.
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.
Not that I'm aware of, unfortunately. I'm pleasantly surprised that the error message is decent.
WorkflowSwiftUI/Sources/Store.swift
Outdated
/// Derives a collection of stores from an `IdentifiedArray` of child models. | ||
/// | ||
/// Stores in the returned collection are lazy created on first access, and invalidated when | ||
/// the ID is no longer present in the collection. Reorders do not invalidate. |
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.
Reorders do not invalidate.
Is it worth expanding on this a bit? I'm not sure I understand the ramifications.
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.
Hmm, honestly maybe I shouldn't mention store invalidation in the API docs, as that's sort of an implementation detail. What's more important to consumers is knowing how mutations to individual elements will affect access to other elements.
The tl;dr is that on every render we invalidate child stores that are no longer connected to state in case SwiftUI decides to send a value to a stale binding.
// mutate element | ||
do { | ||
var state = State(array: [.init()]) | ||
let model = StateAccessor(state: state) { update in | ||
update(&state) | ||
} | ||
let (store, _) = Store.make(model: model) | ||
|
||
withPerceptionTracking { | ||
_ = store.scope(collection: \.array) | ||
} onChange: { | ||
XCTFail("Array should not change") | ||
} | ||
|
||
withPerceptionTracking { | ||
_ = store.scope(collection: \.array)[0] | ||
} onChange: { | ||
XCTFail("Array element 0 should not change") | ||
} | ||
|
||
let array0NameDidChange = expectation(description: "array[0].didChange") | ||
withPerceptionTracking { | ||
_ = store.scope(collection: \.array)[0].name | ||
} onChange: { | ||
array0NameDidChange.fulfill() | ||
} | ||
|
||
state.array[0].name = "test" | ||
|
||
await fulfillment(of: [array0NameDidChange], timeout: 0) | ||
} |
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.
🥳 Awesome. Thanks for the great test coverage.
@@ -113,6 +113,20 @@ public func _$isIdentityEqual<T: ObservableState>( | |||
lhs._$id == rhs._$id | |||
} | |||
|
|||
@inlinable | |||
public func _$isIdentityEqual<T: ObservableState>( |
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.
Is it worth moving this to a file outside of the Derived
folder? It'd probably make diffs between this and the TCA ObservableState easier.
This PR adds new scoping methods to
Store
that allow you to create child stores from nested observable state.These same scoping methods already exist for child models, typically created by rendering child workflows and composing their renderings, but not for nested state within a single workflow.
I've added a new
ObservableCounter
sample to demonstrate. The existingObservableScreen
sample has been renamed toObservableComposition
.Example
Consider a state like this:
If one was trying to iterate over
children
and form bindings to thefoo
property, you might attempt this construction, which creates aBinding<[ChildState]>
and then usesForEach
to map over individualBinding<ChildState>
s.Unfortunately this will compile and run, but writes to
child.foo
will not causeFooView
to be invalidated. The exact mechanics behind this are opaque, but the bindings captured by theForEach
don't register observation at the right scope.Using the new scoping methods, you can iterate over child stores, and create a binding from each store instead.
Checklist