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.

Sign up for GitHub

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

Support Turbo 8 same-page refreshing + morphing #178

Merged
merged 3 commits into from
Feb 23, 2024
Merged

Support Turbo 8 same-page refreshing + morphing #178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f6bd2de
Support Turbo 8 same-page refreshes and include debug logging
jayohms Feb 23, 2024
96bb4c3
Support visit.isPageRefresh and avoid displaying the activity indicat…
jayohms Feb 23, 2024
47e393b
Provide backwards compatibility support for Turbo 7.x and Turbolinks 5
jayohms Feb 23, 2024
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
4 changes: 4 additions & 0 deletions Source/Session/Session.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,12 +164,16 @@ extension Session: VisitDelegate {
}

func visitWillStart(_ visit: Visit) {
guard !visit.isPageRefresh else { return }

visit.visitable.showVisitableScreenshot()
activateVisitable(visit.visitable)
}

func visitDidStart(_ visit: Visit) {
guard !visit.hasCachedSnapshot else { return }
guard !visit.isPageRefresh else { return }

visit.visitable.showVisitableActivityIndicator()
}

Expand Down
5 changes: 3 additions & 2 deletions Source/Visit/JavaScriptVisit.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,11 @@ final class JavaScriptVisit: Visit {
}

extension JavaScriptVisit: WebViewVisitDelegate {
func webView(_ webView: WebViewBridge, didStartVisitWithIdentifier identifier: String, hasCachedSnapshot: Bool) {
log("didStartVisitWithIdentifier", ["identifier": identifier, "hasCachedSnapshot": hasCachedSnapshot])
func webView(_ webView: WebViewBridge, didStartVisitWithIdentifier identifier: String, hasCachedSnapshot: Bool, isPageRefresh: Bool) {
log("didStartVisitWithIdentifier", ["identifier": identifier, "hasCachedSnapshot": hasCachedSnapshot, "isPageRefresh": isPageRefresh])
self.identifier = identifier
self.hasCachedSnapshot = hasCachedSnapshot
self.isPageRefresh = isPageRefresh

delegate?.visitDidStart(self)
}
Expand Down
1 change: 1 addition & 0 deletions Source/Visit/Visit.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ class Visit: NSObject {
let location: URL

var hasCachedSnapshot: Bool = false
var isPageRefresh: Bool = false
private(set) var state: VisitState

init(visitable: Visitable, options: VisitOptions, bridge: WebViewBridge) {
Expand Down
2 changes: 2 additions & 0 deletions Source/WebView/ScriptMessage.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ extension ScriptMessage {
case pageLoadFailed
case errorRaised
case visitProposed
case visitProposalScrollingToAnchor
case visitProposalRefreshingPage
case visitStarted
case visitRequestStarted
case visitRequestCompleted
Expand Down
8 changes: 6 additions & 2 deletions Source/WebView/WebViewBridge.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ protocol WebViewPageLoadDelegate: AnyObject {
}

protocol WebViewVisitDelegate: AnyObject {
func webView(_ webView: WebViewBridge, didStartVisitWithIdentifier identifier: String, hasCachedSnapshot: Bool)
func webView(_ webView: WebViewBridge, didStartVisitWithIdentifier identifier: String, hasCachedSnapshot: Bool, isPageRefresh: Bool)
func webView(_ webView: WebViewBridge, didStartRequestForVisitWithIdentifier identifier: String, date: Date)
func webView(_ webView: WebViewBridge, didCompleteRequestForVisitWithIdentifier identifier: String)
func webView(_ webView: WebViewBridge, didFailRequestForVisitWithIdentifier identifier: String, statusCode: Int)
Expand Down Expand Up @@ -125,8 +125,12 @@ extension WebViewBridge: ScriptMessageHandlerDelegate {
delegate?.webViewDidInvalidatePage(self)
case .visitProposed:
delegate?.webView(self, didProposeVisitToLocation: message.location!, options: message.options!)
case .visitProposalScrollingToAnchor:
break
case .visitProposalRefreshingPage:
break
case .visitStarted:
visitDelegate?.webView(self, didStartVisitWithIdentifier: message.identifier!, hasCachedSnapshot: message.data["hasCachedSnapshot"] as! Bool)
visitDelegate?.webView(self, didStartVisitWithIdentifier: message.identifier!, hasCachedSnapshot: message.data["hasCachedSnapshot"] as! Bool, isPageRefresh: message.data["isPageRefresh"] as! Bool)
case .visitRequestStarted:
visitDelegate?.webView(self, didStartRequestForVisitWithIdentifier: message.identifier!, date: message.date)
case .visitRequestCompleted:
Expand Down
20 changes: 12 additions & 8 deletions Source/WebView/turbo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,14 +99,18 @@
// Adapter interface

visitProposedToLocation(location, options) {
if (window.Turbo && typeof Turbo.navigator.locationWithActionIsSamePage === "function") {
if (Turbo.navigator.locationWithActionIsSamePage(location, options.action)) {
Turbo.navigator.view.scrollToAnchorFromLocation(location)
return
}
if (window.Turbo && Turbo.navigator.locationWithActionIsSamePage(location, options.action)) {
// Scroll to the anchor on the page
this.postMessage("visitProposalScrollingToAnchor", { location: location.toString(), options: options })
Turbo.navigator.view.scrollToAnchorFromLocation(location)
} else if (window.Turbo && Turbo.navigator.location?.href === location.href) {
Copy link
Contributor

@pfeiffer pfeiffer Feb 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Wondering if this is sufficient to detect a page refresh - the corresponding check in Turbo compares the pathname of the two URLs (so replacing to a different query string is also considered a refresh) as well as explicitly checking the that the action is replace:

  isPageRefresh(visit) {
    return !visit || (this.lastRenderedLocation.pathname === visit.location.pathname && visit.action === "replace")
  }

I'm thinking the behavior should be the same here.

Preferrably, Turbo made available a Turbo.navigator.isPageRefresh(location, options) that could be used here and also by Turbo itself, similar to locationWithActionIsSamePage(..) used a few lines above. Any future changes in the logic to detect refreshes in Turbo library would also be applied here without the need for native updates.

What are your thoughts? Happy to wrap up a PR! CC @afcapel, @jayohms

Copy link
Collaborator Author

@jayohms jayohms Feb 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There are two separate concerns here, even though they're related. The first is answering the question, when do we want to potentially (re)create new ViewController instances? Previously, revisiting the same url as the current Visit/ViewController would go through the whole native proposal callback lifecycle.

I don't think this is necessary anymore going forward for replace or advance visits — even for when Turbo 8 morphing is not enabled (and for Turbo 7). If the same visit url is being proposed, let the adapter handle it completely and transparently now without letting it be an app decision. The fact that we hadn't done this previously was mostly a legacy concern before Strada components made it possible to observe state in the WebView and maintain proper state with native features built on top of the WebView. Recreating the ViewController was a blunt way to force the screen to update its state. Strada allows apps to solve this in an elegant way, now.

The second concern is does the library properly handle Turbo 8 morph refreshes? With this PR, it looks like the answer is "yes".

So, while the implementations are slightly different, I don't think we should limit ViewController visit "refreshes" to when Turbo 8 morphing is used or even just replace visits. The concern about using .href vs .pathname is valid, though — and we should decide if we want to handle it in the same way. The only concern I have here is you could theoretically setup a path configuration to handle different query urls for different ViewController implementations. Maybe that use case is an edge case that's not worth worrying about, though.

Thoughts @pfeiffer?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jayohms, I've been giving it some thought and I think what you say here makes a lot of sense. In answering the question "should this visit trigger a new view controller?", I suppose the answer is no, no matter if it's a replace or advance action, as long as the URL is the same.

The same happen in a browser context, where clicking a link element pointing to the same URL as the current URL, actually reloads the page, without pushing it onto the history stack. This is true for non-Turbo websites as well as Turbo-enabled websites.

Regarding .pathname vs .href, the behavior in a browser context would be to push a new item onto the history stack if the href's don't match. I guess the current behavior in this PR actually does the same; if the query params differ, a new visit will be proposed to the native side and what you mention with a path configuration handling different query urls for different ViewController implementation would be supported.

What could be considered is adding support for the pathname + replace visit as Turbo Web also does, eg letting the normal adapter perform the visit without proposing a visit on the native side in the case that:

  1. The href's match OR
  2. The pathname's match AND action = replace
const currentLocation = Turbo.navigator.location

currentLocation?.href === location.href ||
  (currentLocation?.pathname === location.pathname && options.action === "replace")

This would ensure that a replace with potential morph happen when query params change, but only if a replace action was explicitly provided. This matches the behavior on Turbo web.

Copy link
Contributor

Choose a reason for hiding this comment

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

One could argue that the behavior of an advance action on Turbo web to the exact same href should also be considered a page refresh and use morphing, though 🤷🏻‍♂️ I mean; no history is pushed and it is a request to refresh the page.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great idea @pfeiffer, I like that balance. Could you put together a PR with this isolated change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perfect, thanks!

Copy link

Choose a reason for hiding this comment

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

@jayohms sorry, I didn't quite understand this. Is there a special reason why you decided to use href instead of pathname? The behavior changed in hotwired/turbo#1079, you can read more about the argumentation there. Why is it different? Or is this what @pfeiffer intends to change? The same would apply to Turbo Android as well. I can do a PR on both libraries to correct that if that seems appropriate to you. Thanks!

Copy link
Contributor

@pfeiffer pfeiffer Feb 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@brunoprietog @jayohms - I was starting to implement the pathname comparison as discussed above, but ultimately I've come to the conclusion that it will lead to unexpected behavior, and that I think the href only is a better a comparison here unless some other changes are made to the library:

In difference to the browser context, any Visitable drives navigations with it's URL. When doing refreshes bypassing the native adapters to what is essentially a different URL (same path, different params), the Visitable loose track of the URL it's displaying. The problem is that this URL is used various places, eg when reloading a Session natively. Strada components also filter messages by their location, so a difference in document.location.href vs visitableURL could lead to a broken Strada bridge.

Imagine this example:

  1. Open screen A (/one)
  2. On that screen, a replace visit to /one?tab=latest) is performed; as the pathnames are equal, native adapter proposal is bypassed.
  3. Initiate a pull-to-refresh action on the screen - the Session will reload /one, not /one?tab=latest as expected

Ultimately, I think any visit that would change the document.location.href (except anchor navigations) should be proposed natively, so I don't think there's need for changing the behavior introduced in this PR.

Copy link

Choose a reason for hiding this comment

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

I regret we have that limitation, I honestly wish I had the same behavior as the browser adapter. Maybe there the conceptual design error is to think that doing a refresh is simply triggering a reload of the exact current URL, rather than executing a replace action. But I haven't gotten too deep into Turbo native to go into more depth.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pfeiffer You bring up some great points — right now any Visitable destination treats its location as immutable for the current ViewController. I think what we have right now will work for most situations, but we may have to tweak some fundamental assumptions in the native libraries if we want to make sure everything works consistently with the web browser adapter long-term.

@brunoprietog I think we can align things, but it's not a trivial change right now with how the iOS and Android libraries are designed.

Maybe there the conceptual design error is to think that doing a refresh is simply triggering a reload of the exact current URL, rather than executing a replace action.

It's more complicated than this. Any unique location path pattern can map to unique ViewController instances — native or web. So, any visit proposal with a url change could require a new ViewController instance, even for replace actions. There's an opportunity to be smarter about this, but it'd be quite a bit of work.

I'm going to cut a new release of turbo-ios and turbo-android with what we have now, since it's a big improvement over what we had previously. We can improve further improve things later.

// Refresh the page without native proposal
this.postMessage("visitProposalRefreshingPage", { location: location.toString(), options: options })
this.visitLocationWithOptionsAndRestorationIdentifier(location, options, Turbo.navigator.restorationIdentifier)
} else {
// Propose the visit
this.postMessage("visitProposed", { location: location.toString(), options: options })
}

this.postMessage("visitProposed", { location: location.toString(), options: options })
}

// Turbolinks 5
Expand All @@ -116,7 +120,7 @@

visitStarted(visit) {
this.currentVisit = visit
this.postMessage("visitStarted", { identifier: visit.identifier, hasCachedSnapshot: visit.hasCachedSnapshot() })
this.postMessage("visitStarted", { identifier: visit.identifier, hasCachedSnapshot: visit.hasCachedSnapshot(), isPageRefresh: visit.isPageRefresh || false })
this.issueRequestForVisitWithIdentifier(visit.identifier)
this.changeHistoryForVisitWithIdentifier(visit.identifier)
this.loadCachedSnapshotForVisitWithIdentifier(visit.identifier)
Expand Down