Abstract ViewEnvironment to shared framework #185

n8chur · 2023-01-31T00:20:27Z

Overview

Environment value objects and corresponding propagation systems have become a useful tool in application architecture. Implementations of this tool can be found in SwiftUI, Blueprint, WorkflowUI, and the Market Design System's MarketEnvironment/MarketEnvironmentUI.

In building out Market, we've found that there is great utility in consolidating these environments, particularly when their conceptual domain for propagation directly overlaps. Workflow's ViewEnvironment and Market's MarketEnvironment is a great example of this—both define an environment value bucket which is propagated through UIViewController hierarchies.

Needing to manage two types that attempt to solve an extremely similar goal leads to a few ares of complexity:

Bridging

Bridging between the two propagation systems is more tedious than it need be since we're forced to acknowledge all properties we would like to bridge from one environment to the other at their boundaries (e.g. a vanilla UIViewController hierarchy that uses an environment propagation system which contains a child WorkflowHostingController that needs that environment information for theming).

It'd be much more convenient if we could just pass the entire environment along and avoid the need to build protocol abstractions over their shared API.

We could even eventually provide official support for automatically bridging between UIViewController and Workflow propagation of the environment within WorkflowUI (e.g. in WorkflowHostingController or a similar type), which would reduce a huge pain point and common mistake found in the ios-register codebase.

Generics

We use protocols to abstract similar concepts which are shared amongst our environments (e.g. the presence of a theme property, the ability to resolve a modal presentation style from an Environment, etc.). This leads to the use of generics upstream of these APIs, which adds unnecessary clutter when building features which intersect with these concerns.

There is a particular use-case with our Modals framework which would be drastically simplified and cleaned up if we could share an environment type for all use cases which a Modal presentation style could be resolved.

Code duplication

We currently duplicate all of our ViewEnvironment key conformances and extensions for use in MarketEnvironment. It'd be nice not to have to maintain both.

Why is abstracting `ViewEnvironment` to a separate framework important? Why not just `import WorkflowUI` and use it where this reuse would be convenient?

While Workflow/WorkflowUI is a fairly lightweight framework, we've had requests not to depend on all of Workflow/WorkflowUI in some contexts. We'd prefer the reliance on a dependency like Workflow/WorkflowUI not to be the blocking factor in adopting the Market design system. This abstracted ViewEnvironment framework will completely replace our existing MarketEnvironment framework, so binary size etc. should not be increased as a result of this change in those use cases.

Should we consider pulling `ViewEnvironment` out into a separate repository?

Probably! Just pulling this dependency out in Workflow was the easiest way to achieve our immediate needs for simplification so we decided to start here, but there may be utility in sharing ViewEnvironment in other contexts.

Checklist

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

This reverts commit 0e1ac17.

kyleve · 2023-01-31T00:24:40Z

Development.podspec

@@ -15,6 +15,7 @@ Pod::Spec.new do |s|
  s.dependency 'WorkflowRxSwift'
  # s.dependency 'WorkflowCombine' # TODO: Disabled because app specs cannot increase the deployment target of the root


Unrelated, but I bet we can turn these on globally now cc @jamieQ

I don't believe we can if this is to work around a bug in pod gen unless we're bumping all podspecs to iOS 13.

The project attempts to generate with an iOS 11 deployment target and fails to resolve all targets as a result (since these commented ones require iOS 14). I just reported the bug in #cocoapods here.

I figured we could bump stuff in the whole repo to 14 as well. Anyway, not important for this PR!

I'd expect that you'd be able generate a project with pod gen which ends up just using the highest deployment target found in your development podspecs, but it doesn't appear to work this way (it's either an order thing or choosing the lowest version I think).

looking into bumping the versions here. can do that after this is merged though & will update the values in the new podspec.

kyleve · 2023-01-31T00:24:57Z

ViewEnvironment.podspec

@@ -0,0 +1,22 @@
+require_relative('version')
+
+Pod::Spec.new do |s|


Any tests to move too?

I didn't see any but I thought about adding a simple .empty initialization test.

Not necessary here, but there is logical surface area to cover in the subscript and setting methods.

n8chur · 2023-01-31T00:32:32Z

I didn't realize a draft would tag folks for review—I'll be updating this PR with a description of what and why this change is desirable and open it for full review soon!

The TL;DR Is that we'd get some pretty huge benefit from being able to share the ViewEnvironment type in our vanilla, Workflow-less UIViewController propagation system we use to propagate theming information outside of Workflow in Market.

nononoah · 2023-01-31T14:56:57Z

ViewEnvironment.podspec

@@ -0,0 +1,22 @@
+require_relative('version')
+
+Pod::Spec.new do |s|


Not necessary here, but there is logical surface area to cover in the subscript and setting methods.

nononoah · 2023-01-31T15:01:11Z

WorkflowUI/Sources/ModuleExports.swift

@@ -0,0 +1 @@
+@_exported import ViewEnvironment


This is a compatibility gesture, right? Assuming that Foo refers to the same module, can two modules call export Foo without conflict?

My hunch is that it's fine, and that there's probably a lot of @_exported Foundation lurking around, for example, but I figured it worth asking.

This is a compatibility gesture, right?

Yup!

can two modules call export Foo without conflict?

I believe so

lickel · 2023-02-02T00:28:13Z

ViewEnvironment.podspec

+    # 1.7 is needed for `swift_versions` support
+    s.cocoapods_version = '>= 1.7.0'
+
+    s.swift_versions = ['5.0']


Nit: move these things into version.rb ?

Seems reasonable to me—but I'm going to leave it as for this PR to avoid conflating that change here.

watt · 2023-02-02T00:52:33Z

RELEASING.md

@@ -47,6 +47,7 @@ For Squares, membership is managed through the `Workflow Swift Owners` registry
    bundle exec pod trunk push WorkflowSwiftUI.podspec --synchronous
    bundle exec pod trunk push WorkflowCombine.podspec --synchronous
    bundle exec pod trunk push WorkflowCombineTesting.podspec --synchronous
+    bundle exec pod trunk push ViewEnvironment.podspec --synchronous


These are ordered by dependency — you'll have to push this one first (or at least before WorkflowUI).

Good catch, done!

jamieQ · 2023-02-02T13:06:08Z

Development.podspec

@@ -15,6 +15,7 @@ Pod::Spec.new do |s|
  s.dependency 'WorkflowRxSwift'
  # s.dependency 'WorkflowCombine' # TODO: Disabled because app specs cannot increase the deployment target of the root


looking into bumping the versions here. can do that after this is merged though & will update the values in the new podspec.

* 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)

…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)

n8chur added 3 commits January 30, 2023 16:11

Abstract ViewEnvironment framework

259138b

Bump deployment target to iOS 14

ebd8bc0

Revert "Bump deployment target to iOS 14"

f5e9e1b

This reverts commit 0e1ac17.

kyleve reviewed Jan 31, 2023

View reviewed changes

kyleve approved these changes Jan 31, 2023

View reviewed changes

Update releasing instructions

bd9dcba

Add ViewEnvironment to SPM

e410bc6

nononoah approved these changes Jan 31, 2023

View reviewed changes

Fix tutorial target

d8255ea

n8chur changed the title ~~[WIP][DNR][DNM] Abstract ViewEnvironment to shared framework~~ Abstract ViewEnvironment to shared framework Feb 1, 2023

n8chur marked this pull request as ready for review February 1, 2023 23:54

n8chur requested a review from a team as a code owner February 1, 2023 23:54

n8chur requested a review from bencochran February 1, 2023 23:56

lickel reviewed Feb 2, 2023

View reviewed changes

watt approved these changes Feb 2, 2023

View reviewed changes

jamieQ approved these changes Feb 2, 2023

View reviewed changes

Reorder ViewEnvironment pod publishing step in RELEASING docs

f69e7a7

n8chur enabled auto-merge (squash) February 2, 2023 20:53

n8chur disabled auto-merge February 2, 2023 20:53

n8chur enabled auto-merge (squash) February 2, 2023 20:53

n8chur merged commit f924e81 into main Feb 2, 2023

n8chur deleted the westin/abstract-view-environment branch February 2, 2023 20:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Abstract ViewEnvironment to shared framework #185

Abstract ViewEnvironment to shared framework #185

n8chur commented Jan 31, 2023 •

edited

Loading

kyleve Jan 31, 2023

n8chur Jan 31, 2023 •

edited

Loading

kyleve Jan 31, 2023

n8chur Jan 31, 2023 •

edited

Loading

jamieQ Feb 2, 2023

kyleve Jan 31, 2023

n8chur Jan 31, 2023

nononoah Jan 31, 2023

n8chur commented Jan 31, 2023

nononoah Jan 31, 2023

nononoah Jan 31, 2023

n8chur Jan 31, 2023

lickel Feb 2, 2023

n8chur Feb 2, 2023

watt Feb 2, 2023

n8chur Feb 2, 2023

jamieQ Feb 2, 2023

		@@ -15,6 +15,7 @@ Pod::Spec.new do \|s\|
		s.dependency 'WorkflowRxSwift'
		# s.dependency 'WorkflowCombine' # TODO: Disabled because app specs cannot increase the deployment target of the root

		@@ -0,0 +1,22 @@
		require_relative('version')

		Pod::Spec.new do \|s\|

Abstract ViewEnvironment to shared framework #185

Abstract ViewEnvironment to shared framework #185

Conversation

n8chur commented Jan 31, 2023 • edited Loading

Overview

Bridging

Generics

Code duplication

Why is abstracting ViewEnvironment to a separate framework important? Why not just import WorkflowUI and use it where this reuse would be convenient?

Should we consider pulling ViewEnvironment out into a separate repository?

Checklist

Choose a reason for hiding this comment

n8chur Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n8chur Jan 31, 2023 • edited Loading

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 Jan 31, 2023

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 Jan 31, 2023 •

edited

Loading

Why is abstracting `ViewEnvironment` to a separate framework important? Why not just `import WorkflowUI` and use it where this reuse would be convenient?

Should we consider pulling `ViewEnvironment` out into a separate repository?

n8chur Jan 31, 2023 •

edited

Loading

n8chur Jan 31, 2023 •

edited

Loading