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

Include default server route rules by default #76

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -72,4 +72,8 @@ public extension PathProperties {
var animated: Bool {
self["animated"] as? Bool ?? true
}

internal var historicalLocation: Bool {
self["historical_location"] as? Bool ?? false
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,8 @@ public extension VisitProposal {
var animated: Bool {
properties.animated
}

internal var isHistoricalLocation: Bool {
properties.historicalLocation
}
}
25 changes: 22 additions & 3 deletions Source/Turbo/Navigator/NavigationHierarchyController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ class NavigationHierarchyController {
visitable.visitableView.allowsPullToRefresh = proposal.pullToRefreshEnabled
}

dismissModalIfNeeded(for: proposal)

switch proposal.presentation {
case .default:
navigate(with: controller, via: proposal)
Expand Down Expand Up @@ -175,6 +177,11 @@ class NavigationHierarchyController {
}

private func refresh(via proposal: VisitProposal) {
if proposal.isHistoricalLocation {
refreshIfTopViewControllerIsVisitable(from: .main)
return
}

if navigationController.presentedViewController != nil {
if modalNavigationController.viewControllers.count == 1 {
navigationController.dismiss(animated: proposal.animated)
Expand All @@ -183,10 +190,11 @@ class NavigationHierarchyController {
modalNavigationController.popViewController(animated: proposal.animated)
refreshIfTopViewControllerIsVisitable(from: .modal)
}
} else {
navigationController.popViewController(animated: proposal.animated)
refreshIfTopViewControllerIsVisitable(from: .main)
return
}

navigationController.popViewController(animated: proposal.animated)
refreshIfTopViewControllerIsVisitable(from: .main)
}

private func replaceRoot(with controller: UIViewController, via proposal: VisitProposal) {
Expand All @@ -204,4 +212,15 @@ class NavigationHierarchyController {
newTopmostVisitable: navControllerTopmostVisitable)
}
}

private func dismissModalIfNeeded(for visit: VisitProposal) {
// The desired behaviour for historical location visits is
// to always dismiss the "modal" stack.
guard visit.isHistoricalLocation,
navigationController.presentedViewController != nil else {
return
}

navigationController.dismiss(animated: visit.animated)
Comment on lines +219 to +224
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The guard with the != nil feels like a lot of inverse logic. What if we used an if statement here instead?

if visit.isHistoricalLocation, navigationController.presentedViewController != nil {
    navigationController.dismiss(animated: visit.animated)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally don't have issues with this and use guard extensively for early returns. If you feel strongly about this we can switch to if.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel strongly, no. It just took me a few reads to understand when dismiss(animated:) would be called.

}
}
5 changes: 4 additions & 1 deletion Source/Turbo/Path Configuration/PathConfiguration.swift
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ public final class PathConfiguration {
public private(set) var settings: [String: AnyHashable] = [:]

/// The list of rules from the configuration: `{ rules: [] }`
public private(set) var rules: [PathRule] = []
/// Default server route rules are included by default.
public private(set) var rules: [PathRule] = PathRule.defaultServerRoutes

/// Sources for this configuration, setting it will
/// cause the configuration to be loaded from the new sources
Expand Down Expand Up @@ -93,6 +94,8 @@ public final class PathConfiguration {
// Update our internal state with the config from the loader
settings = config.settings
rules = config.rules
// Always include the default server route rules.
rules.append(contentsOf: PathRule.defaultServerRoutes)
delegate?.pathConfigurationDidUpdate()
}
}
Expand Down
36 changes: 36 additions & 0 deletions Source/Turbo/Path Configuration/PathRule+ServerRoutes.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import Foundation

extension PathRule {
static let defaultServerRoutes: [PathRule] = [
.recedeHistoricalLocation,
.resumeHistoricalLocation,
.refreshHistoricalLocation
]

static let recedeHistoricalLocation = PathRule(
patterns: ["/recede_historical_location"],
properties: [
"presentation": "pop",
"context": "default",
"historical_location": true
]
)

static let resumeHistoricalLocation = PathRule(
patterns: ["/resume_historical_location"],
properties: [
"presentation": "none",
"context": "default",
"historical_location": true
]
)

static let refreshHistoricalLocation = PathRule(
patterns: ["/refresh_historical_location"],
properties: [
"presentation": "refresh",
"context": "default",
"historical_location": true
]
)
}
31 changes: 31 additions & 0 deletions Tests/Turbo/Fixtures/test-configuration-historical-locations.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
{
"rules":[
{
"patterns":[
"/recede_historical_location"
],
"properties":{
"presentation":"pop",
"context":"modal"
}
},
{
"patterns":[
"/resume_historical_location"
],
"properties":{
"presentation":"refresh",
"context":"modal"
}
},
{
"patterns":[
"/refresh_historical_location"
],
"properties":{
"presentation":"pop",
"context":"modal"
}
}
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
@testable import HotwireNative
import XCTest

@MainActor
final class NavigationHierarchyControllerHistoricalLocationTests: XCTestCase {
override func setUp() {
navigationController = TestableNavigationController()
modalNavigationController = TestableNavigationController()

navigator = Navigator(session: session, modalSession: modalSession)
hierarchyController = NavigationHierarchyController(
delegate: navigator,
navigationController: navigationController,
modalNavigationController: modalNavigationController
)
navigator.hierarchyController = hierarchyController

loadNavigationControllerInWindow()
}

// Resume behaviour:
// 1. Dismiss the modal view controller.
// 2. Arrive back at the view controller on the "default" stack.
func test_resumeHistoricalLocation() async throws {
let defaultOne = VisitProposal(path: "/default_one", context: .default)
navigator.route(defaultOne)
try await delay()

let defaultTwo = VisitProposal(path: "/default_two", context: .default)
navigator.route(defaultTwo)
try await delay()

XCTAssertEqual(navigationController.viewControllers.count, 2)

let modalOne = VisitProposal(path: "/modal_one", context: .modal)
navigator.route(modalOne)

try await delay()
XCTAssertEqual(modalNavigationController.viewControllers.count, 1)

// Reset spy's properties.
session.visitWasCalled = false
session.visitAction = nil

let resumeHistoricalLocationProposal = VisitProposal(
path: PathRule.resumeHistoricalLocation.patterns.first!,
additionalProperties: PathRule.resumeHistoricalLocation.properties
)
navigator.route(resumeHistoricalLocationProposal)
try await delay()

XCTAssertNil(navigationController.presentedViewController)
XCTAssertEqual(navigationController.viewControllers.count, 2)
XCTAssertEqual(navigator.session.activeVisitable?.visitableURL, defaultTwo.url)
XCTAssertFalse(session.visitWasCalled)
}

// Refresh behaviour:
// 1. Dismiss the modal view controller.
// 2. Arrive back at the view controller on the "default" stack.
// 3. Refresh the view controller on the "default" stack by revisiting the location.
func test_refreshHistoricalLocation() async throws {
let defaultOne = VisitProposal(path: "/default_one", context: .default)
navigator.route(defaultOne)
try await delay()

XCTAssertEqual(navigationController.viewControllers.count, 1)

let modalOne = VisitProposal(path: "/modal_one", context: .modal)
navigator.route(modalOne)
try await delay()

XCTAssertEqual(modalNavigationController.viewControllers.count, 1)

// Reset spy's properties.
session.visitWasCalled = false
session.visitAction = nil

let refreshHistoricalLocationProposal = VisitProposal(
path: PathRule.refreshHistoricalLocation.patterns.first!,
additionalProperties: PathRule.refreshHistoricalLocation.properties
)
navigator.route(refreshHistoricalLocationProposal)
try await delay()

XCTAssertNil(navigationController.presentedViewController)
XCTAssertEqual(navigationController.viewControllers.count, 1)
XCTAssertEqual(navigator.session.activeVisitable?.visitableURL, defaultOne.url)
XCTAssertTrue(session.visitWasCalled)
XCTAssertEqual(session.visitAction, .restore)
}

// Recede behaviour:
// 1. Dismiss the modal view controller.
// 2. Arrive back at the view controller on the "default" stack.
// 3. Pop the view controller on the "default" stack (unless it's already at the beginning of the backstack).
// 4. This will trigger a refresh on the appeared view controller if the snapshot cache has been cleared by a form submission.
@MainActor
func test_recedeHistoricalLocation() async throws {
let defaultOne = VisitProposal(path: "/default_one", context: .default)
navigator.route(defaultOne)
try await delay()

let defaultTwo = VisitProposal(path: "/default_two", context: .default)
navigator.route(defaultTwo)
try await delay()

XCTAssertEqual(navigationController.viewControllers.count, 2)

let modalOne = VisitProposal(path: "/modal_one", context: .modal)
navigator.route(modalOne)
try await delay()

XCTAssertEqual(modalNavigationController.viewControllers.count, 1)

let recedeHistoricalLocationProposal = VisitProposal(
path: PathRule.recedeHistoricalLocation.patterns.first!,
additionalProperties: PathRule.recedeHistoricalLocation.properties
)
navigator.route(recedeHistoricalLocationProposal)
try await delay()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The delays are far from ideal, but they are necessary to ensure the view lifecycle methods execute properly and the tests pass. I’ll explore architectural improvements in the future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised these are needed even with the TestableNavigationController presenting everything without animation. Does pushing, popping, or dismissing require the delay?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without the delay the recede historical location test fails. Even though the presentations are not animated, I doubt the view lifecycle methods are all called synchronously. I'll investigate.


XCTAssertNil(navigationController.presentedViewController)
XCTAssertEqual(navigationController.viewControllers.count, 1)
XCTAssertEqual(navigator.session.activeVisitable?.visitableURL, defaultOne.url)
}

func delay() async throws {
try await Task.sleep(nanoseconds: 1_000_000_000) // 1 second
}

private let session = SessionSpy(webView: Hotwire.config.makeWebView())
private let modalSession = Session(webView: Hotwire.config.makeWebView())

private var navigator: Navigator!
private var hierarchyController: NavigationHierarchyController!
private var navigationController: TestableNavigationController!
private var modalNavigationController: TestableNavigationController!

private let window = UIWindow()

// Simulate a "real" app so presenting view controllers works under test.
private func loadNavigationControllerInWindow() {
window.rootViewController = navigationController
window.makeKeyAndVisible()
navigationController.loadViewIfNeeded()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,7 @@ private class EmptyNavigationDelegate: NavigationHierarchyControllerDelegate {

// MARK: - VisitProposal extension

private extension VisitProposal {
extension VisitProposal {
init(path: String = "",
action: VisitAction = .advance,
context: Navigation.Context = .default,
Expand All @@ -441,7 +441,7 @@ private extension VisitProposal {
"context": context.rawValue,
"presentation": presentation.rawValue
]
let properties = defaultProperties.merging(additionalProperties) { (current, _) in current }
let properties = defaultProperties.merging(additionalProperties) { (_, new) in new }

self.init(url: url, options: options, properties: properties)
}
Expand Down
3 changes: 2 additions & 1 deletion Tests/Turbo/Navigator/TestableNavigationController.swift
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import UIKit
@testable import HotwireNative

/// Manipulate a navigation controller under test.
/// Ensures `viewControllers` is updated synchronously.
/// Manages `presentedViewController` directly because it isn't updated on the same thread.
class TestableNavigationController: UINavigationController {
class TestableNavigationController: HotwireNavigationController {
override var presentedViewController: UIViewController? {
get { _presentedViewController }
set { _presentedViewController = newValue }
Expand Down
Loading