[feat]: add runtime observation API #168

jamieQ · 2022-12-13T17:57:58Z

Issue

the Swift implementation of the Workflow library currently does not offer much support for consumers to tie into runtime operations in which they may be interested. by contrast, the Kotlin implementation has supported various facilities for this over time, and currently allows most runtime methods to be observed or even replaced by clients.

Description

adds a new type WorkflowSession that represents metadata about the node backing a particular Workflow 'instance' in the tree. this was largely modeled after the analogous concept in the Kotlin library. this type includes:
1. the type of the corresponding workflow
2. the string key used to render the workflow
3. an opaque identifier type (currently implemented via an underlying increasing global counter)
4. an optional reference to the parent node's session data
adds the WorkflowObserver protocol, which provides (optional) runtime hooks for the following:
1. session start/end (i.e. when a node is installed/removed from the tree)
2. when a Worfklow creates its initial state
3. before/after a Worfklow is rendered
4. when an existing Workflow is updated
5. when a WorkflowAction is received
6. before/after a WorkflowAction is applied to a Workflow's State
provides an internal WorkflowObserver implementation to multiplex observers provided by clients
adds/updates tests

Checklist

Unit Tests
~~UI Tests~~ N/A
~~Snapshot Tests (iOS only)~~ N/A
I have made corresponding changes to the documentation

amorde · 2022-12-13T18:24:01Z

Workflow/Sources/WorkflowObserver.swift

+    /// Marks the end of a `WorkflowSession`, indicating that the corresponding `WorkflowNode` has been removed from the tree of Workflows.
+    /// - Parameter session: The `WorkflowSession` that ended.
+    func sessionDidEnd(
+        _ session: WorkflowSession


it looks like a few of these don't have access to state, and none of them have access to the underlying workflow instance. is there an underlying limitation preventing that? I somewhat assumed WorkflowSession would contain some of those things at first glance but realized I was wrong after further reading

the session stuff in particular was largely modeled on the analogous concepts/interfaces in the kotlin repo.

i think i need a bit of clarification on this thought:

none of them have access to the underlying workflow instance

by 'underlying workflow instance', what do you mean exactly? in many of them the type conforming to Workflow is passed as a parameter for the observer to use.

is there an underlying limitation preventing that?

for the session lifecycle in particular, the state and corresponding workflow (conforming type instance) could be passed. though there is a question about how to order the sessionDidBegin and workflowDidMakeInitialState.

amorde · 2022-12-13T18:24:37Z

Workflow/Sources/WorkflowObserver.swift

+    ///   - action: The action that was received.
+    ///   - session: The `WorkflowSession` corresponding to the backing `WorkflowNode`.
+    func workflowDidReceiveAction<Action: WorkflowAction>(
+        _ action: Action,


can the workflow itself be passed in here, and its state?

it probably can – i think i had experimented with some variants of that, and seem to recall some issues that weren't straightforward to resolve, but i will take a look at it again.

i added the workflow back in, but getting the state available in this context seems a bit more difficult. i did add another hook for when the action is applied to the state, so perhaps that could be used instead. i'm inclined to leave it as-is for now, unless there's a clear reason we'd want to expose the state here in particular.

amorde · 2022-12-13T18:25:14Z

Workflow/Sources/WorkflowObserver.swift

+        let rawIdentifier: UInt64 = Self._makeNextSessionID()
+    }
+
+    private indirect enum IndirectParent {


out of curiosity, what drove the need for this?

structs and enums cannot contain stored properties that reference their type by default, but if you mark an enumeration case as 'indirect', it allows this form of recursion. originally i had this as a class, but swapped to the 'indirect enum case' (i.e. a custom optional implementation that allows recursion), mostly just to see if implementing this with the value types was possible.

IMO it is a bit unusual, but seems to work. though it might also make sense to remove the parent reference entirely if it doesn't seem valuable. it does let you walk from a given node up to the root though, which might be useful in some circumstances.

To make sure I understand, if you deleted the indirect keyword from the declaration of IndirectParent, it wouldn't compile?

I see that is true if some case of IndirectParent has an associated value of type IndirectParent, but I didn't realize that's true also if the associated value type is only mutually recursive with IndirectParent, like WorkflowSession is.

Yeah this is reasonable. Another typical workaround is to use a "box":

final class Box<T> { let value: T init(_ value: T) { self.value = value } } // ... let parent: Box<WorkflowSession>?

To make sure I understand, if you deleted the indirect keyword from the declaration of IndirectParent, it wouldn't compile?

@square-tomb that's right. if i remove it, i get the following compiler error at the declaration of the _indirectParent property:

Value type 'WorkflowSession' cannot have a stored property that recursively contains it

i assume this is because value types are to be stack-allocated, and so their entire layout must have a fixed size (or at least a bounded size?) so the compiler knows how to set up the function prologues appropriately. if a value type contains a recursive reference (anywhere in a descendant property), then you get this error, presumably because the type's layout size cannot be determined.

Another typical workaround is to use a "box"

@JaviSoto yep, and i believe that is essentially what's done internally when you mark an enum as indirect. i found this thread had some useful information and links on the topic.

Workflow/Sources/WorkflowObserver.swift

amorde · 2022-12-13T18:29:25Z

Workflow/Sources/WorkflowObserver.swift

+        renderKey: String,
+        parent: WorkflowSession?
+    ) {
+        self.workflowType = WorkflowType.self


any reason for not storing the workflow itself?

i was thinking of the session as like mostly fixed metadata about the 'node' that persists over time. the actual Workflow instance (that clients provide) that is associated with a given node can change over time, so if we stored that here we'd have to deal with that fact. also, this is largely modeled from the Kotlin implementation, which doesn't store the Workflow directly on this type (or, the 'props' as they're called in that context).

mjohnson12

Looks great to me

Workflow/Sources/SubtreeManager.swift

Workflow/Sources/WorkflowHost.swift

Workflow/Sources/WorkflowNode.swift

Workflow/Sources/WorkflowObserver.swift

square-tomb · 2023-01-10T18:34:04Z

Workflow/Sources/WorkflowObserver.swift

+        let rawIdentifier: UInt64 = Self._makeNextSessionID()
+    }
+
+    private indirect enum IndirectParent {


To make sure I understand, if you deleted the indirect keyword from the declaration of IndirectParent, it wouldn't compile?

I see that is true if some case of IndirectParent has an associated value of type IndirectParent, but I didn't realize that's true also if the associated value type is only mutually recursive with IndirectParent, like WorkflowSession is.

JaviSoto

This was super easy to follow, the approach makes a ton of sense, and the tests are fantastic. This is really great!

JaviSoto · 2023-01-10T18:36:27Z

Workflow/Sources/WorkflowAction.swift

@@ -37,6 +37,10 @@ public protocol WorkflowAction {
 public struct AnyWorkflowAction<WorkflowType: Workflow>: WorkflowAction {
    private let _apply: (inout WorkflowType.State) -> WorkflowType.Output?

+    /// Underlying type-erased `WorkflowAction` value, if it exists. Will be nil if the
+    /// action is defined by a closure. Primarily used for testing purposes.
+    let _wrappedValue: Any?


This could be

let _wrappedValue: (any WorkflowAction)?

But that requires Xcode 14, which we may not want to require in case users of the library haven't updated?

Of course given Xcode 14 we can also completely remove this type-erasure since you can now do any WorkflowAction<WorkflowType> (we'd have to add a default generic type to the protocol too)

Square is on Xcode 14.2 now. Can we make this change?

happy to do that, but that will be a breaking change which will require a major version bump, so would prefer to do it independently of this PR

Workflow is an open source repo and currently supports iOS 11+. Making it swift 5.7+ required is definitely a breaking change.

Ah, I kept forgetting that 👍
Alternatively, you could do it with #if compiler(), but I wouldn't recommend it unless we have CI set up to make sure everything builds under both Xcode 14.0 and 14.2

(Not to backseat code here; but shall we do the deployment target bump some time soon? It feels fine for folks still needing < 14 to just use an old version)

Workflow/Sources/WorkflowHost.swift

Workflow/Sources/WorkflowObserver.swift

JaviSoto · 2023-01-10T18:42:43Z

Workflow/Sources/WorkflowObserver.swift

+        let rawIdentifier: UInt64 = Self._makeNextSessionID()
+    }
+
+    private indirect enum IndirectParent {


Yeah this is reasonable. Another typical workaround is to use a "box":

final class Box<T> { let value: T init(_ value: T) { self.value = value } } // ... let parent: Box<WorkflowSession>?

Workflow/Tests/WorkflowObserverTests.swift

JaviSoto · 2023-01-10T19:00:48Z

Workflow/Tests/WorkflowObserverTests.swift

+            XCTAssert(observer?.observers.last is NoOpObserver)
+        }
+    }
+}


These are super thorough!! 👍👍👍

Workflow/Tests/WorkflowObserverTests.swift

jbmorgan · 2023-01-30T23:55:58Z

Workflow/Sources/WorkflowHost.swift

@@ -36,7 +36,7 @@ public final class WorkflowHost<WorkflowType: Workflow> {

    private let (outputEvent, outputEventObserver) = Signal<WorkflowType.Output, Never>.pipe()

-    private let rootNode: WorkflowNode<WorkflowType>
+    let rootNode: WorkflowNode<WorkflowType>


You mentioned concerns about potentially leaking some Workflow internals that could lead to Workflows hanging around longer than expected. Is this one of those places?

not this spot – this change was just to enable some unit tests

Sometimes I use this little trick to make it clearer why something that one may think should be private isn't, and also to emphasize that while it isn't private, it's also no public:

// @testable internal let rootNode: WorkflowNode<WorkflowType>

You could also leverage @_spi here to force someone to opt into the SPI interface to access this.

Workflow/Sources/WorkflowObserver.swift

* main: [fix]: use weak reference to internal sinks when vending to clients (#189) [feat]: add primary associated types to more protocols (#188) [chore]: pre major version bump cleanup (#187) [chore]: bump minimum deployment & swift versions (#186) Abstract ViewEnvironment to shared framework (#185) Bump activesupport from 6.1.4.4 to 6.1.7.1 (#183) [feat]: add primary associated types to `Workflow` protocol (#181) [chore]: update swiftformat ifdef indent rule to no-indent (#182)

…rvers * origin/main: [chore]: refactor some internal actions to use existential any (#190)

…ow-workflow-conformance * origin/main: Expose AnyScreen.wrappedScreen for inspection (#193) Improve introspection for SignalProducerWorkflow actions (#192) [release]: bump version to 2.2.0 & remove separate concurrency version (#191) [feat]: add runtime observation API (#168) [chore]: refactor some internal actions to use existential any (#190) [fix]: use weak reference to internal sinks when vending to clients (#189) [feat]: add primary associated types to more protocols (#188) [chore]: pre major version bump cleanup (#187) [chore]: bump minimum deployment & swift versions (#186) Abstract ViewEnvironment to shared framework (#185)

amorde reviewed Dec 13, 2022

View reviewed changes

Workflow/Sources/WorkflowObserver.swift Show resolved Hide resolved

amorde reviewed Dec 13, 2022

View reviewed changes

jamieQ force-pushed the jquadri/workflow-observers branch from 36e94e6 to cbb4981 Compare December 19, 2022 18:28

jamieQ changed the title ~~[WIP]: add runtime observation API~~ [feat]: add runtime observation API Jan 5, 2023

jamieQ marked this pull request as ready for review January 9, 2023 18:09

jamieQ requested a review from a team as a code owner January 9, 2023 18:09

mjohnson12 approved these changes Jan 9, 2023

View reviewed changes

square-tomb approved these changes Jan 10, 2023

View reviewed changes

JaviSoto reviewed Jan 10, 2023

View reviewed changes

[feat]: add runtime observation API

b745169

jamieQ force-pushed the jquadri/workflow-observers branch from c4aff5b to b745169 Compare January 17, 2023 18:51

possible interface for global observer injection

c6f9c73

jbmorgan reviewed Jan 30, 2023

View reviewed changes

jbmorgan reviewed Jan 31, 2023

View reviewed changes

Workflow/Sources/WorkflowObserver.swift Show resolved Hide resolved

jamieQ added 5 commits January 30, 2023 20:10

renaming & don't let internal sink escape

f651a5b

anticipate merge conflicts

d78c0ec

pr feedback

f1c00b6

more pr feedback

f92694c

jamieQ mentioned this pull request Feb 7, 2023

[chore]: refactor some internal actions to use existential any #190

Merged

4 tasks

jbmorgan approved these changes Feb 7, 2023

View reviewed changes

jamieQ added 5 commits February 7, 2023 16:40

Merge remote-tracking branch 'origin/main' into jquadri/workflow-obse…

6ed3c25

…rvers * origin/main: [chore]: refactor some internal actions to use existential any (#190)

streamline action notifications using new existential wrappers

beb861f

add another test for action obseration

3e79338

remove some now-unused parameters

f4d0022

and some more minor cleanup

42cd613

jamieQ merged commit 9fbed9a into main Feb 8, 2023

jamieQ deleted the jquadri/workflow-observers branch February 8, 2023 03:52

This was referenced Feb 22, 2023

Improve introspection for SignalProducerWorkflow actions #192

Merged

Expose AnyScreen.wrappedScreen for inspection #193

Merged

[feat]: add runtime observation API #168

[feat]: add runtime observation API #168

Uh oh!

Conversation

jamieQ commented Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Description

Checklist

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!

jamieQ Dec 13, 2022 • 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!

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!

mjohnson12 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JaviSoto left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jamieQ commented Dec 13, 2022 •

edited

Loading

jamieQ Dec 13, 2022 •

edited

Loading