-
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 all 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,46 +18,57 @@ | |
|
||
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 | ||
} | ||
|
||
/// An environment customization that will be applied to the environment of the root screen. | ||
public var customizeEnvironment: CustomizeEnvironment { | ||
didSet { setNeedsEnvironmentUpdate() } | ||
} | ||
|
||
private(set) var rootViewController: UIViewController | ||
|
||
private let workflowHost: WorkflowHost<AnyWorkflow<ScreenType, Output>> | ||
|
||
private let (lifetime, token) = Lifetime.make() | ||
|
||
public var rootViewEnvironment: ViewEnvironment { | ||
didSet { | ||
update(screen: workflowHost.rendering.value, environment: rootViewEnvironment) | ||
} | ||
} | ||
|
||
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 +79,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 +93,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 +118,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 +175,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) } | ||
) | ||
} | ||
} | ||
|
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!