Skip to content
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

Observable State for WorkflowSwiftUI #283

Merged
merged 8 commits into from
Aug 16, 2024
Merged

Conversation

watt
Copy link
Collaborator

@watt watt commented May 20, 2024

This is a complete implementation of Workflow-powered SwiftUI views using fine-grained observability to minimize render impact. It's based on the prototype in #276, and @square-tomb's previous prototype #260. Squares can learn more at go/workflow-swiftui.

Observation

We're depending on Perception, Point-Free's backport of Observation. The approach to observable state is adapted from TCA's approach, with a custom @ObservableState macro that endows struct types with a concept of identity.

In workflows, you must annotate your state with @ObservableState, and render a type conforming to ObservableModel, which wraps your state and sends mutations into the unidirectional flow. There are several built-in conveniences for rendering common cases, or you can create a custom type.

On the view side, your View will consume a Store<Model> wrapper, which provides access to state, sinks, and any child stores from nested workflows.

To wire things up, you can implement a trivial type conforming to ObservableScreen and map your rendering. It's strongly recommended to keep these layers separate: render a model, implement a view, and create a screen only where needed. This allows for compositions of workflows and views, which is difficult or impossible if rendering a screen directly.

Check out the new ObservableScreen sample app for complete examples of most concepts being introduced here. I'll also write up an adoption guide that expands on each requirement.

SwiftPM

The @ObservableState macro means we cannot ship WorkflowSwiftUI with CocoaPods. For now, this PR only removes WorkflowSwiftUI podspec, but it may be preferable to remove all podpsecs and migrate everything to SwiftPM to reduce the maintenance burden.

To work on WorkflowSwiftUI locally, you can use xcodegen to create a project, similarly to how pod gen works.

Checklist

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

Follow-up tasks

  • Write an adoption guide (WorkflowSwiftUI adoption guide #292)
  • Screen integration tests
  • Remove podspecs
  • Replace xcodegen with Tuist
  • Improve the screen-backing UIHostingController (UI-5797)

@watt watt force-pushed the awatt/swiftui-observable-state branch 3 times, most recently from e2a4c9f to 2d6af7e Compare June 19, 2024 01:17
@watt watt changed the base branch from main to awatt/update-ci-config June 19, 2024 01:18
@watt watt changed the title [WIP] Observable State for WorkflowSwiftUI Observable State for WorkflowSwiftUI Jun 19, 2024
@watt watt force-pushed the awatt/swiftui-observable-state branch 7 times, most recently from 9c0f328 to 50e8a73 Compare June 19, 2024 06:46
Base automatically changed from awatt/update-ci-config to main June 20, 2024 20:23
@watt watt force-pushed the awatt/swiftui-observable-state branch from fcd9e4d to 512fd39 Compare June 24, 2024 21:49
@watt watt marked this pull request as ready for review June 24, 2024 22:04
@watt watt requested review from a team as code owners June 24, 2024 22:04
NOTICE.txt Outdated Show resolved Hide resolved
"Workflow",
"WorkflowUI",
"WorkflowSwiftUIMacros",
.product(name: "CasePaths", package: "swift-case-paths"),
Copy link
Contributor

Choose a reason for hiding this comment

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

[Question for myself]
Confirm we want this and below?


var body: some View {
let _ = Self._printChanges()
WithPerceptionTracking {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised to see that need this here... Can this be hoisted into a shared layer? It feels like it should be avoidable boilerplate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It cannot unfortunately. This is a requirement for the Observation backport to work on iOS <17. You can learn more in this Point-Free series on Observation or in the docs for Perception.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, maybe I'm not 100% understanding, but this more reads to me that this is about leveraging the backport, not that we need this in the view itself... Eg I'm imaging if we had a view that lived right above this that Market owned, that could do something like

struct MarketInternalView : View {

   var wrapped : some View

   var body : some View {
      WithPerceptionTracking {
          wrapped.body
       }
   }
}

Or similar?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You might expect it to work that way, especially coming from Blueprint, but SwiftUI bodies are evaluated independently, not as a recursive operation. You can't directly call into wrapped.body like that. If you built a wrapper like this:

private struct PerceptionWrapper<Content: View>: View {
    var content: Content
    
    var body: some View {
        WithPerceptionTracking {
            content
        }
    }
}

You'd find that PerceptionWrapper.body doesn't evaluate content.body at all — it's evaluated later by itself. So the WithPerceptionTracking wrapper, which captures observations synchronously, won't capture any observations done inside Content.

This is actually a good thing, because it allows for PerceptionWrapper and Content to be re-evaluated independently. But it means that any view using a Store must add its own WithPerceptionTracking wrapper to track the observations in its own body.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, makes sense!

I still don't think it's really expected / reasonable to expect everyone to have to wrap like this – perhaps a good case for a macro of our own?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you elaborate on that idea? Not sure how I see how that could be implemented. Open to suggestions and contributions if anyone has a better way!

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it's an unfortunate necessity, and we should expect that views that take escaping closures will force us to even write multiple WithPerceptionTracking wrappers in the same view body, like

WithPerceptionTracking {
  ForEach(store.scope(state: \.rows, action: \.rows), id: \.state.id) { store in
    WithPerceptionTracking {
      Text(store.title)
    }
  }
}

AFAIK TCA hasn't come up with any sugar for this.

We do have a runtime warning that you'll see if you access an ObservableState type's property outside of a WithPerceptionTracking, correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We do have a runtime warning that you'll see if you access an ObservableState type's property outside of a WithPerceptionTracking, correct?

That's right. You'll get a "purple" runtime warning.

Screenshot 2024-08-08 at 4 29 59 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

and we should expect that views that take escaping closures will force us to even write multiple WithPerceptionTracking wrappers in the same view body, like

Huh, I thought that Apple had fixed this somehow, but I don't remember how...

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you elaborate on that idea?

I was thinking we could potentially add a type-level macro that could mutate the body and add the WithPerceptionTracking call, but perhaps that not possible.

Samples/ObservableScreen/Sources/CounterWorkflow.swift Outdated Show resolved Hide resolved
State(count: initialValue, maxValue: maxValue, info: info)
}

func workflowDidChange(from previousWorkflow: CounterWorkflow, state: inout State) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit unclear what this does – is it required for updating dependencies from the parent? Can we automate this somehow? Maintaining this sort of thing seems pretty error prone and fragile; if you add a property but forget to add it here etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I'll have to cover this in the adoption docs, because it's definitely one of the biggest gotchas.

Parent dependencies need to go on your State so that we can track when they change and avoid body invalidations when they don't change, and workflowDidChange is the hook for updating state between two renders.

If a dependency is @ObservableState you can assign it directly and the macro handles checking changes. If it's not, you've got to do a comparison yourself. Otherwise, the property will be considered mutated on every render, and invalidate the body of any view that access that property on every render.

I think this is potentially something we could iterate on to make easier in the future, but for now I couldn't come up with anything better.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should definitely iterate on this! Wonder if a custom macro applied to the workflow itself could deal with this.

Copy link
Member

Choose a reason for hiding this comment

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

+1 to spending some time iterating on this. Feels like it's almost certain to cause issues.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Open to suggestions on this, but I won't have any additional iterations in this PR. Perhaps after we see some real usage, common patterns will be revealed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that a macro on the workflow type seems like a promising way reduce boilerplate here, but is a deep enough problem that it should be left for a future iteration.

This is a great example case for that problem, where the workflow's props are a mix of ObservableState and non-OS types.

}

private func send<Value>(keyPath: WritableKeyPath<State, Value>, value: Value) {
guard !invalidated else {
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is false, the change will be silently dropped – should this be a fatal instead, or can we add some sort of additional validation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, I'm not sure we can. 🤔

Besides preventing the cache from growing unbounded, invalidating stores is useful because SwiftUI itself has been known to send values into bindings long after their views are invalid, and we don't want those values (and they will potentially assert at the workflow level). I'm not sure there's a way we could tell if a value came from SwiftUI misbehaving or programmer misbehaving.

WorkflowSwiftUI/Sources/Store.swift Show resolved Hide resolved
WorkflowSwiftUI/Sources/Store.swift Outdated Show resolved Hide resolved
WorkflowSwiftUI/Sources/Store.swift Show resolved Hide resolved
WorkflowSwiftUIMacros/Sources/Availability.swift Outdated Show resolved Hide resolved
Copy link
Member

@robmaceachern robmaceachern left a comment

Choose a reason for hiding this comment

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

I'm still digesting Store.swift and others but figured I'd share feedback so far. Will continue review tomorrow!

.github/workflows/swift.yaml Show resolved Hide resolved
WorkflowSwiftUI/Sources/ObservableState.swift Outdated Show resolved Hide resolved
Samples/ObservableScreen/Sources/AppDelegate.swift Outdated Show resolved Hide resolved
Samples/ObservableScreen/Sources/CounterScreen.swift Outdated Show resolved Hide resolved
Samples/ObservableScreen/Sources/CounterWorkflow.swift Outdated Show resolved Hide resolved
Samples/ObservableScreen/Sources/MultiCounterView.swift Outdated Show resolved Hide resolved
WorkflowSwiftUI/Sources/ObservableModel.swift Outdated Show resolved Hide resolved
/// public var body: some View {
/// Toggle(
/// "Enabled",
/// isOn: $store.isOn.sending(action: \.toggle)
Copy link
Member

Choose a reason for hiding this comment

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

woah that's pretty cool

Copy link

Choose a reason for hiding this comment

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

+1

WorkflowSwiftUI/Sources/Store.swift Show resolved Hide resolved
State(count: initialValue, maxValue: maxValue, info: info)
}

func workflowDidChange(from previousWorkflow: CounterWorkflow, state: inout State) {
Copy link
Member

Choose a reason for hiding this comment

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

+1 to spending some time iterating on this. Feels like it's almost certain to cause issues.

Copy link
Member

@robmaceachern robmaceachern left a comment

Choose a reason for hiding this comment

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

Lots of really cool stuff here. Nice work @watt!

I think it'd be a great idea to schedule a walkthrough for UIS and Foundation folks in the next week or two to improve the bus factor on this stuff.

WorkflowSwiftUI/Sources/Store.swift Outdated Show resolved Hide resolved
}

state.count += 1
await fulfillment(of: [countDidChange], timeout: 0)
Copy link
Member

Choose a reason for hiding this comment

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

Does a timeout: 0 represent that this is a synchronous operation? I'm not sure I've seen expectations used this way before (not opposed to it!)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Basically, yes. No timeout means this call will hang forever if it's not fulfilled, 0 timeout means there's no waiting and the test will fail if the expectations haven't been fulfilled when this is called.

WorkflowSwiftUI/Tests/ObservableStateTests.swift Outdated Show resolved Hide resolved
WorkflowSwiftUI/Tests/ObservableStateTests.swift Outdated Show resolved Hide resolved
countDidChange.fulfill()
}

var newState = State(count: 1)
Copy link
Member

Choose a reason for hiding this comment

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

Does count even need to be different for this test to succeed? If I understand, instantiating a new State will cause it to be given a new storage/location/identity.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, the count doesn't need to change for the observation to trigger, but later this test is also testing that mutations made through the store affect the model that we pass in. Changing the count just makes it easier to do those assertions.

WorkflowSwiftUI/Sources/ObservableState.swift Outdated Show resolved Hide resolved
Comment on lines +103 to +111
return MultiCounterModel(
accessor: context.makeStateAccessor(state: state),
counters: counters,
maxCounter: maxCounter,
sumAction: sumAction,
counterAction: counterAction,
resetAction: resetAction
)
}
Copy link
Member

Choose a reason for hiding this comment

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

Something that's confusing to me is what this is rendering and how it relates to the views / screen. Sharing the thought process I went through when trying to review this.

It looks like the new expectation is Workflows render a model instead of a screen. But how does that model relate to the eventual screen / view that gets displayed? My first guess was to look at MultiCounterModel and see if it has something that produces a screen / view, but it doesn't, so I hit a bit of a dead end because the Workflow doesn't seem to expose any association to the view that gets displayed.

So then I searched around for MultiCounterView and found the only place that creates it is the MultiCounterScreen. Then I searched for who creates that, and it's actually created by the code that uses MultiCounterWorkflow:

let root = WorkflowHostingController(
    workflow: MultiCounterWorkflow().mapRendering(MultiCounterScreen.init)
)

This is jarring because it means that an individual workflow type (in this case MultiCounterWorkflow) has no direct tie to the actual screen / view that uses the data from the Workflow. Perhaps this is the intention, but it feels like a weird use of Workflow's paradigms. For example, the name of "rendering" no longer seems to fit here - Workflows don't produce a rendering anymore, they produce a model that an unrelated screen knows how to convert to a SwiftUI view. And if I'm a developer trying to consume a Workflow from another module, how do I know what view I'm supposed to map the rendering to? could we tie them together a bit more so developers don't have to literally read the code to figure out what model connects to which screen?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this would work at all but something that comes to mind is composing the two concepts together, so you have:

  • A workflow that renders a model, this implementation of MultiCounterWorkflow
  • A workflow that renders a screen / view, and consumes the other workflow (in this case MultiCounterWorkflow) as a child in it's render function and does the translation from MultiCounterWorkflow.Rendering to MultiCounterScreen

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good feedback! I'll break it up a bit to respond.

This is jarring because it means that an individual workflow type (in this case MultiCounterWorkflow) has no direct tie to the actual screen / view that uses the data from the Workflow. Perhaps this is the intention, but it feels like a weird use of Workflow's paradigms.

That's correct. The view and the workflow both have a dependency on the state and actions (collectively, the model), but not necessarily on each other. If the state & actions are nested in the workflow type, then the view has a dependency on that, effectively inverting the dependency you'd see if rendering a screen. It's a different pattern, for sure, and it's something we expected to require some education. I'm working on an adoption guide to help with that.

For example, the name of "rendering" no longer seems to fit here - Workflows don't produce a rendering anymore, they produce a model that an unrelated screen knows how to convert to a SwiftUI view.

The rendering is, by definition, whatever the workflow renders. The notion that every workflow should be rendering a screen is so pervasive today that I'd say it's harmful — nothing is composable and tests have to force-unwrap type erasers. In a world where views are easily composable, you often won't need a screen to act as the glue, so I'm trying to discourage the idea that every workflow has associated screen type.

And if I'm a developer trying to consume a Workflow from another module, how do I know what view I'm supposed to map the rendering to? could we tie them together a bit more so developers don't have to literally read the code to figure out what model connects to which screen?

Well in most cases I think it'll be obvious, because they'll live in the same module and have a naming pattern. In other cases, it's a matter of matching up the model. There's no guaranteed 1:1 mapping — you could write 2 different view implementations that take the same model type, for example. You could even power a view with 2 different workflows, but I think that's unlikely.

Not sure if this would work at all but something that comes to mind is composing the two concepts together, so you have:

  • A workflow that renders a model, this implementation of MultiCounterWorkflow
  • A workflow that renders a screen / view, and consumes the other workflow (in this case MultiCounterWorkflow) as a child in it's render function and does the translation from MultiCounterWorkflow.Rendering to MultiCounterScreen

You could write a workflow that does that latter step, but that's exactly what mapRendering() does!

Copy link
Member

Choose a reason for hiding this comment

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

You could write a workflow that does that latter step, but that's exactly what mapRendering() does!

Fair! But I was looking for a way for consumers to not have to dig into the code to know what to map the rendering to. Sure they might be similarly named but its still an awkward interface.

The rendering is, by definition, whatever the workflow renders. The notion that every workflow should be rendering a screen is so pervasive today that I'd say it's harmful — nothing is composable and tests have to force-unwrap type erasers. In a world where views are easily composable, you often won't need a screen to act as the glue, so I'm trying to discourage the idea that every workflow has associated screen type.

100% understand this - I'm more thinking of it as a developer coming the existing norm to this.

I guess the overall feeling is that this doesn't really fit into how we use Workflow today but its still built on top of Workflow as we know it. I wonder if theres some way we could separate the two a bit. What would it look like if you were to implement this from scratch, and how would you want to integrate that into an existing codebase that uses Workflow?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess the overall feeling is that this doesn't really fit into how we use Workflow today but its still built on top of Workflow as we know it. I wonder if theres some way we could separate the two a bit. What would it look like if you were to implement this from scratch, and how would you want to integrate that into an existing codebase that uses Workflow?

Well, you're not wrong, but I think that was sort of the goal, to build a SwiftUI integration on top of Workflow. I'm not sure how we could separate the two further without breaking that. We did prototype some alternative designs that were much more similar to how Workflow is used today, but ultimately opted for this instead because its advantages seemed to be worth the cost of the pattern disruption.

///
/// Rather than creating this model directly, you should use the
/// ``Workflow/RenderContext/makeActionModel(state:)`` method to create an instance of this model.
public struct ActionModel<State: ObservableState, Action>: ObservableModel, SingleActionModel {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: since SingleActionModel refines ObservableModel do we need both conformances here? i guess if it doesn't cause any compiler warnings it's fine, and maybe better to be more explicit. curious if there is a functional distinction with the ObservableModel conformance removed though

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No difference. SingleActionModel only exists so that it can be used as a constraint in some conveniences for when there's only one action.

@watt
Copy link
Collaborator Author

watt commented Aug 3, 2024

I wrote up an adoption guide in #292.

Co-authored-by: Jamie <2119834+jamieQ@users.noreply.github.com>
Co-authored-by: Robert MacEachern <rob@robmaceachern.com>
Copy link
Contributor

@jamieQ jamieQ left a comment

Choose a reason for hiding this comment

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

overall, this seems like an interesting and exciting direction – nice work! as others have pointed out, it is a bit of a break from some existing patterns & paradigms that are fairly prevalent, but that seems somewhat expected given the move to a different UI 'backend' of sorts. some high-level thoughts on things i wish i had a better sense of:

  1. how difficult will plugging existing Workflows into this system be? i assume it will depend on the specifics to a large extent, but my impression is that Screen-based renderings won't be easy to re-use (but maybe that's fine & expected).
  2. relatedly, what does Worker integration look like in this model? might be good to include that in an example somewhere at some point.
  3. [vague thought] what facilities will we suggest to folks for testing the view binding logic? snapshot tests?

i don't really have blocking concerns about the implementation. i think getting some 'real world' feedback on using it would be quite valuable.

Copy link

@g-mark g-mark left a comment

Choose a reason for hiding this comment

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

This is great work, @watt!

Please forgive the no0b questions.

.monospacedDigit()

Button {
store.send(.increment)
Copy link

Choose a reason for hiding this comment

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

I noticed that something like store.count += 1 works equally as well here, instead of sending an action.

Is there any guidance around how to choose between actions, bindings (e.g. for a TextField), or direct mutation? Asked another way, how tightly do we want to embrace/enforce the unidirectional data flow aspect of Workflow?

Copy link
Contributor

Choose a reason for hiding this comment

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

Does the author of CounterWorkflow have the option to declare CounterWorkflow.State.count as fileprivate(set) var, allowing it to be mutated only by CounterWorkflow.Action, and making it impossible to mutate it directly from a view or create a binding from it?

Copy link

Choose a reason for hiding this comment

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

Yes, when I did that locally it prevented both direct mutation and the creation of bindings.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is there any guidance around how to choose between actions, bindings (e.g. for a TextField), or direct mutation? Asked another way, how tightly do we want to embrace/enforce the unidirectional data flow aspect of Workflow?

The unidirectional flow is enforced either way. Mutations from setters like store.count += 1 are routed through a StateMutationSink, which sends a generic AnyWorkflowAction<WorkflowType> under the hood.

The choice of when to use a custom action is probably going to be driven by:

  • need to do something more than updating the property (maybe you touch 2 properties, maybe there's logging, etc)
  • migrating a workflow that already a custom action

In other words, "whenever you need it or want it", heh.

/// public var body: some View {
/// Toggle(
/// "Enabled",
/// isOn: $store.isOn.sending(action: \.toggle)
Copy link

Choose a reason for hiding this comment

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

+1

/// - Parameter sink: The sink to receive an action with values from the binding.
/// - Parameter action: An action to contain sent values.
/// - Returns: A binding.
public func sending<Action>(
Copy link

Choose a reason for hiding this comment

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

Just noodling on the API...

  • what if the API looked like this from the call site:
    $store.foo.sending(\.fooAction, to: \.actionSink)
  • Or, given that the func returns a Binding, if the func name:
    $store.foo.binding(\.fooAction, to: \.actionSink)
    $store.foo.bind(to: \.sink, action: \.fooAction)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's not bad! I probably prefer sending because $store.foo is already a binding, and we're only refining one side of it. I'll keep this in mind for API refinements since we'll likely have others come up too.

var count: Int {
get { _count }
set {
if let maxValue, newValue > maxValue {
Copy link

Choose a reason for hiding this comment

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

Is there a concern that the logic for protecting state can be located in a setter as well as in an action? I mean, this could probably have been done prior to SwiftUI, but this state is mutable from the SwiftUI view because it's @ObservableState, so will be much easier to do.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, it's not exactly the setter that's the concern, but because accessible properties get a "free" mutation action, there is definitely a gotcha where you might declare an action and forget to use it. It'll be easily discoverable if your action does anything, but might still be an annoying lesson to learn.


var body: some View {
let _ = Self._printChanges()
WithPerceptionTracking {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it's an unfortunate necessity, and we should expect that views that take escaping closures will force us to even write multiple WithPerceptionTracking wrappers in the same view body, like

WithPerceptionTracking {
  ForEach(store.scope(state: \.rows, action: \.rows), id: \.state.id) { store in
    WithPerceptionTracking {
      Text(store.title)
    }
  }
}

AFAIK TCA hasn't come up with any sugar for this.

We do have a runtime warning that you'll see if you access an ObservableState type's property outside of a WithPerceptionTracking, correct?

.monospacedDigit()

Button {
store.send(.increment)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the author of CounterWorkflow have the option to declare CounterWorkflow.State.count as fileprivate(set) var, allowing it to be mutated only by CounterWorkflow.Action, and making it impossible to mutate it directly from a view or create a binding from it?

State(count: initialValue, maxValue: maxValue, info: info)
}

func workflowDidChange(from previousWorkflow: CounterWorkflow, state: inout State) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that a macro on the workflow type seems like a promising way reduce boilerplate here, but is a deep enough problem that it should be left for a future iteration.

This is a great example case for that problem, where the workflow's props are a mix of ObservableState and non-OS types.

}

typealias Rendering = ActionModel<State, Action>
typealias Model = ActionModel<State, Action>
Copy link
Contributor

Choose a reason for hiding this comment

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

This typealias is not necessary to conform to some protocol; it just looks nicer to write CounterWorkflow.Model than CounterWorkflow.Rendering in some places?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah. I tried to be consistent with the use of Model as a first-class concept.

HStack {
Text("Sum")
Spacer()
Text("\(store.counters.map(\.count).reduce(0, +))")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is interesting. WWDC counsels against doing potentially expensive calculations in view bodies (e.g. O(n) string manipulations), but I'm not sure where else you could conveniently locate this kind of calculation across multiple observable models' values.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, yeah, I'm not sure. If it's expensive it's not great in the workflow render either.

@watt
Copy link
Collaborator Author

watt commented Aug 9, 2024

  1. how difficult will plugging existing Workflows into this system be? i assume it will depend on the specifics to a large extent, but my impression is that Screen-based renderings won't be easy to re-use (but maybe that's fine & expected).

Highly dependent on the workflow and what your goals for migration are, but you are correct that screens are generally not reusable. If you are replacing a Blueprint screen rendering with a SwiftUI screen, most of the work may go towards rewriting your Element as a View. If you are hoping to use the same workflow to power both a Blueprint screen and SwiftUI screen during the transition, you'll also need to adapt your Blueprint screen to consume/wrap the view model.

I'll probably write a guide specifically for migrations like this after we've seen what that looks like in practice.

  1. relatedly, what does Worker integration look like in this model? might be good to include that in an example somewhere at some point.

I'm not anticipating any changes there. Workers won't interact directly with SwiftUI, and don't need to observe state.

  1. [vague thought] what facilities will we suggest to folks for testing the view binding logic? snapshot tests?

Snapshot tests, primarily. XCUI tests may also be very helpful, to fill KIF's shoes, and to validate that state changes only invalidate the views we expect. I have a follow-up task to figure out what that looks like.

@watt watt merged commit b027b1d into main Aug 16, 2024
14 checks passed
@watt watt deleted the awatt/swiftui-observable-state branch August 16, 2024 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants