[feat]: add experimental WorkflowUI telemetry hooks

[jamieQ]

Motivation

we'd like to be able to build out some observation tooling that is aware of certain events that occur within WorkflowUI. specifically, view lifecycle data is of particular interest. currently the options for tying into UIViewController lifecycle events are either:

1. introduce new subclasses in client code that override the methods of interest and provide observation hooks there.
2. swizzle the methods of interest to allow client observation

option 1. may place a large burden on consumers that have many existing subclasses (of, say, ScreenViewController), and only works for the open types defined in WorkflowUI, which excludes WorkflowHostingController & DescribedViewController. option 2. may require less code to implement, but adds indirection (swizzling is often not particularly discoverable), runtime overhead, and may not work for Swift-only methods.

note: this is all currently implemented behind 'experimental' SPI declarations. the intent would be to treat this as sort of an 'alpha' version of this functionality, and to set expectations that it may have breaking changes going forward, without a major version increment in the library.

Implementation

to achieve this goal, we've implemented an approach that does the following:

• unifies all UIViewController subtypes defined in WorkflowUI under a new parent class WorkflowUIViewController
• instruments some view controller lifecycle events (viewDidAppear, etc) within the shared base class
• exposes a static value that can be used to register a 'global' observer of these events
• adds some new protocols and event data types to enable observation of WorkflowUI lifecycle events

Usage Example

the envisioned consumer is something like a shared telemetry system that wishes to observe certain WorkflowUI events and directly use or attach metadata to them. for example, imagine a consumer wished to log an event whenever a ViewController from WorkflowUI appeared on screen. with the proposed changes, this could be accomplished as follows:

// implement an observer
struct AppearanceLogger: WorkflowUIObserver {
    var loggingSystem: LoggingSystem

    // handle all emitted observation events
    func observeEvent<E: WorkflowUIEvent>(_ event: E) {
        // perform specific observation logic
        if let appearanceEvent = event as? ViewDidAppearEvent {
            loggingSystem.logAppearanceEvent(appearanceEvent.viewController)
        }
    }
}

// install the global observer
WorkflowUIObservation.sharedUIObserver = AppearanceLogger()

Checklist

• Unit Tests
• UI Tests
• Snapshot Tests (iOS only)
• I have made corresponding changes to the documentation

[jamieQ]

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

[amorde]

Structure looks good to me

[jamieQ]

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

[mjohnson12]

This looks good to me and flows well with the rest of the WorkflowUI code.

[Kiesco08]

Approach lgtm too, seems easy to use

[watt]

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.

[jamieQ]

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?

[square-tomb]

Looks good to me!

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

Other approaches to SwiftUI support that we're experimenting with involve a view controller type that sublclasses UIHostingController and so cannot subclass WorkflowUIViewController.

However, I think any view controller type like that can be modified to contain, rather than subclass, UIHostingController. The only downside AFAIK would be a deeper VC hierarchy.

[n8chur]

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!

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

Are we planning on adding documentation that describes in detail how to adhere to these event contract rules once it's out of alpha? Could Swift macros help make adding adherence easier in the future without needing to wrap one VC in another? It'd be nice to avoid needing to compose a nested VC to get this behavior.

[kyleve]

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.

[square-tomb]

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.

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.

[jamieQ]

@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.

[jamieQ]

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.

[jamieQ]

@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.

[n8chur]

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.

[jamieQ]

@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?