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

WorkflowSwiftUI adoption guide #292

Merged
merged 6 commits into from
Sep 25, 2024
Merged

WorkflowSwiftUI adoption guide #292

merged 6 commits into from
Sep 25, 2024

Conversation

watt
Copy link
Collaborator

@watt watt commented Aug 3, 2024

A documentation follow-up to #283.

@watt watt requested a review from a team as a code owner August 3, 2024 00:39
@watt watt mentioned this pull request Aug 3, 2024
4 tasks

## Parent dependencies

Because `WorkflowSwiftUI` relies on observation to function, most properties on your model cannot be safely accessed directly through the `Store` — only the state, child models, and action sinks are accessible. So, when a workflow has dependencies provided by a parent (inputs), they must be added to your workflow’s own state in order to be visible by your view. They must also be updated in the `workflowDidChange` function of your workflow in order to receive updates from your parent.
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: might be worth also referencing the 'props' terminology for the 'inputs' that is used in some of the existing documentation (maybe mostly on the Kotlin-side).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure, done

Copy link
Contributor

@kyleve kyleve 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, thanks! Posting some comments that I think I may've posted on the main PR, but my thoughts are a bit more well-formed now, so repeating myself a bit


For an `ObservableState` type, creating a copy does not change the state’s identity in the copy. However, creating a new value by calling the initializer will result in a new identity.

If your state has nested types, you can annotate them with `@ObservableState` as well, for fine-grained observation of the properties of that type as well.
Copy link
Contributor

Choose a reason for hiding this comment

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

If these types definitions were structurally nested within each other, would we still need the nested type to have @ObservableState?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it's not inherited. I've updated the example to demo that better.

state: State,
context: RenderContext<Self>
) -> ActionModel<State, Action> {
context.makeActionModel(state: state)
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if you call this and there's more than one case in your Action?

Copy link
Member

Choose a reason for hiding this comment

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

I think the idea is that this used for a single WorkflowAction type (multiple cases is fine). It'd probably be worth adding an additional action case to this example to help illustrate/reinforce that though (and a small tweak to the wording above the example).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated the example to have another case demonstrating this.


```

For complex workflows that have multiple actions or compose observable models from child
Copy link
Contributor

Choose a reason for hiding this comment

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

For complex workflows that have multiple actions

I feel like this is basically the common case, it'd be nice to make this require less boilerplate/code to be written.


```swift
@ObservableState
public struct State {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remind me, but does Apple's observation stuff support structs yet, or is it just classes?

Copy link
Member

Choose a reason for hiding this comment

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

AFAIK structs are still disallowed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, just classes, that's why we need our own ObservableState macro.


typealias PersonModel = ActionModel<PersonState, PersonAction>

struct PersonView: View {
Copy link
Contributor

Choose a reason for hiding this comment

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

Spitballing on how to avoid people needing WithPerceptionTracking:

We define a ObservedView protocol,

protocol ObservingView: View {

   var observing : some View { get }

}

extension ObservingView {
   var body : some View {
      WithPerceptionTracking { observing }
   }
}

Where folks would implement observing

Which requires learning the type, but once you know, you won't need the perception tracking wrapper, I think?

Copy link
Member

Choose a reason for hiding this comment

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

@watt Clients will be able to drop WithPerceptionTracking once the deployment target is bumped to 17, right?

If that's the case, I probably wouldn't introduce a new wrapper protocol just for that. Maybe we'll end up wanting/needing to wrap up some other functionality/convenience at the Market level and we can figure out how best to slip it in there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@watt Clients will be able to drop WithPerceptionTracking once the deployment target is bumped to 17, right?

That's right. You also need to use it inline in some lazy contexts, like GeometryReader.

}
```

If you forget to wrap your view's body in `WithPerceptionTracking`, a runtime warning will remind you.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we turn this into an assert as well? I worry folks will miss the runtime warning. Not sure if this is possible though since I assume this comes from the point free library!

Copy link
Member

Choose a reason for hiding this comment

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

The good news is that their runtime warning will fail tests when they are encountered in that context.

/// After the initial construction, the view will be updated by injecting new values into the
/// store.
@ViewBuilder
static func makeView(store: Store<Model>) -> Content
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit / thought: view(with:) or body(with:)?

Your screen will be relatively simple, as it has the relatively simple job of acting as the glue between your rendering model and your view type.

```swift
struct PersonScreen: ObservableScreen {
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 I mentioned this, but it'd be neat if we had a generic type that folks could use instead of needing to define this on their own

}
```

To use this screen, render your workflow and then use the `mapRendering()` function to wrap your workflow’s model rendering in a screen.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: include an example?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added


Because `WorkflowSwiftUI` relies on observation to function, most properties on your model cannot be safely accessed directly through the `Store` — only the state, child models, and action sinks are accessible. So, when a workflow has dependencies provided by a parent (inputs), they must be added to your workflow’s own state in order to be visible by your view. They must also be updated in the `workflowDidChange` function of your workflow in order to receive updates from your parent.

When updating dependencies, remember that any value being set is considered a mutation by the observation framework. To avoid invalidating your view on every render, check if dependencies have actually changed before copying them.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add some sort of operator to make this simpler, or a func to help with it? Eg, add a ~= that checks the values before assigning them?

Base automatically changed from awatt/swiftui-observable-state to main August 16, 2024 22:52
@watt watt requested a review from a team as a code owner August 16, 2024 22:52
@watt watt force-pushed the awatt/swiftui-adoption-guide branch from 9442f16 to 4d827d8 Compare August 16, 2024 22:57
@watt
Copy link
Collaborator Author

watt commented Sep 25, 2024

I think I've addressed all the documentation-related comments. Punting on the stuff related to API and patterns — I think we covered most of that in the main PR.

@watt watt merged commit a85c4e1 into main Sep 25, 2024
14 checks passed
@watt watt deleted the awatt/swiftui-adoption-guide branch September 25, 2024 00:30
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.

4 participants