Turbo navigator without session #74

olivaresf · 2023-11-02T02:21:48Z

Experimenting with breaking apart TurboNavigator into two classes: TurboNavigator (which handles session logic) and TurboNavigationHierarchyController (which handles UIKit nav stack manipulation).

…HierarchyController)

joemasilotti · 2023-11-02T02:53:54Z

Exciting! Is the goal to figure out an approach to #69 or code quality?

olivaresf · 2023-11-02T04:11:53Z

This is an approach to #69!

* Ensure one is always set * Expose an open, default implementation * Implement optional methods in extension

joemasilotti · 2023-11-07T19:29:22Z

This is looking great! I just pushed some changes that:

Ensures presenting modals on alerts work by exposing TurboNavigationHierarchyController .activeNavigationController
Reworks TurboNavigatorDelegate to provide a default implementation with implemented functions in an extension. This also removes the need to check for nil.
Ensures WKUIDelegate is set up instantiation. But I left a comment because I don't love this approach.
Adds a convenience initializer to pass in a PathConfiguration (only) and have TurboNavigator set up the Session instances
Copies over cookies after each web request completes

joemasilotti · 2023-11-07T19:30:05Z

Sources/TurboNavigator.swift

-    public var rootViewController: UIViewController { navigationController }
-    public let navigationController: UINavigationController
-    public let modalNavigationController: UINavigationController
+        defer { self.webkitUIDelegate = TurboWKUIController(delegate: self) }


defer is needed to trigger the didSet callback above. I don't like this but can't think of another way without duplicating the logic in that callback.

I don't mind duplicating the logic. It really should just be about setting the delegate to both sessions and that's what we're doing here already. I don't feel strongly about it, though.

joemasilotti · 2023-11-07T19:31:23Z

Sources/TurboNavigationHierarchyController.swift

+        navigationController.presentedViewController != nil ? modalNavigationController : navigationController
+    }
+
+    var animationsEnabled: Bool = true


Why does this exist? I don't see a way to set it publicly as a consumer of the library. If anything, I'd prefer to see this live in the path configuration somehow so individual links can be customized instead of a global option.

Ha. That was a hack. There's no way to pass along a type of UINavigationController into this class anymore, so testing was broken. What I did was create this variable and keep it internal so consumers cannot use it but unit tests can reach it.

Ah, nice catch! What about exposing the navigation controllers in the initializer but defaulted like we had before?

init(delegate: TurboNavigationHierarchyControllerDelegate, navigationController: UINavigationController = UINavigationController(), modalNavigationController: UINavigationController = UINavigationController()) { self.delegate = delegate self.navigationController = navigationController self.modalNavigationController = modalNavigationController }

Yeah, I think that's the way forward. I originally didn't go with this because I didn't want to expose them, but since this class is now internal, I don't see any issues at all.

for authentication!

zoejessica · 2023-11-08T11:52:37Z

Sources/TurboNavigator.swift

+    public var webkitUIDelegate: TurboWKUIController? {
+        didSet {
+            session.webView.uiDelegate = webkitUIDelegate
+            modalSession.webView.uiDelegate = webkitUIDelegate
+        }


Oh should we make this public var webkitUIDelegate: WKUIDelegate? instead here? Otherwise I think we run into the issue @joemasilotti raised here about having to keep our TurboWKUIController in step with the WKUIDelegate protocol that we don't control, because subclasses can't implement protocol methods that aren't in the superclass.

And it'd be nice for users to be able to opt out of the alerting behaviour if they want, by supplying their own WKUIDelegate.

Perhaps we could set the default TurboWKUIController up as part of the convenience init on TurboNavigator, if the intention there is to provide the easiest route for set up 🤔?

Great catch! One caveat is that TurboWKUIController is a class, not a protocol. So a developer can subclass it to customize alert/confirm behavior or add new functionality.

But doing so is a little awkward because it needs a reference back to TurboNavigator:

self.turboNavigator = TurboNavigator(pathConfiguration: self.pathConfiguration) self.turboNavigator.webkitUIDelegate = WKUIController(delegate: self.turboNavigator) class WKUIController: TurboWKUIController { // Override functions to customize behavior, presenting alerts on `delegate`. }

If we expose a generic WKUIDelegate then I don't see how a developer can grab a reference back to the TurboWKUIDelegate needed to actually do the alert presenting!

Here's the diff I'm working with to allow subclassing:

diff --git a/Sources/TurboWKUIDelegate.swift b/Sources/TurboWKUIDelegate.swift index 94eb1d4..f14ebba 100644 --- a/Sources/TurboWKUIDelegate.swift +++ b/Sources/TurboWKUIDelegate.swift @@ -5,9 +5,10 @@ public protocol TurboWKUIDelegate : AnyObject { func present(_ alert: UIAlertController, animated: Bool) } -public class TurboWKUIController : NSObject, WKUIDelegate { - weak var delegate: TurboWKUIDelegate? - init(delegate: TurboWKUIDelegate!) { +open class TurboWKUIController: NSObject, WKUIDelegate { + public private(set) weak var delegate: TurboWKUIDelegate? + + public init(delegate: TurboWKUIDelegate!) { self.delegate = delegate }

Ohh that's a good point about the delegate. Would wrapping like this work do you reckon? So you'd be able to pass in either a subclassed TurboWKUIController or your own WKUIDelegate and have either be accessible from the outside?

public enum WKUIDelegateType { case standardBehaviour(TurboWKUIController) case custom(WKUIDelegate) var delegate: WKUIDelegate { switch self { case .custom(let delegate): return delegate case .standardBehaviour(let controller): return controller } } } ... public var webkitUIDelegate: WKUIDelegateType? { didSet { session.webView.uiDelegate = webkitUIDelegate?.delegate modalSession.webView.uiDelegate = webkitUIDelegate?.delegate } } ... // then in the init method: defer { self.webkitUIDelegate = .standardBehaviour(TurboWKUIController(delegate: self)) }

Oh should we make this public var webkitUIDelegate: WKUIDelegate? instead here?

I went with a class implementing WKUIDelegate so users could subclass it and override any protocol function in WKUIDelegate. If we explosed WKUIDelegate we leave all the responsibility of catching and building alerts to the consumer. We should definitely allow that flexibility, just not as a default. TurboWKUIController is the default.

Having said that...

subclasses can't implement protocol methods that aren't in the superclass.

This is an interesting idea! I see WKUIDelegate is an Obj-C protocol, so your observation is great: what happens if TurboWKUIDelegate does not conform to an optional method? Can users override it? I actually didn't know if this was true or not so I just tested it in a Playground and I don't think it's entirely true.

import UIKit @objc protocol TestProtocol { @objc optional func doSomething() @objc optional func saySomething() } class MyClassA : TestProtocol { func doSomething() { print("Do A") } } class MyClassB : MyClassA { override func doSomething() { print("Do B") } func saySomething() { print("Say") } } let classA = MyClassA() let classB = MyClassB() classA.doSomething() classA.saySomething?() // ❌ Value of type 'MyClassA' has no member 'saySomething' classB.saySomething() classB.doSomething()

*Note that MyClassB.doSomething is overridden but MyClassB.saySomething is not.

If you comment out classA.saySomething?() the code compiles and behaves as expected. This is good news as it means we don't actually need to implement all methods in WKUIDelegate. We can conform to however many we want, and users could just conform to functions that we don't implement.

Ohh that's a good point about the delegate. Would wrapping like this work do you reckon? So you'd be able to pass in either a subclassed TurboWKUIController or your own WKUIDelegate and have either be accessible from the outside?

I don't think I'm understanding the idea here. Why are we adding another layer of indirection via the enum? In my mind, a user would just subclass TurboWKUIDelegate and just pass their instance as the controller. As soon as it's passed, it's set as the WKUI delegate.

If the issue is the default, we can just have TurboNavigator build its own TurboWKUIDelegate.

I just pushed two commits that refactor both delegates a bit. I think this should get us everything we need. Check out the ComplexSceneDelegate at the bottom of SceneDelegate.swift.

The changes move the default implementation of TurboNavigatorDelegate to a protocol extension. Which means developers only have to implement the functions they want.

And makes TurboWKUIController open for subclassing, allowing a developer to customize only what they need.

I'm pretty happy with this! And I think ComplexSceneDelegate shows how it all works together. Let me know if I missed something or drifted off target.

@olivaresf Ohhhh you're totally right, apologies for the detour - I'd got mixed up with another subclassing/protocol scenario which I'll no doubt bore you with over coffee sometime 🙇‍♀️

@joemasilotti The new commits look great – I'm with Fernando, let's start to upstream and get real 😃

olivaresf · 2023-11-08T19:49:19Z

Sources/TurboNavigator.swift

+    /// - Parameters:
+    ///   - pathConfiguration:
+    ///   - delegate: an optional delegate to handle custom view controllers
+    public convenience init(pathConfiguration: PathConfiguration, delegate: TurboNavigatorDelegate? = nil) {


I really, really like this initializer. I think it's now very clear that the only required dependency for TurboNavigator is the path configuration.

Building on that… I don't think it is unreasonable for us to provide a default path configuration if one isn't provided. One that handles the required routing, only.

olivaresf · 2023-11-08T19:52:44Z

Demo/Demo/SceneDelegate.swift

+    func scene(_ scene: UIScene, willConnectTo session: UISceneSession, options connectionOptions: UIScene.ConnectionOptions) {
+        guard let windowScene = scene as? UIWindowScene else { return }
+
+        TurboConfig.shared.userAgent += " CUSTOM STRADA USER AGENT"


Definitely something to consider later, but I think this is the last piece of the puzzle that I can't make fit. I don't like the static configuration. I know Android has a WebView subclass due to technical constraints. Maybe we can make it work somehow.

Agreed. This static configuration made more sense (to me) before Strada existed and I was only setting the user agent. Honestly, we could even rip this out of turbo-ios (i.e. never upstream it) and force folks to use the Session initializer if they want to customize the web view. But I'll see how that plays out in the upstream PR.

Note that we have an instance in HEY where our composer's WebView is separate from the rest of the app and does not support the same Strada components as the rest of the app, so the user-agent is actually different for that webview. So we should support continue to support situations where the apps can set the user agent per WebView/Session — but that doesn't need to be the default path for simpler apps.

On the Android side, we just notify the app see whenever the session is (re)created and can update the user agent directly for the webView: https://github.com/hotwired/turbo-android/blob/main/demo/src/main/kotlin/dev/hotwire/turbo/demo/main/MainSessionNavHostFragment.kt#L49

Agreed. Both the user agent and the web view subclass should be supported. I was just saying there should be a better way of achieving both goals than a static configuration.

olivaresf · 2023-11-08T19:54:32Z

I think we're in a great spot to get this upstreamed to Turbo. I think it was a great idea to have a SceneDelegate and a ComplexSceneDelegate to clearly see how the minimum implementation looks like and how a custom implementation looks like. Really dig it.

I'm happy we could make this architecture work. It feels very flexible! 🙌🏻

joemasilotti · 2023-11-08T22:19:02Z

Exciting! And I agree. This feels like the last bit of unknowns.

joemasilotti · 2023-11-11T17:28:14Z

This PR provided enough guidance to start upstreaming Turbo Navigator into turbo-ios.

Follow progress on #158!

olivaresf added 3 commits November 1, 2023 19:58

TurboNavigator -> TurboNavigationHierarchyController

a71d2d3

TurboNavigator now becomes a single entry point

e3790d0

Remove session behavior from old TurboNavigation (now TurboNavigation…

6dc761d

…HierarchyController)

olivaresf and others added 13 commits November 3, 2023 20:22

Continue polishing

93685d7

Set session delegate

5275fd1

Add missing path config

979400e

Begin re-enabling tests

438f8e7

Fix some more unit tests

10f5e5d

Add a WebkitUIDelegate object

7dd6260

Add TurboWKUIDelegate conformance to TurboNavigator

ad0bdda

Fix typo in group name: HElpers → Helpers

dd662ee

route(url:) → route(_:)

ba44036

Ensure presenting modals on alerts works

9c82925

Rework TurboNavigatorDelegate

2e250ce

* Ensure one is always set * Expose an open, default implementation * Implement optional methods in extension

Ensure WKUIDelegate is set upon initialization

657078a

Code formatting

74124a0

joemasilotti reviewed Nov 7, 2023

View reviewed changes

joemasilotti added 2 commits November 7, 2023 11:37

Convenience initializer to not expose Session

ee81553

Copy over cookies after each completed web request

f753dff

for authentication!

zoejessica reviewed Nov 8, 2023

View reviewed changes

joemasilotti added 2 commits November 8, 2023 11:05

Refactor default class to a protocol extension

eb1b4ea

TEMP: Example with more Turbo Navigator features

ded3633

olivaresf commented Nov 8, 2023

View reviewed changes

joemasilotti closed this Nov 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Turbo navigator without session #74

Turbo navigator without session #74

olivaresf commented Nov 2, 2023

joemasilotti commented Nov 2, 2023

olivaresf commented Nov 2, 2023

joemasilotti commented Nov 7, 2023 •

edited

Loading

joemasilotti Nov 7, 2023

olivaresf Nov 8, 2023

joemasilotti Nov 7, 2023

olivaresf Nov 7, 2023

joemasilotti Nov 7, 2023

olivaresf Nov 8, 2023

zoejessica Nov 8, 2023

joemasilotti Nov 8, 2023

zoejessica Nov 8, 2023

olivaresf Nov 8, 2023 •

edited

Loading

olivaresf Nov 8, 2023

joemasilotti Nov 8, 2023

zoejessica Nov 9, 2023

olivaresf Nov 8, 2023

joemasilotti Nov 8, 2023

olivaresf Nov 8, 2023

joemasilotti Nov 8, 2023

jayohms Nov 9, 2023

olivaresf Nov 9, 2023

olivaresf commented Nov 8, 2023

joemasilotti commented Nov 8, 2023

joemasilotti commented Nov 11, 2023

Turbo navigator without session #74

Turbo navigator without session #74

Conversation

olivaresf commented Nov 2, 2023

joemasilotti commented Nov 2, 2023

olivaresf commented Nov 2, 2023

joemasilotti commented Nov 7, 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

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

olivaresf Nov 8, 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

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

olivaresf commented Nov 8, 2023

joemasilotti commented Nov 8, 2023

joemasilotti commented Nov 11, 2023

joemasilotti commented Nov 7, 2023 •

edited

Loading

olivaresf Nov 8, 2023 •

edited

Loading