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.

Sign up for GitHub

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

Observable State for WorkflowSwiftUI #283

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
512fd39
Observable state WorkflowSwiftUI
watt Apr 10, 2024
4c67859
remove unused cruft from sample
watt Jul 25, 2024
1501911
fix max counter invalidation
watt Jul 25, 2024
e4dc19d
indicate files primarily derived from TCA/Apple
watt Jul 29, 2024
f77ee5a
rename changed to isChanged
watt Jul 30, 2024
3522b1d
enforce maxValue in state instead of view
watt Jul 30, 2024
d080efd
test optional presence cases
watt Jul 30, 2024
354eb49
fix documentation typos
watt Aug 6, 2024
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
47 changes: 42 additions & 5 deletions .github/workflows/swift.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ jobs:
-destination "$IOS_DESTINATION" \
build test | bundle exec xcpretty

spm:
xcodegen-apps:
runs-on: macos-latest

steps:
Expand All @@ -61,18 +61,55 @@ jobs:
- name: Switch Xcode
run: sudo xcode-select -s /Applications/Xcode_${XCODE_VERSION}.app

- name: Swift Package Manager - iOS
- name: Install XcodeGen
run: brew install xcodegen

- name: Generate Xcode project
run: xcodegen generate
watt marked this conversation as resolved.
Show resolved Hide resolved

- name: ObservableScreen
run: |
xcodebuild \
-project Workflow.xcodeproj \
-scheme "ObservableScreen" \
-destination "$IOS_DESTINATION" \
-skipMacroValidation \
build

package-tests:
runs-on: macos-latest

steps:
- uses: actions/checkout@v4

- name: Switch Xcode
run: sudo xcode-select -s /Applications/Xcode_${XCODE_VERSION}.app

- name: Install XcodeGen
run: brew install xcodegen

- name: Generate Xcode project
run: xcodegen generate

# Macros are only built for the compiler platform, so we cannot run macro tests on iOS. Instead
# we target a scheme from project.yml which selectively includes all the other tests.
- name: Tests - iOS
run: |
xcodebuild \
-scheme "Workflow-Package" \
-project Workflow.xcodeproj \
-scheme "Tests-iOS" \
-destination "$IOS_DESTINATION" \
-skipMacroValidation \
test

- name: Swift Package Manager - macOS
# On macOS we can run all tests, including macro tests.
- name: Tests - macOS
run: |
xcodebuild \
-scheme "Workflow-Package" \
-project Workflow.xcodeproj \
-scheme "Tests-All" \
-destination "platform=macOS" \
-skipMacroValidation \
test

tutorial:
Expand Down
6 changes: 5 additions & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,14 @@ xcuserdata/
# Sample workspace
SampleApp.xcworkspace

# XcodeGen
Workflow.xcodeproj/
/TestingSupport/AppHost/App/Info.plist

# ios-snapshot-test-case Failure Diffs
FailureDiffs/

Samples/**/*Info.plist
!Samples/Tutorial/AppHost/Configuration/Info.plist
!Samples/Tutorial/AppHost/TutorialTests/Info.plist
!Samples/AsyncWorker/AsyncWorker/Info.plist
!Samples/AsyncWorker/AsyncWorker/Info.plist
3 changes: 2 additions & 1 deletion .swiftformat
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# file config

--swiftversion 5.7
--swiftversion 5.9
--exclude Pods,Tooling,**Dummy.swift

# format config
Expand All @@ -24,6 +24,7 @@
--enable spaceInsideBraces
--enable specifiers
--enable trailingSpace # https://google.github.io/swift/#horizontal-whitespace
--enable wrapMultilineStatementBraces

--allman false
--binarygrouping none
Expand Down
9 changes: 0 additions & 9 deletions Development.podspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,6 @@ Pod::Spec.new do |s|
test_spec.source_files = 'WorkflowTesting/Tests/**/*.swift'
end

s.app_spec 'SampleSwiftUIApp' do |app_spec|
app_spec.ios.deployment_target = WORKFLOW_IOS_DEPLOYMENT_TARGET
app_spec.dependency 'WorkflowSwiftUI'
app_spec.pod_target_xcconfig = {
'IFNFOPLIST_FILE' => '${PODS_ROOT}/../Samples/SampleSwiftUIApp/SampleSwiftUIApp/Configuration/Info.plist'
}
app_spec.source_files = 'Samples/SampleSwiftUIApp/SampleSwiftUIApp/**/*.swift'
end

s.app_spec 'SampleTicTacToe' do |app_spec|
app_spec.source_files = 'Samples/TicTacToe/Sources/**/*.swift'
app_spec.resources = 'Samples/TicTacToe/Resources/**/*'
Expand Down
9 changes: 9 additions & 0 deletions NOTICE.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
Part of the distributed code is also derived in part from
https://github.com/pointfreeco/swift-composable-architecture, licensed under MIT
(https://github.com/pointfreeco/swift-composable-architecture/blob/main/LICENSE).
Copyright (c) 2020 Point-Free, Inc.

Part of the distributed code is also derived in part from
https://github.com/apple/swift licensed under Apache
(https://github.com/apple/swift/blob/main/LICENSE.txt). Copyright 2024 Apple,
Inc.
43 changes: 40 additions & 3 deletions Package.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// swift-tools-version:5.7
// swift-tools-version:5.9
// The swift-tools-version declares the minimum version of Swift required to build this package.

import CompilerPluginSupport
import PackageDescription

let package = Package(
Expand Down Expand Up @@ -58,7 +59,12 @@ let package = Package(
dependencies: [
.package(url: "https://github.com/ReactiveCocoa/ReactiveSwift.git", from: "7.1.1"),
.package(url: "https://github.com/ReactiveX/RxSwift.git", from: "6.6.0"),
.package(url: "https://github.com/nicklockwood/SwiftFormat", exact: "0.44.14"),
.package(url: "https://github.com/nicklockwood/SwiftFormat", exact: "0.54.0"),
.package(url: "https://github.com/apple/swift-syntax", from: "509.0.0"),
.package(url: "https://github.com/pointfreeco/swift-case-paths", from: "1.1.0"),
.package(url: "https://github.com/pointfreeco/swift-identified-collections", from: "1.0.0"),
.package(url: "https://github.com/pointfreeco/swift-macro-testing", from: "0.4.0"),
.package(url: "https://github.com/pointfreeco/swift-perception", from: "1.1.4"),
],
targets: [
// MARK: Workflow
Expand Down Expand Up @@ -96,11 +102,42 @@ let package = Package(
dependencies: ["WorkflowUI", "WorkflowReactiveSwift"],
path: "WorkflowUI/Tests"
),

// MARK: WorkflowSwiftUI

.target(
name: "WorkflowSwiftUI",
dependencies: ["Workflow"],
dependencies: [
"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?

.product(name: "IdentifiedCollections", package: "swift-identified-collections"),
.product(name: "Perception", package: "swift-perception"),
],
path: "WorkflowSwiftUI/Sources"
),
.testTarget(
name: "WorkflowSwiftUITests",
dependencies: ["WorkflowSwiftUI"],
path: "WorkflowSwiftUI/Tests"
),
.macro(
name: "WorkflowSwiftUIMacros",
dependencies: [
.product(name: "SwiftSyntaxMacros", package: "swift-syntax"),
.product(name: "SwiftCompilerPlugin", package: "swift-syntax"),
],
path: "WorkflowSwiftUIMacros/Sources"
),
.testTarget(
name: "WorkflowSwiftUIMacrosTests",
dependencies: [
"WorkflowSwiftUIMacros",
.product(name: "MacroTesting", package: "swift-macro-testing"),
],
path: "WorkflowSwiftUIMacros/Tests"
),

// MARK: WorkflowReactiveSwift

Expand Down
3 changes: 1 addition & 2 deletions RELEASING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ For Squares, membership is managed through the `Workflow Swift Owners` registry

> ⚠️ [Optional] To avoid possible headaches when publishing podspecs, validation can be performed before updating the Workflow version number(s). To do this, run the following in the root directory of this repo:
> ```bash
> bundle exec pod lib lint Workflow.podspec ViewEnvironment.podspec ViewEnvironmentUI.podspec WorkflowTesting.podspec WorkflowReactiveSwift.podspec WorkflowUI.podspec WorkflowRxSwift.podspec WorkflowReactiveSwiftTesting.podspec WorkflowRxSwiftTesting.podspec WorkflowSwiftUI.podspec WorkflowSwiftUIExperimental.podspec WorkflowCombine.podspec WorkflowCombineTesting.podspec WorkflowConcurrency.podspec WorkflowConcurrencyTesting.podspec
> bundle exec pod lib lint Workflow.podspec ViewEnvironment.podspec ViewEnvironmentUI.podspec WorkflowTesting.podspec WorkflowReactiveSwift.podspec WorkflowUI.podspec WorkflowRxSwift.podspec WorkflowReactiveSwiftTesting.podspec WorkflowRxSwiftTesting.podspec WorkflowSwiftUIExperimental.podspec WorkflowCombine.podspec WorkflowCombineTesting.podspec WorkflowConcurrency.podspec WorkflowConcurrencyTesting.podspec
> ```
> You may need to `--include-podspecs` for pods that have changed and are depended on by other of the pods.

Expand All @@ -43,7 +43,6 @@ For Squares, membership is managed through the `Workflow Swift Owners` registry
bundle exec pod trunk push WorkflowRxSwift.podspec --synchronous
bundle exec pod trunk push WorkflowReactiveSwiftTesting.podspec --synchronous
bundle exec pod trunk push WorkflowRxSwiftTesting.podspec --synchronous
bundle exec pod trunk push WorkflowSwiftUI.podspec --synchronous
watt marked this conversation as resolved.
Show resolved Hide resolved
bundle exec pod trunk push WorkflowSwiftUIExperimental.podspec --synchronous
bundle exec pod trunk push WorkflowCombine.podspec --synchronous
bundle exec pod trunk push WorkflowCombineTesting.podspec --synchronous
Expand Down
21 changes: 21 additions & 0 deletions Samples/ObservableScreen/Sources/AppDelegate.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import UIKit
import Workflow
import WorkflowUI

@main
class AppDelegate: UIResponder, UIApplicationDelegate {
var window: UIWindow?

func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplication.LaunchOptionsKey: Any]?) -> Bool {
let root = WorkflowHostingController(
workflow: MultiCounterWorkflow().mapRendering(MultiCounterScreen.init)
)
root.view.backgroundColor = .systemBackground

window = UIWindow(frame: UIScreen.main.bounds)
window?.rootViewController = root
window?.makeKeyAndVisible()

return true
}
}
61 changes: 61 additions & 0 deletions Samples/ObservableScreen/Sources/CounterView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import SwiftUI
import ViewEnvironment
import WorkflowSwiftUI

struct CounterView: View {
typealias Model = CounterModel

let store: Store<Model>
let key: String

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.

let _ = print("Evaluated CounterView[\(key)] body")
HStack {
Text(store.info.name)

Spacer()

Button {
store.send(.decrement)
} label: {
Image(systemName: "minus")
}

Text("\(store.count)")
.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.

} label: {
Image(systemName: "plus")
}

if let maxValue = store.maxValue {
Text("(max \(maxValue))")
}
}
}
}
}

#if DEBUG

#Preview {
CounterView(
store: .preview(
state: .init(
count: 0,
info: .init(
name: "Preview counter",
stepSize: 1
)
)
),
key: "preview"
)
.padding()
}

#endif
Loading