Skip to content

[feat]: add experimental WorkflowUI telemetry hooks #221

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

Merged
merged 9 commits into from
Jun 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions WorkflowUI/Sources/Hosting/WorkflowHostingController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import UIKit
import Workflow

/// Drives view controllers from a root Workflow.
public final class WorkflowHostingController<ScreenType, Output>: UIViewController where ScreenType: Screen {
public final class WorkflowHostingController<ScreenType, Output>: WorkflowUIViewController where ScreenType: Screen {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@square-tomb responding here for threading purposes:

WorkflowSwiftUI contains a type WorkflowHostingViewController that should probably now subclass WorkflowUIViewController, too.

good callouts, but given the current motivations for this interface, i think it's okay to punt on the story with SwiftUI and ignore this for the time being.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder, do we need events fromScreenViewController at all? Maybe the events from DescribedViewController are sufficient if that VC usually maps 1:1 to a screen.

being able to recover the Screen type from ScreenViewController may be of use

If not, and DescribedViewController is often used to display a succession of different screens, we would have to add some events here. Or we could wrap currentViewController in a container WorkflowUIViewController if we don't mind deepening the VC hierarchy.

adding some additional events that are more specific to the logic in some of the VC subtypes may be something we want to do eventually, but for now i thought we'd just start with VC lifecycle information and see how far that gets us.

Copy link
Contributor Author

@jamieQ jamieQ Jun 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@n8chur responding here for threading purposes:

I'm a bit nervous about the reliance on a UIViewController superclass when ViewControllerDescription does not enforce this, but it seems like it may be worth the alternative given our goals!

that's a reasonable point, but i think we can revisit this if it ends up being problematic in practice.

ViewControllerDescriptions not describing a ScreenViewController subclass does seem like a potential footgun here (what @square-tomb pointed out in a previous comment).

that's true, though i'm not sure if that will pose an issue for the initial use of this information. @amorde any concerns with this?

Are we planning on adding documentation that describes in detail how to adhere to these event contract rules once it's out of alpha?

yep, this is a first pass so i have low conviction on what exactly the 'right' or desired contract is.

Could Swift macros help make adding adherence easier in the future without needing to wrap one VC in another?

possibly, though i don't really have a sense of how we could concretely leverage that tool at the moment. i thought macros had to be additive, so i'm not sure we'd be able to change any existing code in client-supplied VCs.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i thought macros had to be additive, so i'm not sure we'd be able to change any existing code in client-supplied VCs.

I dove a little deeper into Swift Macros after my initial comment and I think you're right, it looks like it may not be useful here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kyleve responding here for threading purposes:

ViewControllerDescriptions not describing a ScreenViewController subclass does seem like a potential footgun here

Agreed. I don't think this is something we can/should enforce here, IMO.

could you clarify – you think this is a risk, but not one we should change the contract of ViewControllerDescription to deal with?

public typealias CustomizeEnvironment = (inout ViewEnvironment) -> Void

/// Emits output events from the bound workflow.
Expand Down Expand Up @@ -118,13 +118,13 @@ public final class WorkflowHostingController<ScreenType, Output>: UIViewControll
updatePreferredContentSizeIfNeeded()
}

public override func viewWillLayoutSubviews() {
override public func viewWillLayoutSubviews() {
super.viewWillLayoutSubviews()
applyEnvironmentIfNeeded()
}

override public func viewDidLayoutSubviews() {
super.viewDidLayoutSubviews()
defer { super.viewDidLayoutSubviews() }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change needed for your hooks to work? We were considering moving the frame assignment to viewWillLayoutSubviews as part of the ViewEnvironmentUI feature, but wanted to isolate that change due to potential for side effects in code that was depending on this order of events.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no i don't think this change is necessary. the previous implementation of super.viewDidLayoutSubviews() did nothing (according to the docs), and the new one emits an event. it seems like the event emission should occur after any logic in this method, but i don't think the ordering relative to the frame assignment will matter to any of the envisioned consumers.

if we're potentially going to move the frame assignment out of this method, then perhaps we should leave this proposed change since it will preserve the relative ordering with the new observation events?

rootViewController.view.frame = view.bounds
}

Expand Down
56 changes: 56 additions & 0 deletions WorkflowUI/Sources/Observation/WorkflowUIEvents.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/*
* Copyright 2023 Square Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#if canImport(UIKit)
import Foundation
import UIKit

/// Protocol that describes an observable 'event' that may be emitted from `WorkflowUI`.
@_spi(ExperimentalObservation)
public protocol WorkflowUIEvent {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking for feedback on the general structuring of the event types and related protocols

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Structure looks good to me

var viewController: UIViewController { get }
}

// MARK: ViewController Lifecycle Events

/// Event emitted from a `WorkflowUIViewController`'s `viewWillLayoutSubviews` method.
@_spi(ExperimentalObservation)
public struct ViewWillLayoutSubviewsEvent: WorkflowUIEvent, Equatable {
public let viewController: UIViewController
}

/// Event emitted from a `WorkflowUIViewController`'s `viewDidLayoutSubviews` method.
@_spi(ExperimentalObservation)
public struct ViewDidLayoutSubviewsEvent: WorkflowUIEvent, Equatable {
public let viewController: UIViewController
}

/// Event emitted from a `WorkflowUIViewController`'s `viewWillAppear` method.
@_spi(ExperimentalObservation)
public struct ViewWillAppearEvent: WorkflowUIEvent, Equatable {
public let viewController: UIViewController
public let animated: Bool
public let isFirstAppearance: Bool
}

/// Event emitted from a `WorkflowUIViewController`'s `viewDidAppear` method.
@_spi(ExperimentalObservation)
public struct ViewDidAppearEvent: WorkflowUIEvent, Equatable {
public let viewController: UIViewController
public let animated: Bool
public let isFirstAppearance: Bool
}
#endif
35 changes: 35 additions & 0 deletions WorkflowUI/Sources/Observation/WorkflowUIObserver.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*
* Copyright 2023 Square Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#if canImport(UIKit)
import Foundation

/// Protocol to observe events emitted from WorkflowUI.
/// **N.B. This is currently part of an experimental interface, and may have breaking changes in the future.**
@_spi(ExperimentalObservation)
public protocol WorkflowUIObserver {
func observeEvent<E: WorkflowUIEvent>(_ event: E)
}

// MARK: - Global Observation

@_spi(ExperimentalObservation)
public enum WorkflowUIObservation {
/// The shared `WorkflowUIObserver` instance to which all `WorkflowUIEvent`s will be forwarded.
public static var sharedUIObserver: WorkflowUIObserver?
}

#endif
77 changes: 77 additions & 0 deletions WorkflowUI/Sources/Observation/WorkflowUIViewController.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
/*
* Copyright 2023 Square Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#if canImport(UIKit)
import Foundation
import UIKit

/// Ancestor type from which all ViewControllers in WorkflowUI inherit.
open class WorkflowUIViewController: UIViewController {
/// Set to `true` once `viewDidAppear` has been called
public private(set) final var hasViewAppeared: Bool = false

// MARK: Event Emission

/// Observation event emission point.
/// - Parameter event: The event forwarded to any observers.
@_spi(ExperimentalObservation)
public final func sendObservationEvent<E: WorkflowUIEvent>(
_ event: @autoclosure () -> E
) {
WorkflowUIObservation
.sharedUIObserver?
.observeEvent(event())
}

// MARK: Lifecycle Methods

override open func viewWillAppear(_ animated: Bool) {
sendObservationEvent(ViewWillAppearEvent(
viewController: self,
animated: animated,
isFirstAppearance: !hasViewAppeared
))
super.viewWillAppear(animated)
}

override open func viewDidAppear(_ animated: Bool) {
let isFirstAppearance = !hasViewAppeared
if isFirstAppearance { hasViewAppeared = true }

super.viewDidAppear(animated)

sendObservationEvent(ViewDidAppearEvent(
viewController: self,
animated: animated,
isFirstAppearance: isFirstAppearance
))
}

override open func viewWillLayoutSubviews() {
sendObservationEvent(
ViewWillLayoutSubviewsEvent(viewController: self)
)
super.viewWillLayoutSubviews()
}

override open func viewDidLayoutSubviews() {
super.viewDidLayoutSubviews()
sendObservationEvent(
ViewDidLayoutSubviewsEvent(viewController: self)
)
}
}
#endif
2 changes: 1 addition & 1 deletion WorkflowUI/Sources/Screen/ScreenViewController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ import ViewEnvironment
/// }
/// }
/// ```
open class ScreenViewController<ScreenType: Screen>: UIViewController {
open class ScreenViewController<ScreenType: Screen>: WorkflowUIViewController {
public private(set) final var screen: ScreenType

public final var screenType: Screen.Type {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

import UIKit

public final class DescribedViewController: UIViewController {
public final class DescribedViewController: WorkflowUIViewController {
var currentViewController: UIViewController

public init(description: ViewControllerDescription) {
Expand Down
106 changes: 106 additions & 0 deletions WorkflowUI/Tests/WorkflowUIObservationTestCase.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
/*
* Copyright 2023 Square Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#if canImport(UIKit)
import Combine
import Workflow
import XCTest

@_spi(ExperimentalObservation) import WorkflowUI

open class WorkflowUIObservationTestCase: XCTestCase {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's some scratch work in here, but some ideas for how one could tie into this system and forward events through Combine

var publishingObserver: PublishingObserver!

var observedEvents: [WorkflowUIEvent] = []

private var cancellables: [AnyCancellable] = []

override open func invokeTest() {
publishingObserver = PublishingObserver()
defer { publishingObserver = nil }

// collect all events emitted during test invocation
publishingObserver.subject
.sink { [weak self] event in
self?.observedEvents.append(event)
}
.store(in: &cancellables)

withGlobalObserver(publishingObserver) {
super.invokeTest()
}
}

private func withGlobalObserver(_ globalObserver: WorkflowUIObserver, perform: () -> Void) {
let oldObserver = WorkflowUIObservation.sharedUIObserver
defer {
WorkflowUIObservation.sharedUIObserver = oldObserver
}

WorkflowUIObservation.sharedUIObserver = globalObserver
perform()
}

func observationEvents(
from viewController: WorkflowUIViewController,
perform: () -> Void
) -> [WorkflowUIEvent] {
var events: [WorkflowUIEvent] = []

let scopedObserver = publishingObserver
.publisher
.filter { $0.viewController === viewController }
.sink { events.append($0) }
defer { scopedObserver.cancel() }

perform()

return events
}
}

final class PublishingObserver: WorkflowUIObserver {
let subject: PassthroughSubject<WorkflowUIEvent, Never>
private(set) lazy var publisher = { subject.eraseToAnyPublisher() }()

init() {
self.subject = .init()
}

func observeEvent<E: WorkflowUIEvent>(_ event: E) {
subject.send(event)
}
}

// MARK: Event Introspection Utilities

typealias EventDescriptor = String
extension EventDescriptor {
static var viewWillAppear: EventDescriptor = "\(ViewWillAppearEvent.self)"

static var viewDidAppear: EventDescriptor = "\(ViewDidAppearEvent.self)"

static var viewWillLayoutSubviews: EventDescriptor = "\(ViewWillLayoutSubviewsEvent.self)"

static var viewDidLayoutSubviews: EventDescriptor = "\(ViewDidLayoutSubviewsEvent.self)"
}

extension WorkflowUIEvent {
var descriptor: EventDescriptor {
"\(type(of: self))"
}
}
#endif
Loading