[HOLD] Components system example

[staykids]

Here's an example of how we might pattern some of our new Components system work. Open up Demo.xcodeproj and give it a try in the Simulator.

I do not view any of this as final or definitive, but instead a conversation starter for how we collectively might move forward. I think some of it is good and some is bad, and I'd like to encourage us all to feel out what works and doesn't, what we'd like to keep and what we'd like to ditch. (Apologies that it isn't a particularly pretty app to look at 🤠)

To answer some of the specifics initially laid out in https://phabricator.wikimedia.org/T329329:

1. How do we theme a component?
   1. As automatically as we can! See WKAppEnvironment and the Views classes below. In the UIView based Components, I've used the idea of a reusable configure() method that a component is responsible for to restyle itself based on the current app environment.
2. How do we to apply fonts to a component?
   1. I think it feels good to define our font primitives somehow like is presented in WKFont. You'll notice some cases are using system text styles, and others are using custom metrics.
3. How will items 1 and 2 automatically update upon system dark mode change and dynamic type change?
   1. Transiting changes in the environment via the AppEnvironment class has been kind of nice and frictionless. In the client app, I created a new UIWindow class RootWindow that all the UI is placed within. As the trait collection changes, it passes those changes to AppEnvironment which then automatically publishes those changes to any object (view or controller) participating in the Components system.
4. How will we write a component for a UIKit context vs a SwiftUI context?
   1. I've included an example of purely UIView based approaches (like if we're inserting a UIView based Component into an existing UIViewController), a UIViewController vending approach, a UIHostingController/SwiftUI based approach, and an intermixed UIKit/SwiftUI based approach.
5. How will components be added to the view hierarchy? Where do responsibilities lie? (i.e. Canvas approach).
   1. Through iteration, it ultimately did feel useful to have some kind of base Canvas to lay components on to.

Classes

Components Framework

Environment

• WKAppEnvironment: An @ObservedObject to transit app environment data like the current theme or trait collection to all objects that subscribe to the Components system.

Views

• WKComponentView: A UIView that subscribes to the Components system. Published changes to the WKAppEnvironment are listened for and then communicated to subclasses via appEnvironmentDidChange().
• WKComponentViewController: A UIViewController that subscribes to the Components system.
• WKComponentHostingController: A UIHostingController (itself a UIViewController) for SwiftUI views that subscribes to the Components system.
• WKCanvas: This is a WKComponentView that automatically changes its background color when the theme changes.
• WKCanvasViewController: This is a UIKit based WKComponentViewController that includes some utility methods to quickly add Components to the canvas.

Style

• WKColor: Our color primitives used in the Components. This is equivalent to what we currently call the Wikimedia Style Guide Colors. Components themselves never use these directly, but instead just use WKTheme properties which reference these.
• WKFont: Our font primitives used in the Components. This is in a way similar DynamicTextStyle. Components use these primitives directly. By passing in the current trait collection, these fonts will scale to support the current category size.
• WKTheme: A struct that contains the WKColor properties per theme. Components use these properties directly when styling themselves.

Client app

• RootWindow: This is the base window the app's UI is contained within. When the trait collection changes, it updates the Component framework's WKAppEnvironment with the changes.
• ViewController: All the demo controllers are constructed here.

Demo app

The demo app includes 4 examples. Try using Xcode's environment override feature to switch between light/dark appearance and dynamic type size to see the changes spiral through the UI.

Buttons

This example adds WKComponent's directly to a UIView. This is a UIViewController that doesn't participate in the Components system itself, but adds Components that do participate in the system. As you use Xcode's environment overrides to switch between light/dark/dynamic text size, you'll see the WKButton Components themselves adapt to the environment but the base UIViewController view does not.

Top Read

This example adds a SwiftUI View to a WKCanvasViewController via a WKComponentHostingController. From the client app we pass in data.

Random Photo

This example uses a WKComponentViewController. This uses two random image APIs I found, so please forgive me if something NSFW pops up.

Say Hello

This example mixes both SwiftUI and UIView based Components into a UIKit based WKCanvasViewController. Typing out a message in the UITextField and tapping the button displays that message in the SwiftUI view via an observed data object.

Random Notes

• ✅ To me, using Combine and the RootWindow approach to communicate trait collection changes to WKAppEnvironment is nice.
• ✅ You'll notice how straightforward a lot of the client app is. If needed, there are cases where client app data is passed inwards (like top read), but for cases where that isn't needed like the Random Photo component, we don't even have to worry about hooking up buttons. I feel like this really does help make the Components more reusable, as depending on the nature of the component there's less knowledge the client app has to have to use it. The client app code becomes more navigation and interface layout focused.
• ✅ I think the pattern in the UIKit based WKComponentViews of having a one time layout setup() method, and a reusable configure() method that styles the component pattern is nice.
• ✅ Exposing internal API only as necessary (as seen in WKButton's addTarget(...) API which mimics UIButton's) is nice. True it involves essentially shadow rewriting API that the underlying UIView uses, but it feels great to be intentional about what a component is allowed to change without exposing the underlying implementation.
• ⚠️ It's annoying to prefix our classes with WK just because SwiftUI steals namespace for things we'd like to use like Font and Color, but as it's common to have to refer to both in SwiftUI View construction, it's better than typing SwiftUI.Font and SwiftUI.Color all the time to distinguish.
• ⚠️ There's some redundant messaging that happens when the WKAppEnvironment changes. For example, if a WKComponentView is hosted in a WKComponentViewController, both the component view and the controller are informed of the environment change. Not necessarily a bad thing, but just wanted to call it out.
• ⚠️ A way I tried to reduce some redundant messaging can be seen in WKAppEnvironment's set(...). Instead of directly setting the theme or traitCollection properties themselves, which will fire a publish event for each set, this coalesces changing both at the same time (which is likely the most common intent) into just one fired event. Personally, I think it's nicer to just set the properties directly and remove this complexity, but I think that should be guided by if we notice negative performance or not with excessive Component messaging.
• 🚨 Supporting both UIKit and SwiftUI based Components kinda threw a wrench into keeping things consistent.
• 🚨 Related to above, I don't think the general data model and delegation patterns I have exampled here are particularly consistent or good. It would be best if we determined a consistent, flexible enough for both UIKit/SwiftUI approach here.
• 🚨 It would be nice to be able to direct a developer into the setup() and configure() pattern more automatically in a UIKit based WKComponentView. I explored some protocol based approaches that may work, but ultimately took them out because they didn't yet resonate with me.

[tonisevener]

Awesome work! Here are my initial thoughts from a first pass.

1. It is really nice that we no longer have to pass around theme everywhere, or propagate the applyTheme calls into subviews. I suspect that alone will fix a great number of theming bugs that we currently have.
2. I like that you have separated logic as WKComponent{Suffix} classes containing app environment subscription logic and nothing more, and WKCanvas{Suffix} classes containing common constraint and theming logic.
3. I'm a little iffy about some of the app-side view controllers being so feature-specific. We definitely will need these types of view controllers that are capable of adding child/subview components to their view, but I worry about the intention of each becoming confusing. For example, the distinction between TopReadController and WKTopReadHostingController could feel blurry, and components-related logic may begin to slip into the app-side TopReadController. Not necessarily something to jump on though - we can wait and see how it plays out with a feature.

There's some redundant messaging that happens when the WKAppEnvironment changes.

I didn't test extensively, but I noticed whenever I changed a theme after the view first appeared, I get the exact calls to configure() that I would have expected. So I don't think you're blasting every view here with lots of reconfiguration calls.

One thing I did notice is that some of these views are not getting deallocated, which may account for it appearing like additional configuration calls are happening upon theme change (when really they are happening on memory-leaked views). So we should look into that. The WKButtons for example, do not call deinit, but they do when I set their superclass as a raw UIView. So maybe this has something do to with the Combine setup in WKComponentView.

[tonisevener]

I may be missing a clear benefit, but for this simple example I would avoid the additional feature-specific view controller layer with WKFeatureNavigationHostingController and instantiate it like:

let featureNavigationController = WKComponentHostingController(rootView: WKFeatureNavigationView(delegate: self))
addComponent(featureNavigationController, pinToEdges: true, respectSafeArea: true)

[tonisevener]

Minor but enums without cases always feel a bit weird to me. I'm open to keeping it if there's consensus though.

[staykids]

I can understand your viewpoint – there are times I also go back and forth between approaches. In the case of colors, I've generally always found the caseless enum approach has a nice balance in use.

• Namespacing: I mostly treat the enum as just a way to namespace the colors (which are just static properties that don't need to be computed like with our fonts). By going with an enum type, we can ensure the developer doesn't accidentally instantiate a Color as they would if it were a struct or class.

enum Color { ... }
❌ let color = Color()

struct Color { ... }
✅ let color = Color()

class Color { ... }
✅ let color = Color()

• More concise: The static properties on an enum approach is much more concise, in definition and in use, than making cases for each color.

enum Colors {
	static let systemBlack = UIColor(...)
	static let systemRed = UIColor(...)
}

textView.textColor = Colors.systemBlack

vs.

enum MyColor {
    case systemBlack
    case systemRed

    var rawValue: UIColor {
        switch self {
        case .systemRed: return UIColor(...)
        case .systemBlack: return UIColor(...)
        }
    }
}

textView.textColor = Colors.systemBlack.rawValue

We could certainly write something similar to WKFont's static func for(...) approach and use cases for colors as well if my approach here is unpopular, but I think we'd benefit from the more concise, namespace-only approach like above because our colors don't have the same computed requirements as our fonts. Totally open to discussion if there are different ideas here.

[tonisevener]

👍 Makes sense, thanks for the examples! I'm good with keeping them as case-less enums.

[tonisevener]

I'm a little iffy about some of the app-side view controllers being so feature-specific. We definitely will need these types of view controllers that are capable of adding child/subview components to their view, but I worry about the intention of each becoming confusing. For example, the distinction between TopReadController and WKTopReadHostingController could feel blurry, and components-related logic may begin to slip into the app-side TopReadController. Not necessarily something to jump on though - we can wait and see how it plays out with a feature.

I was thinking about this a bit more, and I think the additional app-side feature-specific view controller could be good to use as an additional hook for behavior that isn't rewritten into components yet. For example, if we have a WKWatchlistHostingController, which can do most everything except display a toast component for errors, it could delegate out the error to the app-side WatchlistController. Then the app-side WatchlistController can take that error and display it with our legacy WMFAlertManager.

[tonisevener]

One thing I did notice is that some of these views are not getting deallocated, which may account for it appearing like additional configuration calls are happening upon theme change (when really they are happening on memory-leaked views). So we should look into that. The WKButtons for example, do not call deinit, but they do when I set their superclass as a raw UIView. So maybe this has something do to with the Combine setup in WKComponentView.

FYI, I was able to easily fix the leaks in fc5c7a9. The fixes weren't a must for a simple demo app, but I wanted to make sure there weren't any problems here that would be difficult to solve later once we're building components.

With these leak fixes, I'm good to approve/merge whatever you need in this PR, or if you want to spin up a new one with just the base classes for merging, that would be fine too. Thanks!

[staykids]

I really appreciate you looking at this work!

Awesome work! Here are my initial thoughts from a first pass.
3. I'm a little iffy about some of the app-side view controllers being so feature-specific. We definitely will need these types of view controllers that are capable of adding child/subview components to their view, but I worry about the intention of each becoming confusing. For example, the distinction between TopReadController and WKTopReadHostingController could feel blurry, and components-related logic may begin to slip into the app-side TopReadController. Not necessarily something to jump on though - we can wait and see how it plays out with a feature.

I totally agree – I'd really like to figure out how to better encapsulate the internal types the components framework uses. Ideally, the components framework should only expose the highest level, useable by client app, types.

FYI, I was able to easily fix the leaks in fc5c7a9. The fixes weren't a must for a simple demo app, but I wanted to make sure there weren't any problems here that would be difficult to solve later once we're building components.

Nice – appreciate you locating the leak.

With these leak fixes, I'm good to approve/merge whatever you need in this PR, or if you want to spin up a new one with just the base classes for merging, that would be fine too. Thanks!

If we're all good with this pattern, I'll spin up a new one with just the base stuff we need for merging without all the demo work.

[staykids]

Closing, but leaving branch up.