[feat]: Add support for automatic ViewEnvironment bridging in WorkflowUI #211

n8chur · 2023-05-11T19:32:56Z

Note: This PR is targeting the feature/viewenvironmentui branch and depends on #205.

Overview

This PR adds deep integration of ViewEnvironmentUI to WorkflowUI. This deep integration makes working with contexts that need to bridge between both systems much easier. For example, applications that have not fully adopted Workflow but utilize the ViewEnvironment for theme propagation via ViewEnvironmentUI (as is true in Square's ios-register codebase).

This PR is part of the larger effort to add support for automatic ViewEnvironment bridging between UIKit propagation and WorkflowUI propagation within WorkflowUI.

See the associated proposal for more information.

Changes

This PR integrates ViewEnvironmentUI vanilla UIKit ViewEnvironment propagation support into WorkflowUI. This includes:

Automatic bridging of the ViewEnvironment from UIViewController propagation into Screen's ViewEnvironment within WorkflowHostingController.
Automatic bridging of the ViewEnvironment from Screen's ViewEnvironment into UIViewController propagation within ViewControllerDescription.

With these new features came some minor API adjustments:

ViewControllerDescription now takes in a ViewEnvironment as a parameter. This environment is added into an ancestor propagation node above the described view controller so that it can be served to any UIViewController ViewEnvironment queries (performed via ViewEnvironmentUI's propagation system).
WorkflowHostingController now takes a customizeEnvironment closure instead of a rootViewEnvironment. This makes it possible to configure environment properties as the ViewEnvironment is bridged into the Screen hierarchy without fully overwriting the entire ViewEnvironment.
ScreenViewController no longer takes an environment in its update(...) function. The Environment is now managed by the ViewControllerDescription for the Screen that it's backing. The environment parameter is still present in the initializer for ScreenViewController as it can be convenient to have valid environment state before the ancestor propagation node is configured above it (which is configured after initialization completes).

Square Integration PRs

https://github.com/squareup/market/pull/6224
https://github.com/squareup/ios-register/pull/85648

Checklist

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

kyleve · 2023-05-18T19:47:16Z

WorkflowUI/Sources/Screen/ScreenViewController.swift

@@ -42,11 +44,11 @@ open class ScreenViewController<ScreenType: Screen>: UIViewController {
        return ScreenType.self
    }

-    public private(set) final var environment: ViewEnvironment
+    private var previousEnvironment: ViewEnvironment


Do we need to store this at all? I don't see it being used

It's being used in update(screen:) below.

…ronment property

…ndering's backing VCs

watt

Mostly bikesheds 👍

watt · 2023-05-18T23:25:10Z

WorkflowUI/Sources/Hosting/WorkflowHostingController.swift

+        super.viewWillLayoutSubviews()
+        applyEnvironmentIfNeeded()
+    }
+
    override public func viewDidLayoutSubviews() {
        super.viewDidLayoutSubviews()
        rootViewController.view.frame = view.bounds


Unrelated but we should move this to viewWillLayoutSubviews

I'm fairly confident we should but I was nervous to make this change as part of this work.

Let me know if you think it's worth doing now!

Nah, it's less risky to isolate that to a separate change. Just wanted to flag it.

WorkflowUI/Sources/Hosting/WorkflowHostingController.swift

watt · 2023-05-18T23:32:57Z

WorkflowUI/Sources/Screen/ScreenViewController.swift

@@ -42,11 +44,11 @@ open class ScreenViewController<ScreenType: Screen>: UIViewController {
        return ScreenType.self
    }

-    public private(set) final var environment: ViewEnvironment
+    private var previousEnvironment: ViewEnvironment


bikeshed: lastEnvironment or latestEnvironment? It's not exactly "previous" until an update happens.

It's already built into the public API as previousEnvironment in screenDidChange(from:,previousEnvironment:). I'd rather not change the public API and I think it's nice that this variable name matches the public API parameter it's it exists to support.

That being said, I don't feel super strongly! Let me know if you do.

It's already built into the public API as previousEnvironment in screenDidChange(from:,previousEnvironment:). I'd rather not change the public API and I think it's nice that this variable name matches the public API parameter it's it exists to support.

OK, so, my reasoning is that it's not the "previous" environment while it's being stored — it becomes the previous one during an update, when we read a new environment to replace it. Until then, it's just the latest one we observed. Not a big deal though.

WorkflowUI/Sources/ViewControllerDescription/ViewControllerDescription.swift

watt · 2023-05-18T23:49:29Z

WorkflowUI/Sources/ViewControllerDescription/ViewControllerDescription.swift

+        // We must nil this out first or we'll hit an assertion which protects against overriding the ancestor when
+        // some other system has already attempted to provide an override.
+        viewController.environmentAncestorOverride = nil


Alternatively, we could make PropagationNode a class and update the same instance. I'm not sure if there's a performance concern to updating the override on every render due to the associated object.

Yeah this is an optimization I was talking through with @bencochran. You think it's worth doing now?

Yeah, I think I'd lean towards just doing it now.

Please re-review if/when you have time 🙏

WorkflowUI/Sources/ViewControllerDescription/ViewControllerDescription.swift

WorkflowUI/Tests/WorkflowHostingControllerTests.swift

jamieQ · 2023-05-23T17:56:53Z

WorkflowUI/Sources/Hosting/WorkflowHostingController.swift

 import Workflow

 /// Drives view controllers from a root Workflow.
 public final class WorkflowHostingController<ScreenType, Output>: UIViewController where ScreenType: Screen {
+    public typealias CustomizeEnvironment = (inout ViewEnvironment) -> Void


should this alias be nested under the generic parent type? the fully-specified spelling will be parametrized by the screen/output, which might be unwieldy. then again, maybe nobody will need to reference it in that manner?

Yeah, I'd be surprised if we ever seen folks referencing the type directly in the context of WorkflowHostingController usage.

I can move it out if we'd prefer though! There are a few opportunities in Market to re-use this type if it was at the top level. Let me know if you'd prefer we do move it out, and if so, what to name it/where to put it!

your call to relocate/rename if you think it makes sense – just a thought!

n8chur force-pushed the westin/viewenvironmentui-auto-bridge branch 2 times, most recently from 9ce7fc5 to febb569 Compare May 16, 2023 20:18

n8chur changed the title ~~[DNR WIP] [feat] Add support for automatic ViewEnvironment bridging in WorkflowUI~~ [feat] Add support for automatic ViewEnvironment bridging in WorkflowUI May 16, 2023

n8chur mentioned this pull request May 16, 2023

[feat]: add ViewEnvironmentUI module #205

Merged

4 tasks

n8chur force-pushed the westin/viewenvironmentui-auto-bridge branch 3 times, most recently from 21936dc to 8f096c2 Compare May 18, 2023 17:16

n8chur changed the title ~~[feat] Add support for automatic ViewEnvironment bridging in WorkflowUI~~ [feat]: Add support for automatic ViewEnvironment bridging in WorkflowUI May 18, 2023

kyleve reviewed May 18, 2023

View reviewed changes

kyleve approved these changes May 18, 2023

View reviewed changes

n8chur force-pushed the westin/viewenvironmentui-auto-bridge branch from 8f096c2 to 0f8083a Compare May 18, 2023 22:44

Base automatically changed from westin/viewenvironmentui to feature/viewenvironmentui May 18, 2023 22:58

n8chur mentioned this pull request May 18, 2023

[feat]: add ViewEnvironmentUI and support automatic bridging in WorkflowUI #215

Merged

4 tasks

n8chur added 6 commits May 18, 2023 16:02

Add ViewEnvironmentUI as a dependency to WorkflowUI and module export it

b4c43b4

Add automatic bridging to WorkflowHostingController

f1307be

Add automatic bridging support to ViewControllerDescription

dac25a9

Update ScreenViewController's API to support ViewEnvironmentUI's envi…

dfc680d

…ronment property

Prevent duplicate environment updates in WorkflowHostingController re…

2671db3

…ndering's backing VCs

Test adapted environment support in WorkflowHostingControllerTests

34595b1

n8chur force-pushed the westin/viewenvironmentui-auto-bridge branch from 0f8083a to 34595b1 Compare May 18, 2023 23:02

watt approved these changes May 19, 2023

View reviewed changes

n8chur added 6 commits May 19, 2023 09:42

Add doc comment to customizeEnvironment on WorkflowHostingController

f960ead

Rename configureAncestor parameter from 'from' to 'of'

2b7cf56

Rename establishOverride method

925614c

Add comment about node ownership in ViewControllerDescription

3e9675c

Remove duplicate Key in name for testing type

a5b9e72

Reuse PropagationNodes when possible

e437c72

watt approved these changes May 20, 2023

View reviewed changes

n8chur added 2 commits May 22, 2023 09:59

Fix SPM support

05f0265

Fix tutorials target

a9c6736

n8chur marked this pull request as ready for review May 22, 2023 17:12

n8chur requested review from a team as code owners May 22, 2023 17:12

square-tomb approved these changes May 22, 2023

View reviewed changes

jamieQ reviewed May 23, 2023

View reviewed changes

n8chur mentioned this pull request May 23, 2023

[feat]: merge ViewEnvironmentPropagatingObject into ViewEnvironmentPropagating #218

Merged

4 tasks

n8chur merged commit f52fe53 into feature/viewenvironmentui May 31, 2023

n8chur deleted the westin/viewenvironmentui-auto-bridge branch May 31, 2023 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feat]: Add support for automatic ViewEnvironment bridging in WorkflowUI #211

[feat]: Add support for automatic ViewEnvironment bridging in WorkflowUI #211

n8chur commented May 11, 2023 •

edited

Loading

kyleve May 18, 2023

n8chur May 18, 2023

watt left a comment

watt May 18, 2023

n8chur May 19, 2023

watt May 19, 2023

watt May 18, 2023

n8chur May 19, 2023

watt May 20, 2023

watt May 18, 2023

n8chur May 19, 2023

watt May 19, 2023

n8chur May 19, 2023

n8chur May 19, 2023

jamieQ May 23, 2023

n8chur May 23, 2023

jamieQ May 23, 2023

[feat]: Add support for automatic ViewEnvironment bridging in WorkflowUI #211

[feat]: Add support for automatic ViewEnvironment bridging in WorkflowUI #211

Conversation

n8chur commented May 11, 2023 • edited Loading

Overview

Changes

Square Integration PRs

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

watt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n8chur commented May 11, 2023 •

edited

Loading