Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upstream Turbo Navigator #158

Open
wants to merge 90 commits into
base: main
Choose a base branch
from
Open

Upstream Turbo Navigator #158

wants to merge 90 commits into from

Conversation

joemasilotti
Copy link
Member

@joemasilotti joemasilotti commented Nov 11, 2023

This PR upstreams the Turbo Navigator package.

Turbo Navigator is a drop-in class for Turbo Native apps to handle common navigation flows. It brings turbo-ios closer to feature parity with turbo-android. See the linked README for all 15+ flows it handles.

To-do

  • Merge Replace Quick/Nimble with XCTest #139 into main, which this PR is branched off of
  • Copy-paste codebase
  • Update demo app
  • Consolidate config (TurboConfig vs. TurboLog)
  • Clean up web view configuration via TurboConfig
  • Fix/update tests
  • Migrate/update documentation

@joemasilotti
Copy link
Member Author

I think we are in a good enough spot with the existing Turbo.config for web view configuration. Developers are able to create custom, non-global ones using the designated TurboNavigator initializer if need be. And in the future a higher-level package can better integrate Strada out of the box.

@leonvogt
Copy link
Contributor

Hi there 👋

I use this turbo-navigator branch to build my current iOS project.
After upgrading to the latest version of this branch, I noticed that the TurboNavigator initializer changed.

Previously, I used to initialize the TurboNavigator like this:

TurboNavigator(session: session, modalSession: modalSession, delegate: self)

Where session and modalSession are manually created Session instances, for some custom configurations.

After upgrading I get the error: Extra arguments at positions #1, #2 in call, when trying to initialize the TurboNavigator.

It seems like, I can't use the initializer with custom sessions anymore and should use the "convenience initializer" that doesn't require Session instances.
Looks like this change was introduced in the commit 6590418.
Where the public init function changed like this:

- public init(session: Session, modalSession: Session, delegate: TurboNavigatorDelegate? = nil) {
+ init(session: Session, modalSession: Session, delegate: TurboNavigatorDelegate? = nil) {

Just want to ask if this change was intentional and if there is a way to use custom sessions with the current version of TurboNavigator?

Thanks in advance!

@hey-leon
Copy link

Hi there 👋

I use this turbo-navigator branch to build my current iOS project. After upgrading to the latest version of this branch, I noticed that the TurboNavigator initializer changed.

Previously, I used to initialize the TurboNavigator like this:

TurboNavigator(session: session, modalSession: modalSession, delegate: self)

Where session and modalSession are manually created Session instances, for some custom configurations.

After upgrading I get the error: Extra arguments at positions #1, #2 in call, when trying to initialize the TurboNavigator.

It seems like, I can't use the initializer with custom sessions anymore and should use the "convenience initializer" that doesn't require Session instances. Looks like this change was introduced in the commit 6590418. Where the public init function changed like this:

- public init(session: Session, modalSession: Session, delegate: TurboNavigatorDelegate? = nil) {
+ init(session: Session, modalSession: Session, delegate: TurboNavigatorDelegate? = nil) {

Just want to ask if this change was intentional and if there is a way to use custom sessions with the current version of TurboNavigator?

Thanks in advance!

Is there a specific usecase you are looking for. I think the API expects customisation of the session to happen via:

Turbo.config.userAgent = "Custom user agent here"

Turbo.config.makeCustomWebView { configuration in
  // build a custom web view here
}

The main session and modal session will utilise the results of these closures. They also get setup with a shared process pool.

If you use Xcode I recommend looking the dependencies source code at Source/Turbo.swift.

In saying all this. The current setup may not cover your use case so it would be good to know what it is. There also could be some opportunities to contribute to documentation here (I didnt check if all this is covered in the docs).

@leonvogt
Copy link
Contributor

leonvogt commented May 1, 2024

@hey-leon thanks for you reply and suggestion!
I've gave the Turbo.config.makeCustomWebView another try but it doesn't seem to work for our use case.

We need the WebView of both the session and modalSession to connect each of them with an instance our own JS-bridge.
The TurboNavigator hides the two WebViews (by hiding the sessions), so we used the initializer in question to pass our own sessions.
Not because we are interested in the sessions themselves, but because that allowed us to connect the sessions' WebViews with our JS-bridge.

Note: The existing approach to customize the WebView (by Turbo.config.makeCustomWebView) is not sufficient. We must be able to distinguish between the WebView of the base- and the modal-session.
For our purpose it would be sufficient to get access to the two WebViews, or pass them as arguments to the initializer, or pass a function to configure the WebView (much like Turbo.config.makeCustomWebView)

@joemasilotti
Copy link
Member Author

Note: The existing approach to customize the WebView (by Turbo.config.makeCustomWebView) is not sufficient. We must be able to distinguish between the WebView of the base- and the modal-session.

Mind if I ask why you need to distinguish between the two?

We had some internal discussion and came to the conclusion that a single web view configuration covers all the use cases we could think of.

@leonvogt
Copy link
Contributor

leonvogt commented May 3, 2024

Mind if I ask why you need to distinguish between the two?

Sure! But it's gonna take a line or two to explain 😅

Our situation is the following: We started using Turbo Native early on, without Turbo Navigator or Strada.

Our NativeBridge Approach

So we implemented our own js-bridge (NativeBridge).
The bridge keeps a reference to the webView to which it connects (to call webView.evaluateJavaScript).
When adding Turbo Navigator, we extended this to: Two NativeBridge instances, one with the base WebView, the other with the modal WebView.

private var nativeBridge: NativeBridge?
private var modalNativeBridge: NativeBridge?

We then use the following extension for TurboNavigator, which allows to decide "Are we currently displaying the base, or the model?"

extension TurboNavigator {
  var isModal: Bool { self.rootViewController.presentedViewController != nil }
}

So the bridge of the currently active WebView is:

var activeNativeBridge: NativeBridge? {
  if navigator.isModal {
    return modalNativeBridge
  } else {
    return nativeBridge
  }
}

(We later extended this approach to multiple tabs, in a similar way, by simply returning the active bridge of the active tab)

To create these two NativeBridges we need to have a reference to the WebViews.
However, these are managed by the Session and the sessions (session and modalSession) are internal to the TurboNavigator.
So the only way to get to the WebViews is when creating them.
This is where the initializer in question becomes relevant. It allowed us to create the WebViews in a "controlled" manner.

private func makeNavigator(pathConfiguration: PathConfiguration) -> TurboNavigator {
  self.nativeBridge = NativeBridge(delegate: TurboCentral.instance)
    
  // Create webView and connect it to nativeBridge
  let webView = Turbo.config.makeWebView()
  self.nativeBridge.setWebView(webView: webView)
  let session = Session(webView: webView)
   
  // Do the same thing for the modal
  self.modalNativeBridge = NativeBridge(delegate: TurboCentral.instance)
    
  let modalWebView = Turbo.config.makeWebView()
  self.modalNativeBridge.setWebView(webView: modalWebView)
  let modalSession = Session(webView: modalWebView)
    
  return TurboNavigator(session: session, modalSession: modalSession, delegate: TurboCentral.instance)
}

(the code is slightly simplified)

I hope from this it becomes clear why we want to be able to distinguish between the two WebViews.
But why do we think we to need distinguish in the first place? Here comes our use case.

Looking at the Strada approach for the native bridge, we get the impression that our approach does not adapt very nicely to Strada nor Turbo Navigator.
But that's how it grew and we were quite happy so far.

Use Case

The use case we need to cover with our NativeBridge is for example:

  1. An event is fired in the native code, let's say a push notification is received.
  2. We identify the currently active NativeBridge (and by that the currently active WebView)
  3. We use webView.evaluateJavaScript to send the event to the JS of the currently active WebView
  4. JS updated the view (e.g. by reloading it, or rendering a message, ...)

We need to be able to:

  1. Know if the modal or base view is currently top-most -> This is done with the isModel extension to TurboNavigator
  2. Get the correct WebView -> This is done by storing references to both WebViews in the nativeBridge and modalNativeBridge

If this use case can easily be covered using a more elegant approach, we are very happy to adapt our code accordingly!
Our insistence on the initializer in question is simply because - from our understanding - our approach won't work anymore without it.

@joemasilotti
Copy link
Member Author

Ah, gotcha! Thanks for the explanation.

I think you might know what I'm about to say because you said it yourself: "we get the impression that our approach does not adapt very nicely to Strada nor Turbo Navigator." And you're right.

With Strada you don't need to know "which web view is which" like you did with a traditional JavaScript bridge. My recommendation is to convert your existing codebase to use Strada. If that's out of the question then you can continue to use turbo-ios without Turbo Navigator.

I hope that helps, even if it isn't the answer you might be looking for.

olivaresf and others added 7 commits May 15, 2024 09:55
* Add options parameter

* Add animation key to visit proposal

* Add modal root view controller access

* Change var -> constant

* Add default parameter for visit options

* Clarify default visit options
@mrfidgety
Copy link

@joemasilotti I am using this branch in my iOS app and have found I am missing the following behaviour:

When a Modal is dismissed, I want to perform a restoration visit in the underlying view.

I am a novice Swift developer, and have been struggling to implement this custom behaviour 😅 .

I have tried using a subclass of the UINavigationController to introduce this behaviour, and setting this new class as the Turbo.config.defaultNavigationController. However, this needs to be able to access the session to trigger the restoration visit, which I cannot seem to access.

I hope this all makes sense. Thanks for all the guides and docs ❤️

@mrfidgety
Copy link

Update to the comment above.

I have figured out a solution to my use case, which is definitely a bit rudimentary, but probably worth sharing.

I have a view that is presented in a modal, which can perform a POST request and receive a turbo-stream response. I.e. the modal does not dismiss, and nothing is pushed onto the view stack. When this happens, if the user is to dismiss the modal, I want the underlying view to 'refresh'. I have achieved this by adding a query param to the url of the modal view. This query param is checked when the modal is dismissed, and visitableDelegate?.visitableDidRequestRefresh(self) is called on the rootViewController's (navigation controller) topViewController.

This might be gross, but I have no shame 😅 (please tell me if there is a better way)

## AppDelegate
func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplication.LaunchOptionsKey: Any]?) -> Bool {
    // Set custom view controller
    Turbo.config.defaultViewController = { url in
        return CustomVisitableViewController(url: url)
    }

    // Set custom navigation controller
    Turbo.config.defaultNavigationController = {
        CustomNavigationController()
    }

    return true
}
## SceneDelegate
func scene(_ scene: UIScene, willConnectTo session: UISceneSession, options connectionOptions: UIScene.ConnectionOptions) {
    guard let _ = (scene as? UIWindowScene) else { return }

    // Let the rootViewController have visibility of the TurboNavigator instance
    navigator.rootViewController.setTurboNavigator(navigator)

    window?.rootViewController = navigator.rootViewController
    navigator.route(baseURL)
}

And now for the magic...

class CustomNavigationController: UINavigationController {
    var navigator: TurboNavigator?

    override func dismiss(animated flag: Bool, completion: (() -> Void)? = nil) {
        let modalRequiresRefreshOnDismiss = checkIfModalAndRequiresRefresh()

        super.dismiss(animated: flag) { [weak self] in
            if let self = self, modalRequiresRefreshOnDismiss {
                self.refreshRootController()
            }
            completion?()
        }
    }

    private func checkIfModalAndRequiresRefresh() -> Bool {
        // Guard if navigator not set
        guard let navigator = navigator else { return false }

        // Check if navigator modalRootViewController has a topViewController that requires refresh on dismiss
        return (navigator.modalRootViewController as? CustomNavigationController)?.topViewController?.requiresRefreshOnDismiss() ?? false
    }

    private func refreshRootController() {
        topViewController?.requestRefresh()
    }
}

extension UINavigationController {
    func setTurboNavigator(_ navigator: TurboNavigator?) {
        if let customNavController = self as? CustomNavigationController {
            customNavController.navigator = navigator
        }
    }
}
class CustomVisitableViewController: VisitableViewController {
    var navigator: TurboNavigator?

    override func requiresRefreshOnDismiss() -> Bool {
        print("Checking if dismiss requires refresh")
        
        guard let webView = visitableView.webView,
              let query = webView.url?.query else {
            return false
        }

        // Check for the presence of refresh instruction
        return query.contains("dismiss=refresh")
    }
    
    override func requestRefresh() {
        print("Request refresh")
    
        visitableDelegate?.visitableDidRequestRefresh(self)
    }
}

extension UIViewController {
    @objc func requestRefresh() {}
    
    @objc func requiresRefreshOnDismiss() -> Bool {
        // Default implementation returns false
        return false
    }
}

@scarroll32
Copy link

Any chance this will be merged soon?

@ansonhoyt
Copy link

Any chance this will be merged soon?

I don't know anything, but am expecting to wait until October for movement, as a key player with this PR, the author, is taking some extended time off for a new baby: https://x.com/joemasilotti/status/1796202085542957493 🎉

I wish him and his wife well...but am keeping an eye on this too 😆

@yshmarov
Copy link

yshmarov commented Jul 6, 2024

Until it is merged, I'm just using this branch:
Screenshot 2024-07-06 at 10 33 52

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet