[BREAKING CHANGE]: update WorkflowAction API to expose access to Workflow instance values #349

jamieQ · 2025-05-20T22:20:37Z

this BREAKING change updates the WorkflowAction API to pass through a new parameter in the apply() method that allows reading values off the corresponding node's current Workflow instance. the new method signature is:

func apply(toState state: inout State, context: ApplyContext<WorkflowType>) -> Output?

which mirrors the render() API, but uses a new ApplyContext parameter to expose access to the runtime-managed data. the public API for ApplyContext exposes a read-only subscript for accessing Workflow values via keypath:

public struct ApplyContext<WorkflowType: Workflow> {
    public subscript<Value>(
        workflowValue keyPath: KeyPath<WorkflowType, Value>
    ) -> Value 
}

for reviewing purposes, i'd suggest using the 'commit filter' function to exclude the commit labeled 'AUTOMATED' as that one is essentially just noise where a bunch of new parameters were added.

also, there's definitely some more documentation to be written here, but i've currently punted on that somewhat. do feel free to point out places you'd like to see anything specific though.

Workflow/Sources/RenderContext.swift

g-mark · 2025-05-21T17:37:49Z

Workflow/Sources/WorkflowAction.swift

@@ -27,14 +27,22 @@ public protocol WorkflowAction<WorkflowType> {
    ///
    /// - Returns: An optional output event for the workflow. If an output event is returned, it will be passed up
    ///            the workflow hierarchy to this workflow's parent.
-    func apply(toState state: inout WorkflowType.State) -> WorkflowType.Output?
+    func apply(


Any thoughts around offering both the old and the new functions to avoid changes to lots of files?

I.e., if you added the new one, along with a default impl that called the old one, could we get away with only changing existing action types that need access to the context?

this was explored a few times in the past and i believe the conclusion was that it would be better to 'rip the bandaid off' so to speak. IMO the primary problem with trying to be source compatible is that you end up having to expose sort of redundant protocol requirements. yes, you can default the one with the new param to forward to the one without it, but it's still sort of an odd API. i think ideally you'd want them to be mutually exclusive, but i'm not sure that's expressible.

that being said – if you've got alternate ideas, happy to hear them!

Jamie has already explored ways to automate a large part of this migration, so I am fine with making the breaking change. It's why we have semver!

Yeah, that makes sense - thanks for sharing the previous exploration!

robmaceachern · 2025-05-21T17:42:46Z

Workflow/Sources/WorkflowAction.swift

+    func apply(
+        toState state: inout WorkflowType.State,
+        context: ApplyContext<WorkflowType>
+    ) -> WorkflowType.Output?
+}


I was wondering if there's some clever way to introduce this in a non-breaking way with protocol extensions (to ease the initial integration/migration) but I don't think there is... Throwing it out there in case someone smarter than me sees a way to make it work.

I tried that out here, and it works, but as @jamieQ mentioned above requires confusingly exposing a second protocol for actions.

mjohnson12

Much better than having to store stuff in State that is not state. 👍

jamieQ · 2025-05-21T20:21:21Z

Workflow/Sources/ApplyContext.swift

+ */
+
+/// Internal utility protocol so that `WorkflowTesting` can provide an alternate implementation
+protocol ApplyContextType<WorkflowType> {


i'm no longer certain this abstraction is needed. i mostly copied it from the analogous thing done for RenderTester, but unlike in that case i don't think the testing infra relies on 'real' internals to apply actions.

watt

I've got several questions but no big blockers. Thanks for tackling this!

I also noticed you're adding several TODO comments, where it's not entirely clear if you were intending to address them with this PR or if you're just marking new tech debt.

Workflow/Sources/WorkflowAction.swift

watt · 2025-05-22T00:06:28Z

Workflow/Sources/StateMutationSink.swift

@@ -44,7 +44,7 @@ public struct StateMutationSink<WorkflowType: Workflow> {
    ///   - update: The `State` mutation to perform.
    public func send(_ update: @escaping (inout WorkflowType.State) -> Void) {


thoughts on exposing the ApplyContext to StateMutationSink?

i think that probably makes sense to do, but would prefer to tackle as a follow-up for scope-management reasons, unless you feel this will immediately be desired. i think it's a straightforward plumbing change, and i may have had it implemented at one point but removed it.

I don't have any need for it immediately, was just curious.

Workflow/Sources/ApplyContext.swift

watt · 2025-05-22T00:26:03Z

Workflow/Sources/ApplyContext.swift

+    }
+
+    public subscript<Value>(
+        workflowValue keyPath: KeyPath<WorkflowType, Value>


Decided not to use dynamic property lookup?

yes, for now anyway. this will make callsites more verbose, but also easier to locate in the source code, and somewhat more explicit about what's going on.

Workflow/Sources/ApplyContext.swift

Workflow/Sources/WorkflowNode.swift

watt · 2025-05-22T00:33:10Z

Workflow/Tests/ApplyContextTests.swift

+    func concreteApplyContextInvalidatedAfterUse() async throws {
+        var escapedContext: ApplyContext<EscapingContextWorkflow>?
+        let onApply = { (context: ApplyContext<EscapingContextWorkflow>) in
+            #expect(context[workflowValue: \.property] == 42)


These are the first #expect-based tests in Workflow, I believe. Just to confirm, did you check that failures are caught in CI?

thanks for the callout. yes, i did a 'canary' run yesterday with a test that should have failed CI, and it seemed to behave as expected

WorkflowTesting/Sources/WorkflowActionTester.swift

jamieQ force-pushed the jquadri/action-apply-api-change branch 2 times, most recently from 6446a18 to e05bd71 Compare May 21, 2025 16:30

jamieQ commented May 21, 2025

View reviewed changes

Workflow/Sources/RenderContext.swift Outdated Show resolved Hide resolved

jamieQ force-pushed the jquadri/action-apply-api-change branch from e05bd71 to 2d260c7 Compare May 21, 2025 16:51

g-mark reviewed May 21, 2025

View reviewed changes

robmaceachern reviewed May 21, 2025

View reviewed changes

jamieQ changed the title ~~[WIP]: update WorkflowAction API to expose access to Workflow instance values~~ [breaking][feat]: update WorkflowAction API to expose access to Workflow instance values May 21, 2025

jamieQ marked this pull request as ready for review May 21, 2025 17:50

jamieQ requested a review from a team as a code owner May 21, 2025 17:50

mjohnson12 approved these changes May 21, 2025

View reviewed changes

watt self-requested a review May 21, 2025 20:04

jamieQ commented May 21, 2025

View reviewed changes

watt approved these changes May 22, 2025

View reviewed changes

jamieQ changed the title ~~[breaking][feat]: update WorkflowAction API to expose access to Workflow instance values~~ [BREAKING CHANGE]: update WorkflowAction API to expose access to Workflow instance values May 22, 2025

jamieQ mentioned this pull request May 22, 2025

Relax ActionTester to not require a full Workflow instance when testing ApplyContext interactions #351

Open

jamieQ added 3 commits May 27, 2025 13:15

[AUTOMATED][breaking]: run script to update apply() method params

e5bdc8d

[breaking]: update action apply API

33b6695

[docs]: update some docs per PR feedback

9063748

jamieQ force-pushed the jquadri/action-apply-api-change branch from 86c2e57 to 9063748 Compare May 27, 2025 18:16

square-tomb approved these changes May 28, 2025

View reviewed changes

jamieQ merged commit 876e575 into main May 30, 2025
8 checks passed

jamieQ deleted the jquadri/action-apply-api-change branch May 30, 2025 15:24

jamieQ restored the jquadri/action-apply-api-change branch May 30, 2025 15:24

		@@ -44,7 +44,7 @@ public struct StateMutationSink<WorkflowType: Workflow> {
		/// - update: The `State` mutation to perform.
		public func send(_ update: @escaping (inout WorkflowType.State) -> Void) {

[BREAKING CHANGE]: update WorkflowAction API to expose access to Workflow instance values #349

[BREAKING CHANGE]: update WorkflowAction API to expose access to Workflow instance values #349

Uh oh!

Conversation

jamieQ commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

square-tomb May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjohnson12 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

watt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jamieQ commented May 20, 2025 •

edited

Loading

square-tomb May 21, 2025 •

edited

Loading