From 1a74875f51a5395402c27ae2891de0e3d4d62fe4 Mon Sep 17 00:00:00 2001 From: Max Desiatov Date: Fri, 14 Aug 2020 22:37:25 +0100 Subject: [PATCH 1/3] Fix `NavigationView` broken state after re-render --- .../MountedCompositeElement.swift | 16 +++++++++++- Sources/TokamakCore/StackReconciler.swift | 26 +++++++++++-------- .../TokamakCore/State/ObservedObject.swift | 3 ++- 3 files changed, 32 insertions(+), 13 deletions(-) diff --git a/Sources/TokamakCore/MountedViews/MountedCompositeElement.swift b/Sources/TokamakCore/MountedViews/MountedCompositeElement.swift index bfcc4bd97..00b515611 100644 --- a/Sources/TokamakCore/MountedViews/MountedCompositeElement.swift +++ b/Sources/TokamakCore/MountedViews/MountedCompositeElement.swift @@ -20,8 +20,22 @@ import CombineShim class MountedCompositeElement: MountedElement { let parentTarget: R.TargetType + /** An array that stores type-erased values captured with the `@State` property wrappers used + in declarations of this element. + */ var state = [Any]() - var subscriptions = [AnyCancellable]() + + /** An array that stores subscriptions to updates on `@ObservableObject` property wrappers used + in declarations of this element. These subscriptions are transient and may be cleaned up on + every re-render of this composite element. + */ + var transientSubscriptions = [AnyCancellable]() + + /** An array that stores subscriptions to updates on `@StateObject` property wrappers and renderer + observers. These subscriptions are persistent and are only cleaned up when this composite + element is deallocated. + */ + var persistentSubscriptions = [AnyCancellable]() init(_ app: A, _ parentTarget: R.TargetType, _ environmentValues: EnvironmentValues) { self.parentTarget = parentTarget diff --git a/Sources/TokamakCore/StackReconciler.swift b/Sources/TokamakCore/StackReconciler.swift index 624e3505d..65c0751d9 100644 --- a/Sources/TokamakCore/StackReconciler.swift +++ b/Sources/TokamakCore/StackReconciler.swift @@ -94,8 +94,8 @@ public final class StackReconciler { rootElement.mount(with: self) if let mountedApp = rootElement as? MountedApp { - setupSubscription(for: app._phasePublisher, to: \.scenePhase, of: mountedApp) - setupSubscription(for: app._colorSchemePublisher, to: \.colorScheme, of: mountedApp) + setupPersistentSubscription(for: app._phasePublisher, to: \.scenePhase, of: mountedApp) + setupPersistentSubscription(for: app._colorSchemePublisher, to: \.colorScheme, of: mountedApp) } } @@ -153,7 +153,7 @@ public final class StackReconciler { try! property.set(value: state, on: &compositeElement[keyPath: bodyKeypath]) } - private func setupSubscription( + private func setupTransientSubscription( for property: PropertyInfo, of compositeElement: MountedCompositeElement, body bodyKeypath: KeyPath, Any> @@ -165,12 +165,16 @@ public final class StackReconciler { ) as! ObservedProperty // swiftlint:enable force_cast - observed.objectWillChange.sink { [weak self] _ in - self?.queueUpdate(for: compositeElement) - }.store(in: &compositeElement.subscriptions) + // break the reference cycle here as subscriptions are stored in the `compositeElement` + // instance property + observed.objectWillChange.sink { [weak self, weak compositeElement] _ in + if let compositeElement = compositeElement { + self?.queueUpdate(for: compositeElement) + } + }.store(in: &compositeElement.transientSubscriptions) } - private func setupSubscription( + private func setupPersistentSubscription( for publisher: AnyPublisher, to keyPath: WritableKeyPath, of mountedApp: MountedApp @@ -183,7 +187,7 @@ public final class StackReconciler { mountedApp.environmentValues[keyPath: keyPath] = value self?.queueUpdate(for: mountedApp) - }.store(in: &mountedApp.subscriptions) + }.store(in: &mountedApp.persistentSubscriptions) } func render( @@ -199,14 +203,14 @@ public final class StackReconciler { source: &compositeElement[keyPath: bodyKeypath] ) - let needsSubscriptions = compositeElement.subscriptions.isEmpty + compositeElement.transientSubscriptions = [] for property in dynamicProps { // Setup state/subscriptions if property.type is ValueStorage.Type { setupState(id: stateIdx, for: property, of: compositeElement, body: bodyKeypath) stateIdx += 1 - } else if needsSubscriptions && property.type is ObservedProperty.Type { - setupSubscription(for: property, of: compositeElement, body: bodyKeypath) + } else if property.type is ObservedProperty.Type { + setupTransientSubscription(for: property, of: compositeElement, body: bodyKeypath) } } diff --git a/Sources/TokamakCore/State/ObservedObject.swift b/Sources/TokamakCore/State/ObservedObject.swift index 17b6af6fb..4cdb2e3dc 100644 --- a/Sources/TokamakCore/State/ObservedObject.swift +++ b/Sources/TokamakCore/State/ObservedObject.swift @@ -32,7 +32,8 @@ public struct ObservedObject: DynamicProperty where ObjectType: Obse .init( get: { self.root[keyPath: keyPath] - }, set: { + }, + set: { self.root[keyPath: keyPath] = $0 } ) From e63cb4841fce4bae068237beab070893f7a5a239 Mon Sep 17 00:00:00 2001 From: Max Desiatov Date: Sat, 15 Aug 2020 14:46:06 +0100 Subject: [PATCH 2/3] Implement `StateObject` property wrapper --- Sources/TokamakCore/State/StateObject.swift | 31 ++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/Sources/TokamakCore/State/StateObject.swift b/Sources/TokamakCore/State/StateObject.swift index 07245557f..b6a57cb98 100644 --- a/Sources/TokamakCore/State/StateObject.swift +++ b/Sources/TokamakCore/State/StateObject.swift @@ -12,4 +12,33 @@ // See the License for the specific language governing permissions and // limitations under the License. -public typealias StateObject = ObservedObject +import CombineShim + +@propertyWrapper +public struct StateObject: DynamicProperty { + enum Storage { + case initially(() -> ObjectType) + case object(ObservedObject) + } + + var storage: Storage + + public var wrappedValue: ObjectType { projectedValue.root } + + public init(wrappedValue thunk: @autoclosure @escaping () -> ObjectType) { + storage = .initially(thunk) + } + + public var projectedValue: ObservedObject.Wrapper { + switch storage { + case let .object(observed): return observed.projectedValue + default: fatalError() + } + } + + var objectWillChange: AnyPublisher<(), Never> { + wrappedValue.objectWillChange.map { _ in }.eraseToAnyPublisher() + } +} + +extension StateObject: ObservedProperty {} From 3f422b612ecffe92650834a2e0580027482f2b4b Mon Sep 17 00:00:00 2001 From: Max Desiatov Date: Sun, 16 Aug 2020 20:24:27 +0100 Subject: [PATCH 3/3] Finish `@StateObject`, fix `NavigationView` bug --- .../MountedCompositeElement.swift | 6 ++-- Sources/TokamakCore/StackReconciler.swift | 32 +++++++++++-------- .../TokamakCore/State/ObservedObject.swift | 4 +-- Sources/TokamakCore/State/State.swift | 7 ++-- Sources/TokamakCore/State/StateObject.swift | 27 ++++++++-------- .../Views/Navigation/NavigationView.swift | 2 +- Sources/TokamakDOM/Core.swift | 1 + 7 files changed, 44 insertions(+), 35 deletions(-) diff --git a/Sources/TokamakCore/MountedViews/MountedCompositeElement.swift b/Sources/TokamakCore/MountedViews/MountedCompositeElement.swift index 00b515611..907258e63 100644 --- a/Sources/TokamakCore/MountedViews/MountedCompositeElement.swift +++ b/Sources/TokamakCore/MountedViews/MountedCompositeElement.swift @@ -20,10 +20,10 @@ import CombineShim class MountedCompositeElement: MountedElement { let parentTarget: R.TargetType - /** An array that stores type-erased values captured with the `@State` property wrappers used - in declarations of this element. + /** An array that stores type-erased values captured with the `@State` and `@StateObject` property + wrappers used in declarations of this element. */ - var state = [Any]() + var storage = [Any]() /** An array that stores subscriptions to updates on `@ObservableObject` property wrappers used in declarations of this element. These subscriptions are transient and may be cleaned up on diff --git a/Sources/TokamakCore/StackReconciler.swift b/Sources/TokamakCore/StackReconciler.swift index 65c0751d9..690892402 100644 --- a/Sources/TokamakCore/StackReconciler.swift +++ b/Sources/TokamakCore/StackReconciler.swift @@ -99,12 +99,12 @@ public final class StackReconciler { } } - private func queueStateUpdate( + private func queueStorageUpdate( for mountedElement: MountedCompositeElement, id: Int, updater: (inout Any) -> () ) { - updater(&mountedElement.state[id]) + updater(&mountedElement.storage[id]) queueUpdate(for: mountedElement) } @@ -125,7 +125,7 @@ public final class StackReconciler { queuedRerenders.removeAll() } - private func setupState( + private func setupStorage( id: Int, for property: PropertyInfo, of compositeElement: MountedCompositeElement, @@ -134,23 +134,28 @@ public final class StackReconciler { // swiftlint:disable force_try // `ValueStorage` property already filtered out, so safe to assume the value's type // swiftlint:disable:next force_cast - var state = try! property.get(from: compositeElement[keyPath: bodyKeypath]) as! ValueStorage + var storage = try! property.get(from: compositeElement[keyPath: bodyKeypath]) as! ValueStorage - if compositeElement.state.count == id { - compositeElement.state.append(state.anyInitialValue) + if compositeElement.storage.count == id { + compositeElement.storage.append(storage.anyInitialValue) } - if state.getter == nil || state.setter == nil { - state.getter = { compositeElement.state[id] } + if storage.getter == nil { + storage.getter = { compositeElement.storage[id] } + + guard var writableStorage = storage as? WritableValueStorage else { + return try! property.set(value: storage, on: &compositeElement[keyPath: bodyKeypath]) + } // Avoiding an indirect reference cycle here: this closure can be owned by callbacks // owned by view's target, which is strongly referenced by the reconciler. - state.setter = { [weak self, weak compositeElement] newValue in + writableStorage.setter = { [weak self, weak compositeElement] newValue in guard let element = compositeElement else { return } - self?.queueStateUpdate(for: element, id: id) { $0 = newValue } + self?.queueStorageUpdate(for: element, id: id) { $0 = newValue } } + + try! property.set(value: writableStorage, on: &compositeElement[keyPath: bodyKeypath]) } - try! property.set(value: state, on: &compositeElement[keyPath: bodyKeypath]) } private func setupTransientSubscription( @@ -207,9 +212,10 @@ public final class StackReconciler { for property in dynamicProps { // Setup state/subscriptions if property.type is ValueStorage.Type { - setupState(id: stateIdx, for: property, of: compositeElement, body: bodyKeypath) + setupStorage(id: stateIdx, for: property, of: compositeElement, body: bodyKeypath) stateIdx += 1 - } else if property.type is ObservedProperty.Type { + } + if property.type is ObservedProperty.Type { setupTransientSubscription(for: property, of: compositeElement, body: bodyKeypath) } } diff --git a/Sources/TokamakCore/State/ObservedObject.swift b/Sources/TokamakCore/State/ObservedObject.swift index 4cdb2e3dc..c1a56f2cb 100644 --- a/Sources/TokamakCore/State/ObservedObject.swift +++ b/Sources/TokamakCore/State/ObservedObject.swift @@ -47,10 +47,10 @@ public struct ObservedObject: DynamicProperty where ObjectType: Obse } public let projectedValue: Wrapper +} +extension ObservedObject: ObservedProperty { var objectWillChange: AnyPublisher<(), Never> { wrappedValue.objectWillChange.map { _ in }.eraseToAnyPublisher() } } - -extension ObservedObject: ObservedProperty {} diff --git a/Sources/TokamakCore/State/State.swift b/Sources/TokamakCore/State/State.swift index 5e7bc2c35..edb5d9320 100644 --- a/Sources/TokamakCore/State/State.swift +++ b/Sources/TokamakCore/State/State.swift @@ -16,10 +16,13 @@ // protocol ValueStorage { var getter: (() -> Any)? { get set } - var setter: ((Any) -> ())? { get set } var anyInitialValue: Any { get } } +protocol WritableValueStorage: ValueStorage { + var setter: ((Any) -> ())? { get set } +} + @propertyWrapper public struct State: DynamicProperty { private let initialValue: Value @@ -46,7 +49,7 @@ protocol ValueStorage { } } -extension State: ValueStorage {} +extension State: WritableValueStorage {} extension State where Value: ExpressibleByNilLiteral { @inlinable diff --git a/Sources/TokamakCore/State/StateObject.swift b/Sources/TokamakCore/State/StateObject.swift index b6a57cb98..51aef0a46 100644 --- a/Sources/TokamakCore/State/StateObject.swift +++ b/Sources/TokamakCore/State/StateObject.swift @@ -16,29 +16,28 @@ import CombineShim @propertyWrapper public struct StateObject: DynamicProperty { - enum Storage { - case initially(() -> ObjectType) - case object(ObservedObject) - } - - var storage: Storage + public var wrappedValue: ObjectType { (getter?() as? ObservedObject.Wrapper)?.root ?? initial() } - public var wrappedValue: ObjectType { projectedValue.root } + let initial: () -> ObjectType + var getter: (() -> Any)? - public init(wrappedValue thunk: @autoclosure @escaping () -> ObjectType) { - storage = .initially(thunk) + public init(wrappedValue initial: @autoclosure @escaping () -> ObjectType) { + self.initial = initial } public var projectedValue: ObservedObject.Wrapper { - switch storage { - case let .object(observed): return observed.projectedValue - default: fatalError() - } + getter?() as? ObservedObject.Wrapper ?? ObservedObject.Wrapper(root: initial()) } +} +extension StateObject: ObservedProperty { var objectWillChange: AnyPublisher<(), Never> { wrappedValue.objectWillChange.map { _ in }.eraseToAnyPublisher() } } -extension StateObject: ObservedProperty {} +extension StateObject: ValueStorage { + var anyInitialValue: Any { + ObservedObject.Wrapper(root: initial()) + } +} diff --git a/Sources/TokamakCore/Views/Navigation/NavigationView.swift b/Sources/TokamakCore/Views/Navigation/NavigationView.swift index dd96d4cae..43a2188be 100644 --- a/Sources/TokamakCore/Views/Navigation/NavigationView.swift +++ b/Sources/TokamakCore/Views/Navigation/NavigationView.swift @@ -22,7 +22,7 @@ final class NavigationContext: ObservableObject { public struct NavigationView: View where Content: View { let content: Content - @ObservedObject var context = NavigationContext() + @StateObject var context = NavigationContext() public init(@ViewBuilder content: () -> Content) { self.content = content() diff --git a/Sources/TokamakDOM/Core.swift b/Sources/TokamakDOM/Core.swift index 733eb1323..53a54781d 100644 --- a/Sources/TokamakDOM/Core.swift +++ b/Sources/TokamakDOM/Core.swift @@ -27,6 +27,7 @@ public typealias ObservableObject = TokamakCore.ObservableObject public typealias ObservedObject = TokamakCore.ObservedObject public typealias Published = TokamakCore.Published public typealias State = TokamakCore.State +public typealias StateObject = TokamakCore.StateObject // MARK: Modifiers & Styles