Consider de-emphasizing `ViewRegistry`.

[rjrjr]

@davidapgar just caught me up on a conversation that's been happening on the Kotlin side: why not get rid of ViewRegistry?

In both the Swift and Kotlin implementations, ViewRegistry is our last remaining source of runtime errors. Meanwhile, the indirection it provides is probably redundant with that already provided by having no restrictions on the workflow rendering type.

Swift already has a Screen marker interface, with a method that makes a screen responsible for instantiating its own ViewController. Isn't that enough?

• Android could just as easily have a Screen.buildView interface / method. That should still work nicely with the new LayoutRunner and ViewShowRendering code introduced in #383 .
• Headless workflows aren't forced to use it as their rendering type
• It'd be easy enough to use mapRendering, or provide a mapViewController method

The only downside we see is that a workflow's view classes become a public concern. That doesn't seem like much of an issue for internal code.

For public code, e.g. a workflow-based library that might want to allow clients to bind its own view code and for which "default" view classes could be a wart, ViewRegistry could be revived as a tool for use within a root workflow. That's practically what's happening inside WorkflowLayout / ContainerViewController already.

[bencochran]

I played with a typing of this idea for Swift a while back and came up with this thought for that side of the world:

public protocol Screen {
    associatedtype ViewControllerType: ScreenViewControllerProtocol where ViewControllerType.ScreenType == Self
}

extension Screen {
    public func provideViewController() -> ViewControllerType {
        return ViewControllerType(screen: self)
    }
}

public protocol ScreenViewControllerProtocol: UIViewController {
    associatedtype ScreenType
    init(screen: ScreenType)
    func update(screen: ScreenType)
}

open class ScreenViewController<ScreenType>: UIViewController, ScreenViewControllerProtocol {
    public required init(screen: ScreenType) {
        super.init(nibName: nil, bundle: nil)
    }

    @available(*, unavailable)
    public required init?(coder aDecoder: NSCoder) { fatalError() }

    open func update(screen: ScreenType) {
    }
}

I like this idea quite a bit. One of my points of pain with the ViewRegistry and Swift is that I can’t make a generic screen like LoadingScreen<Wrapping: Screen> because I would have to register a LoadingScreen<MyFeatureScreen> for every screen I wanted to use wrapped in LoadingScreen.

With this change I could keep my LoadingViewController and LoadingScreen generic (which allows my workflow to render a non-erased type, making mapRendering have enough information to do something useful).

[rjrjr]

Your ScreenViewControllerProtocol looks very similar to what's going on in the ViewShowRendering.kt extensions introduced in #383 . Good sign.

[rjrjr]

@bencochran Why bother with the extension? If we really go for it, having the Screen protocol force a provideViewController method to be implemented seems like the whole point.

public protocol Screen {
    associatedtype ViewControllerType: ScreenViewControllerProtocol where ViewControllerType.ScreenType == Self

    public func provideViewController() -> ViewControllerType {
        return ViewControllerType(screen: self)
    }
}

[rjrjr]

And for that matter: what's the benefit of the associated type for the `ViewController?

public protocol Screen {
    public func provideViewController() -> ViewController 
}

[davidapgar]

WRT the associatedtype of ViewController, you have to do that for generics on protocols in Swift (so we have type safety)

[bencochran]

Yeah, we need the associated type so that we can update the resulting view controller with a new screen. We could theoretically remove that need by doing something like

public protocol Screen {
    func provideViewController(updates: Signal<Self, NoError>) -> UIViewController
}

But then every implementor has to do the ReactiveSwift plumbing, and … no thanks.

The extension’s necessary to provide a default implementation. There’s been talk of cleaning it up to look like you’ve written, but not in the language yet.

[rjrjr]

But then every implementor has to do the ReactiveSwift plumbing, and … no thanks.

Totally. That's exactly what #383 eliminates for Android devs.

[rjrjr]

One important counter argument that might torpedo this for Android is our (probably terrible) habit of sometimes injecting dependencies into our ViewController equivalents. ViewRegistry works really well with that. I suspect that making Screen.buildView injection friendly could lead to quite a mess.

[zach-klippenstein]

I mean, presumably it has access to the Context, so they could fall back to using Components as service locators.

[rjrjr]

I mean, presumably it has access to the Context, so they could fall back to using Components as service locators.

Yeah, I keep going back and forth on whether that would be such a bad thing -- it's a pretty terrible habit anyway, forcing the code that does it to be more terrible might be a good signal.

[bencochran]

Was chatting with @davidapgar about this again this afternoon and realized another approach Swift could take, somewhat out of Blueprint/SwiftUI’s book:

protocol Screen {
    associatedtype ViewControllerType: UIViewController

    func makeViewController() -> ViewControllerType
    func update(viewController: ViewControllerType)
}

And for convenience’s sake we could still provide a base view controller:

open class ScreenViewController<ScreenType>: UIViewController{
    public required init(screen: ScreenType) {
        super.init(nibName: nil, bundle: nil)
    }

    @available(*, unavailable)
    public required init?(coder aDecoder: NSCoder) { fatalError() }

    open func update(screen: ScreenType) {
    }
}

extension Screen where ViewControllerType: ScreenViewController<Self> {

    func makeViewController() -> ViewControllerType {
        return ViewControllerType(screen: self)
    }

    func update(viewController: ViewControllerType) {
        viewController.update(screen: self)
    }

}

A nice thing about this is that allows a Screen to use whatever view controller type it wants without the need for the protocol (and especially, doesn’t require a specific initializer).

[rjrjr]

I worry about this making the UI / logic boundary just blurry enough that we lose that natural separation of concerns that's been such a big win so far. Right now it's basically impossible for a workflow to monkey around with view instances and play dirty tricks. That ends the moment the rendering type has a method like makeViewController. And we know very well that anything which can be abused will be abused, nearly instantly.

[rjrjr]

Once we resolve this, we should also take a pass at making our conventions more consistent. Issues that I'm aware of:

• Swift screens implement a Screen marker interface, and have an extension method on it that produces the ViewController. (Is this related to #553?) Can / should Kotlin do the same, or is this a language difference intruding?
• Swift convention is to embed a private ViewController implementation in the Screen file, Kotlin's are separate. I don't think there's any good reason for them to differ.
• Swift screenDidChange and Kotlin showRendering are the same method, and should have the same name. I prefer the latter b/c renderings can be much finer grained that screens.

[zach-klippenstein]

renderingDidChange? I'm not sure I'd want to use that exact name in Kotlin, the xDidY convention is very Apple-y and would seem out of place in Kotlin. But we usually map these to onXYed names, so onRenderingChanged?

[timdonnelly]

I worry about this making the UI / logic boundary just blurry enough that we lose that natural separation of concerns that's been such a big win so far. Right now it's basically impossible for a workflow to monkey around with view instances and play dirty tricks. That ends the moment the rendering type has a method like makeViewController. And we know very well that anything which can be abused will be abused, nearly instantly.

There's nothing stopping me from passing live view instances around in my view models (screens) today, though. An enterprising developer may find that makeViewController provides a more obvious mechanism with which to circumvent the rules, but we don't actually give up any safety.

The concrete benefit of keeping the registry is that we can build entire chunks of the logic tree with no UI dependencies. By bringing knowledge of UI types into the protocol, we lose that strong separation in the graph.

I'm of the (weak) opinion that the ergonomic win of dropping the view registry is worth it, FWIW.

[dhavalshreyas]

Catching up on this discussion. I believe the biggest win from ViewRegistry is the run-time Screen -> ViewController bindings we can provide. Possibly allowing us to map the same Screen to different ViewControllers based on where it's used.

However, I'm wondering if this makes it easy to introduce some non-obvious bugs like this one:

struct CoolScreen: Screen {}

class CoolViewController: ScreenViewController<CoolScreen> {}
class SweetViewController: ScreenViewController<CoolScreen> {}

func register() {
    var viewRegistry = ViewRegistry()
    viewRegistry.register(screenViewControllerType: CoolViewController.self)
    viewRegistry.register(screenViewControllerType: SweetViewController.self)
}

This becomes slightly more dangerous when view registries are merged.

[dhavalshreyas]

Considering☝️ and being able to trim down on number of new concepts by one, my vote would be to remove ViewRegistry.

[rjrjr]

The fragment you posted would crash with a runtime exception on Android, not a very realistic concern. (Is that not the case in iOS? We should reconcile.)

Removing ViewRegistry is not an option. We already have use cases that rely on such a mechanism, especially on Android. (Happy to discuss offline.) The best we could do is de-emphasize it, and perhaps distribute it as a separate library.

But I continue to be wary about making it so much easier to blur the lines between logic and view concerns. Sure, it's possible to do so today, but you have to make an effort. Having that pattern as the only practical approach is much riskier.

[rjrjr]

I would also point out that there a problems solved by ViewRegistry's ContainerHints that would need to be addressed in a new way.

Well — I suppose I'm really reminding us that we will still need ScreenViewController / LayoutRunner.

[timdonnelly]

But I continue to be wary about making it so much easier to blur the lines between logic and view concerns

Can you elaborate on this? Having knowledge of a type is very different from having runtime access to a specific instance of that type, which is why this concern doesn't resonate strongly with me. We're already building workflows with specific UI implementations in mind (which is expected and reasonable), so it seems surprising that we are not using common patterns in Swift and Kotlin to establish that relationship.

[rjrjr]

It's a Swift v. Kotlin thing! Again. Just had a good chat with @zach-klippenstein @dhavalshreyas and @JustinDSN, and was reminded that Swift extensions are able to make existing types implement new protocols.

Kotlin can't do that (yet? https://github.com/Kotlin/KEEP/pull/87), so if we want to enforce something like, say, the root workflow rendering type must implement the ScreenViewControllerFactory protocol — it actually has to implement it.

Swift can have its cake (every bit of binding flexibility, and decoupling of model and view concerns, that it has today) and eat it it too (compile time safety). Kotlin has to choose.

I think this is going to resolve like Kotlin's PropsT thing, one more mechanism that Kotlin devs have to learn and Swift devs are shielded from.

[JustinDSN]

Completed via #974