-
Notifications
You must be signed in to change notification settings - Fork 47
[feat]: Add support for automatic ViewEnvironment bridging in WorkflowUI #211
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
Changes from 6 commits
b4c43b4
f1307be
dac25a9
dfc680d
2671db3
34595b1
f960ead
2b7cf56
925614c
3e9675c
a5b9e72
e437c72
05f0265
a9c6736
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,10 +18,13 @@ | |
|
||
import ReactiveSwift | ||
import UIKit | ||
@_spi(ViewEnvironmentWiring) import ViewEnvironmentUI | ||
import Workflow | ||
|
||
/// Drives view controllers from a root Workflow. | ||
public final class WorkflowHostingController<ScreenType, Output>: UIViewController where ScreenType: Screen { | ||
public typealias CustomizeEnvironment = (inout ViewEnvironment) -> Void | ||
|
||
/// Emits output events from the bound workflow. | ||
public var output: Signal<Output, Never> { | ||
return workflowHost.output | ||
|
@@ -33,31 +36,38 @@ public final class WorkflowHostingController<ScreenType, Output>: UIViewControll | |
|
||
private let (lifetime, token) = Lifetime.make() | ||
|
||
public var rootViewEnvironment: ViewEnvironment { | ||
didSet { | ||
update(screen: workflowHost.rendering.value, environment: rootViewEnvironment) | ||
} | ||
public var customizeEnvironment: CustomizeEnvironment { | ||
n8chur marked this conversation as resolved.
Show resolved
Hide resolved
|
||
didSet { setNeedsEnvironmentUpdate() } | ||
} | ||
|
||
public init<W: AnyWorkflowConvertible>( | ||
workflow: W, | ||
rootViewEnvironment: ViewEnvironment = .empty, | ||
customizeEnvironment: @escaping CustomizeEnvironment = { _ in }, | ||
observers: [WorkflowObserver] = [] | ||
) where W.Rendering == ScreenType, W.Output == Output { | ||
self.workflowHost = WorkflowHost( | ||
workflow: workflow.asAnyWorkflow(), | ||
observers: observers | ||
) | ||
|
||
self.customizeEnvironment = customizeEnvironment | ||
|
||
var customizedEnvironment: ViewEnvironment = .empty | ||
customizeEnvironment(&customizedEnvironment) | ||
|
||
self.rootViewController = workflowHost | ||
.rendering | ||
.value | ||
.buildViewController(in: rootViewEnvironment) | ||
|
||
self.rootViewEnvironment = rootViewEnvironment | ||
.viewControllerDescription(environment: customizedEnvironment) | ||
.buildViewController() | ||
|
||
super.init(nibName: nil, bundle: nil) | ||
|
||
// Do not automatically forward environment did change notifications to the rendered screen's backing view | ||
// controller. Instead rely on `ViewControllerDescription` to call `setNeedsEnvironmentUpdate()` when updates | ||
// occur. | ||
environmentDescendantsOverride = { [] } | ||
|
||
addChild(rootViewController) | ||
rootViewController.didMove(toParent: self) | ||
|
||
|
@@ -68,7 +78,7 @@ public final class WorkflowHostingController<ScreenType, Output>: UIViewControll | |
.observeValues { [weak self] screen in | ||
guard let self = self else { return } | ||
|
||
self.update(screen: screen, environment: self.rootViewEnvironment) | ||
self.update(screen: screen, environment: self.environment) | ||
} | ||
} | ||
|
||
|
@@ -82,8 +92,17 @@ public final class WorkflowHostingController<ScreenType, Output>: UIViewControll | |
} | ||
|
||
private func update(screen: ScreenType, environment: ViewEnvironment) { | ||
let previousRoot = rootViewController | ||
|
||
update(child: \.rootViewController, with: screen, in: environment) | ||
|
||
if previousRoot !== rootViewController { | ||
// If a new view controller was instantiated and added as a child we need to inform it that the environment | ||
// should be re-requested in order to respond to customizations in this WorkflowHostingController or any | ||
// view controller above it in the UIViewController hierarchy. | ||
setNeedsEnvironmentUpdate() | ||
} | ||
|
||
updatePreferredContentSizeIfNeeded() | ||
} | ||
|
||
|
@@ -98,6 +117,11 @@ public final class WorkflowHostingController<ScreenType, Output>: UIViewControll | |
updatePreferredContentSizeIfNeeded() | ||
} | ||
|
||
public override func viewWillLayoutSubviews() { | ||
super.viewWillLayoutSubviews() | ||
applyEnvironmentIfNeeded() | ||
} | ||
|
||
override public func viewDidLayoutSubviews() { | ||
super.viewDidLayoutSubviews() | ||
rootViewController.view.frame = view.bounds | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unrelated but we should move this to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm fairly confident we should but I was nervous to make this change as part of this work. Let me know if you think it's worth doing now! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nah, it's less risky to isolate that to a separate change. Just wanted to flag it. |
||
|
@@ -150,4 +174,14 @@ public final class WorkflowHostingController<ScreenType, Output>: UIViewControll | |
} | ||
} | ||
|
||
extension WorkflowHostingController: ViewEnvironmentObserving { | ||
public func customize(environment: inout ViewEnvironment) { | ||
customizeEnvironment(&environment) | ||
} | ||
|
||
public func environmentDidChange() { | ||
update(screen: workflowHost.rendering.value, environment: environment) | ||
} | ||
} | ||
|
||
#endif |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,2 @@ | ||
@_exported import ViewEnvironment | ||
@_exported import ViewEnvironmentUI |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,8 @@ | |
#if canImport(UIKit) | ||
|
||
import UIKit | ||
import ViewEnvironment | ||
@_spi(ViewEnvironmentWiring) import ViewEnvironmentUI | ||
|
||
/// Generic base class that can be subclassed in order to to define a UI implementation that is powered by the | ||
/// given screen type. | ||
|
@@ -25,7 +27,7 @@ import UIKit | |
/// ``` | ||
/// struct MyScreen: Screen { | ||
/// func viewControllerDescription(environment: ViewEnvironment) -> ViewControllerDescription { | ||
/// return MyScreenViewController.description(for: self) | ||
/// return MyScreenViewController.description(for: self, environment: environment) | ||
/// } | ||
/// } | ||
/// | ||
|
@@ -42,11 +44,11 @@ open class ScreenViewController<ScreenType: Screen>: UIViewController { | |
return ScreenType.self | ||
} | ||
|
||
public private(set) final var environment: ViewEnvironment | ||
private var previousEnvironment: ViewEnvironment | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to store this at all? I don't see it being used There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's being used in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. bikeshed: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's already built into the public API as That being said, I don't feel super strongly! Let me know if you do. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
OK, so, my reasoning is that it's not the "previous" environment while it's being stored — it becomes the previous one during an update, when we read a new environment to replace it. Until then, it's just the latest one we observed. Not a big deal though. |
||
|
||
public required init(screen: ScreenType, environment: ViewEnvironment) { | ||
self.screen = screen | ||
self.environment = environment | ||
self.previousEnvironment = environment | ||
super.init(nibName: nil, bundle: nil) | ||
} | ||
|
||
|
@@ -55,11 +57,11 @@ open class ScreenViewController<ScreenType: Screen>: UIViewController { | |
fatalError("init(coder:) has not been implemented") | ||
} | ||
|
||
public final func update(screen: ScreenType, environment: ViewEnvironment) { | ||
public final func update(screen: ScreenType) { | ||
let previousScreen = self.screen | ||
self.screen = screen | ||
let previousEnvironment = self.environment | ||
self.environment = environment | ||
let previousEnvironment = self.previousEnvironment | ||
self.previousEnvironment = environment | ||
screenDidChange(from: previousScreen, previousEnvironment: previousEnvironment) | ||
} | ||
|
||
|
@@ -79,8 +81,9 @@ extension ScreenViewController { | |
ViewControllerDescription( | ||
performInitialUpdate: performInitialUpdate, | ||
type: self, | ||
environment: environment, | ||
build: { self.init(screen: screen, environment: environment) }, | ||
update: { $0.update(screen: screen, environment: environment) } | ||
update: { $0.update(screen: screen) } | ||
) | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,8 @@ | |
#if canImport(UIKit) | ||
|
||
import UIKit | ||
import ViewEnvironment | ||
@_spi(ViewEnvironmentWiring) import ViewEnvironmentUI | ||
|
||
/// A ViewControllerDescription acts as a "recipe" for building and updating a specific `UIViewController`. | ||
/// It describes how to _create_ and later _update_ a given view controller instance, without creating one | ||
|
@@ -49,6 +51,7 @@ public struct ViewControllerDescription { | |
/// type changed. | ||
public let kind: KindIdentifier | ||
|
||
private let environment: ViewEnvironment | ||
private let build: () -> UIViewController | ||
private let update: (UIViewController) -> Void | ||
|
||
|
@@ -59,6 +62,11 @@ public struct ViewControllerDescription { | |
/// - performInitialUpdate: If an initial call to `update(viewController:)` | ||
/// will be performed when the view controller is created. Defaults to `true`. | ||
/// | ||
/// - environment: The `ViewEnvironment` that should be injected above the | ||
/// described view controller for ViewEnvironmentUI environment propagation. | ||
/// This is typically passed in from a `Screen` in its | ||
/// `viewControllerDescription(environment:)` method. | ||
/// | ||
/// - type: The type of view controller produced by this description. | ||
/// Typically, should should be able to omit this parameter, but | ||
/// in cases where type inference has trouble, it’s offered as | ||
|
@@ -70,13 +78,16 @@ public struct ViewControllerDescription { | |
public init<VC: UIViewController>( | ||
performInitialUpdate: Bool = true, | ||
type: VC.Type = VC.self, | ||
environment: ViewEnvironment, | ||
build: @escaping () -> VC, | ||
update: @escaping (VC) -> Void | ||
) { | ||
self.performInitialUpdate = performInitialUpdate | ||
|
||
self.kind = .init(VC.self) | ||
|
||
self.environment = environment | ||
|
||
self.build = build | ||
|
||
self.update = { untypedViewController in | ||
|
@@ -95,7 +106,10 @@ public struct ViewControllerDescription { | |
|
||
if performInitialUpdate { | ||
// Perform an initial update of the built view controller | ||
// Note that this also configures the environment ancestor node. | ||
update(viewController: viewController) | ||
} else { | ||
configureAncestor(for: viewController) | ||
} | ||
|
||
return viewController | ||
|
@@ -126,8 +140,43 @@ public struct ViewControllerDescription { | |
""" | ||
) | ||
|
||
configureAncestor(for: viewController) | ||
|
||
update(viewController) | ||
} | ||
|
||
private func configureAncestor(for viewController: UIViewController) { | ||
n8chur marked this conversation as resolved.
Show resolved
Hide resolved
|
||
guard let ancestorOverride = viewController.environmentAncestorOverride else { | ||
// If no ancestor is currently present establish the initial ancestor override | ||
establishAncestorOverride(for: viewController) | ||
return | ||
} | ||
|
||
let currentAncestor = ancestorOverride() | ||
// Check whether the VC's ancestor was overridden by a ViewControllerDescription. | ||
guard currentAncestor is PropagationNode else { | ||
// Do not override the VC's ancestor if it was overridden by something outside of the | ||
// `ViewControllerDescription`'s management of this node. | ||
// The view controller we're managing, or the container it's contained in, likely needs to manage this in a | ||
// special way. | ||
return | ||
} | ||
|
||
// We must nil this out first or we'll hit an assertion which protects against overriding the ancestor when | ||
// some other system has already attempted to provide an override. | ||
viewController.environmentAncestorOverride = nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alternatively, we could make There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah this is an optimization I was talking through with @bencochran. You think it's worth doing now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I think I'd lean towards just doing it now. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Please re-review if/when you have time 🙏 |
||
establishAncestorOverride(for: viewController) | ||
} | ||
|
||
private func establishAncestorOverride(for viewController: UIViewController) { | ||
n8chur marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let ancestor = PropagationNode( | ||
n8chur marked this conversation as resolved.
Show resolved
Hide resolved
|
||
viewController: viewController, | ||
environment: environment | ||
) | ||
viewController.environmentAncestorOverride = { ancestor } | ||
|
||
ancestor.setNeedsEnvironmentUpdate() | ||
n8chur marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
extension ViewControllerDescription { | ||
|
@@ -168,4 +217,39 @@ extension ViewControllerDescription { | |
} | ||
} | ||
|
||
extension ViewControllerDescription { | ||
fileprivate struct PropagationNode: ViewEnvironmentObserving { | ||
|
||
weak var viewController: UIViewController? | ||
|
||
let environment: ViewEnvironment | ||
|
||
init( | ||
viewController: UIViewController, | ||
environment: ViewEnvironment | ||
) { | ||
self.viewController = viewController | ||
self.environment = environment | ||
} | ||
|
||
var environmentAncestor: ViewEnvironmentPropagating? { nil } | ||
|
||
var environmentDescendants: [ViewEnvironmentPropagating] { | ||
[viewController].compactMap { $0 } | ||
} | ||
|
||
func customize(environment: inout ViewEnvironment) { | ||
environment = self.environment | ||
} | ||
|
||
func setNeedsEnvironmentUpdate() { | ||
setNeedsEnvironmentUpdateOnAppropriateDescendants() | ||
} | ||
|
||
func applyEnvironmentIfNeeded() { | ||
/// `apply(environment:)` is not implemented so do nothing. | ||
} | ||
} | ||
} | ||
|
||
#endif |
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.
should this alias be nested under the generic parent type? the fully-specified spelling will be parametrized by the screen/output, which might be unwieldy. then again, maybe nobody will need to reference it in that manner?
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.
Yeah, I'd be surprised if we ever seen folks referencing the type directly in the context of
WorkflowHostingController
usage.I can move it out if we'd prefer though! There are a few opportunities in Market to re-use this type if it was at the top level. Let me know if you'd prefer we do move it out, and if so, what to name it/where to put it!
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.
your call to relocate/rename if you think it makes sense – just a thought!