Add WorkflowSwiftUIExperimental

[square-tomb]

This adds a WorkflowSwiftUIExperimental module containing a set of core reusable types for implementing Workflow screens using SwiftUI.

These types are experimental and not recommended for use in production, but have reoccurred unchanged in a series prototypes over the last year. They were previously copy-pasted in each prototype, and more recently lived in a WorkflowSwiftUIExperimental repo.

Moving them to this repository will allow future prototypes to be written as branches of Workflow, which may include both new sample code and changes to the WorkflowSwiftUIExperimental and Workflow types themselves.

For now this module will be disallowed as a dependency in Register.

Overview

The public types are:

• SwiftUIScreen, a protocol that is adopted by a Workflow rendering that provides screen content in the form of a SwiftUI View, analogous to BlueprintScreen
• ObservableValue<Value>, an object held by the SwiftUI View through which it receives state from and sends actions to a Workflow, analogous to TCA’s ViewStore or AirBnb’s store

This PR does not include extensions that have appeared in some prototypes, including:

• Equatable action sinks
• Representing two-way bindings at the Screen level
• iOS 17 Observation

[jamieQ]

just a question out of curiosity: if we didn't shadow the isDuplicate value here, would passing the filter via a property have undesirable object graph consequences (assuming the compiler allowed it, as i presume the optionality would preclude that in the current formulation)? i.e. would using a property of the ObservableValue as the filter predicate cause the ObservableValue instance to be retained by the newly-created publisher? and if so, would that be a problem or not?

[square-tomb]

If we wrote

subject.removeDuplicates(by: self.isEquivalent).eraseToAnyPublisher()

then the returned publisher would hold a strong reference to the function isEquivalent but not to self since the parameter of removeDuplicates is not an autoclosure.

I.e., after the line

let x = ObservableValue(…).valuePublisher()

the ObservableValue should be deallocated. Does that sound right to you?

[jamieQ]

ah yes, that makes sense, thanks for walking through it. i was playing around with some sample code as i recall some sort of 'gotcha' when passing instance properties around as closures, and i believe the thing i was thinking of was that if we had something like:

// method on ObservableValue
func isEquivalent(v1: Value,  v2: Value) -> Bool

and then passed that as the filter parameter like

subject
  .removeDupliates(by: isEquivalent(v1:v2:))...

then that pipeline would retain the ObservableValue instance as the 'closure' being passed to the filter is actually an instance method (presumably self is an implicit parameter in that formulation, so has to be retained). anyway, file this away as mostly a curio that may occasionally cause surprises.

[square-tomb]

Yes, I think that is exactly right. I have been bitten on multiple occasions by passing an instance method as an escaping function parameter.

[jamieQ]

🤔 what is going on here? is this like to force some internal SwiftUI system to always treat things as having changed?

[kyleve]

Same question

[square-tomb]

Subscript parameter types are required to be Hashable in order that keypaths can be Hashable.

Since our parameter types are functions, they are not actually equatable, and it is safest for our Hashable wrapper to assume they are never equal.

[kyleve]

How does the read/write tracking that SwiftUI does for environment values work with this one big ol' env block? Eg, since the environment isn't equatable, will it just always assume the value changed and re-render anything that read the environment?

For context, this was a major part of the blueprint caching exploration work, eg see:
https://github.com/square/Blueprint/pull/398/files#diff-d78ef8c9d425f712373581c1f800fb283208265b37b8a7a30145452b3780cbe2
and
https://github.com/square/Blueprint/pull/398/files#diff-e664d2d763ebe3ca35c985ddc39c338eff03d90a5dd5f6d90091b7612d02ce4a

[square-tomb]

Yes, I expect that if a view's body accesses any part of viewEnvironment, that body will be unnecessarily reevaluated for any change to any other part of the ViewEnvironment. It's also cumbersome that the View has to write out e.g. @Environment(\.viewEnvironment.marketStylesheet).

I think the next step is to explore mapping ViewEnvironment keys to individual SwiftUI Environment keys, perhaps using UITraitBridgedEnvironmentKey as @n8chur has suggested.

[n8chur]

Rob and I did some experimentation on the @Environment property wrapper and found that an EnvironmentKey observing view will not invalidate if the subset of the value your property wrapper points to (via KeyPath) is unchanged (using what behaves like the same magic comparison that SwiftUI uses for determining changes in other systems).

For example, if you have an Environment value with two parameters in an Environment key:

struct MyThing {
    var foo = 0
    var bar = 0
}
struct MyKey: EnvironmentKey {
    static let defaultValue = MyThing()
}
extension EnvironmentValues {
    var myThing: MyThing {
        get { self[MyKey.self] }
        set { self[MyKey.self] = newValue }
    }
}

and you have a view that observes \.myThing.bar:

struct MyObservingView: View {
    @Environment(\.myThing.bar) var value
    var body: some View {
        let _ = Self._printChanges()
        Text("Bar: \(String(describing: value))")
    }
}

contained within another view that sets \.myValue.foo:

struct MyContainerView: View {
    @State var foo = 0
    var body: some View {
        VStack {
            HStack {
                Stepper("Foo", value: $foo)
                Text("\(String(describing: foo))")
            }
            MyObservingView()
        }
        .environment(\.myThing.foo, foo)
    }
}

Changes to \.myValue.foo do not cause the body of views observing the \.myValue.bar KeyPath (MyObservingView) to be re-evaluated.

Full code for playground

import SwiftUI


struct MyThing {
    var foo = 0
    var bar = 0
}


struct MyKey: EnvironmentKey {
    static let defaultValue = MyThing()
}


extension EnvironmentValues {
    var myThing: MyThing {
        get { self[MyKey.self] }
        set { self[MyKey.self] = newValue }
    }
}


struct MyObservingView: View {
    @Environment(\.myThing.bar) var value
    var body: some View {
        let _ = Self._printChanges()
        Text("Bar: \(String(describing: value))")
    }
}

struct MyContainerView: View {
    @State var foo = 0
    var body: some View {
        VStack {
            HStack {
                Stepper("Foo", value: $foo)
                Text("\(String(describing: foo))")
            }
            MyObservingView()
        }
        .environment(\.myThing.foo, foo)
    }
}


import PlaygroundSupport


PlaygroundPage.current.setLiveView(MyContainerView())

This behavior makes me feel much better about the idea of using the shape proposed here for just composing the environment, but I'm sure there are other benefits to going with the bridged key approach still. This is, for example, still very easy to naively access in a way that would be non performant:

@Environment(\.viewEnvironment) var viewEnvironment
// ...
// I think in some cases this type of access would cause body invalidations if _anything_ on the ViewEnvironment changed?
.someModifier(value: viewEnvironment.some.long.keyPath)

// instead of:
// This should only invalidate the body if the value at the keyPath changes
@Environment(\.viewEnvironment.some.long.keyPath) var value

If the bridging key approach doesn't work out, perhaps an incremental improvement over what we have now would be to just not expose the viewEnvironment key directly and instead require access via another property wrapper which wraps it (e.g. @ViewEnvironment)?

[n8chur]

E.g. we could hide the viewEnvironmnt EnvironmentValues access behind a property wrapper / view modifier set like this:

@propertyWrapper
public struct ViewEnvironmentReader<Value>: DynamicProperty {
    @Environment
    public var wrappedValue: Value

    public init(_ keyPath: KeyPath<ViewEnvironment, Value>) {
        _wrappedValue = Environment((\EnvironmentValues.viewEnvironment).appending(path: keyPath))
    }
}

extension View {

    public func viewEnvironment<Value>(_ keyPath: WritableKeyPath<ViewEnvironment, Value>, _ value: Value) -> some View {
        environment(
            (\EnvironmentValues.viewEnvironment).appending(path: keyPath),
            value
        )
    }
}

[kyleve]

Same question

[kyleve]

Should we have keyPath versions of these functions too?

[square-tomb]

Would that be to enable writing model.scope(\.property) rather than model.scope { $0.property }?

We get that for free since Swift 5.2 due to SE-0249.

[kyleve]

Where can I read about why this is a static? I don't love this, honestly... The ergonomics feel off to me. I guess so you can force folks to use model for property reading tracking?

[square-tomb]

It's possible for this to be an instance method if we are directly mapping Screen values to View values. However, it's not clear how it can be an instance method when we want to use @ObservedObject. See the rest of that doc for why we've favored the latter so far.

However, I think we should take another pass at this soon. You're not the first to be repulsed by it, and some of the rationale may be stale in iOS 15.

[square-tomb]

I took another stab at conforming the rendering directly to View, eliminating the use of ObservableObject. It simplifies the implementation and the developer interface quite a bit.

In iOS 16 at least, I'm not seeing the animation issues (e.g. in Toggle) that we observed at the time of the design doc. This approach might lead to more view body evaluations, but after my performance noodling last month, I think those evaluations are cheap as long as they don't trigger additional work in the Core Animation commit phase.

I'll dig more into this next week! cc @n8chur @watt

[square-tomb]

Opening a draft PR from that branch: #253

[kyleve]

Almost no screens are going to be equatable by themselves – what's the performance cost of not doing this?

[square-tomb]

Many screens become equatable if we replace their callback functions with an equatable action sink.

But I'm still not sure that we should be trying to remove duplicates at the ObservableValue level rather than rely on SwiftUI's internal comparison of View values. I would consider the isDuplicate functionality subject to future removal in ObservableValue and especially in SwiftUIScreen.

[kyleve]

For my own understanding, this is so we can basically isolate each VC into its own SwiftUI hierarchy?

[square-tomb]

I think of it as simply: we need some view controller class for SwiftUIScreen's viewControllerDescription to describe, and that view controller must expose an interface for the description to update with both the ViewEnvironment and screen-specific data.

[square-tomb]

We’re actively exploring significant changes like exposing ViewEnvironment values differently and not using ObservableObject, but merging this now as a baseline after consulting UIS folks.