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

Only update history when Turbo visit is renderable #601

Merged
merged 8 commits into from
Jun 22, 2022
Merged

Only update history when Turbo visit is renderable #601

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6dcf1eb
use location.replace instead of location.reload
manuelpuyol Jun 14, 2022
f6456da
update when we change the history
manuelpuyol Jun 14, 2022
b3c08c3
get location from visitStart
manuelpuyol Jun 14, 2022
ed11325
always pass the visit to renderError/Page
manuelpuyol Jun 14, 2022
fe0d60e
rollback name update
manuelpuyol Jun 14, 2022
6b726a5
revert visit_control view hack
manuelpuyol Jun 14, 2022
3dc8ac7
await nextBeat for the URL to be ready
manuelpuyol Jun 14, 2022
2eb9ea2
remove console log
manuelpuyol Jun 14, 2022
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: 2 additions & 2 deletions src/core/drive/navigator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,9 @@ export class Navigator {
if (responseHTML) {
const snapshot = PageSnapshot.fromHTMLString(responseHTML)
if (fetchResponse.serverError) {
await this.view.renderError(snapshot)
await this.view.renderError(snapshot, this.currentVisit)
} else {
await this.view.renderPage(snapshot)
await this.view.renderPage(snapshot, false, true, this.currentVisit)
}
this.view.scrollToTop()
this.view.clearSnapshotCache()
Expand Down
8 changes: 6 additions & 2 deletions src/core/drive/page_view.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { ErrorRenderer } from "./error_renderer"
import { PageRenderer } from "./page_renderer"
import { PageSnapshot } from "./page_snapshot"
import { SnapshotCache } from "./snapshot_cache"
import { Visit } from "./visit"

export interface PageViewDelegate extends ViewDelegate<PageSnapshot> {
viewWillCacheSnapshot(): void
Expand All @@ -16,15 +17,18 @@ export class PageView extends View<Element, PageSnapshot, PageViewRenderer, Page
lastRenderedLocation = new URL(location.href)
forceReloaded = false

renderPage(snapshot: PageSnapshot, isPreview = false, willRender = true) {
renderPage(snapshot: PageSnapshot, isPreview = false, willRender = true, visit?: Visit) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably in a follow-up refactor but it's starting to look like this function should take in a "options" object rather than individual arguments.

const renderer = new PageRenderer(this.snapshot, snapshot, isPreview, willRender)
if (!renderer.shouldRender) {
this.forceReloaded = true
} else {
visit?.changeHistory()
}
return this.render(renderer)
}

renderError(snapshot: PageSnapshot) {
renderError(snapshot: PageSnapshot, visit?: Visit) {
visit?.changeHistory()
const renderer = new ErrorRenderer(this.snapshot, snapshot, false)
return this.render(renderer)
}
Expand Down
6 changes: 3 additions & 3 deletions src/core/drive/visit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,11 +225,11 @@ export class Visit implements FetchRequestDelegate {
this.cacheSnapshot()
if (this.view.renderPromise) await this.view.renderPromise
if (isSuccessful(statusCode) && responseHTML != null) {
await this.view.renderPage(PageSnapshot.fromHTMLString(responseHTML), false, this.willRender)
await this.view.renderPage(PageSnapshot.fromHTMLString(responseHTML), false, this.willRender, this)
this.adapter.visitRendered(this)
this.complete()
} else {
await this.view.renderError(PageSnapshot.fromHTMLString(responseHTML))
await this.view.renderError(PageSnapshot.fromHTMLString(responseHTML), this)
this.adapter.visitRendered(this)
this.fail()
}
Expand Down Expand Up @@ -267,7 +267,7 @@ export class Visit implements FetchRequestDelegate {
this.adapter.visitRendered(this)
} else {
if (this.view.renderPromise) await this.view.renderPromise
await this.view.renderPage(snapshot, isPreview, this.willRender)
await this.view.renderPage(snapshot, isPreview, this.willRender, this)
this.adapter.visitRendered(this)
if (!isPreview) {
this.complete()
Expand Down
8 changes: 6 additions & 2 deletions src/core/native/browser_adapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export class BrowserAdapter implements Adapter {

visitProgressBarTimeout?: number
formProgressBarTimeout?: number
location?: URL

constructor(session: Session) {
this.session = session
Expand All @@ -27,9 +28,9 @@ export class BrowserAdapter implements Adapter {
}

visitStarted(visit: Visit) {
this.location = visit.location
visit.loadCachedSnapshot()
visit.issueRequest()
visit.changeHistory()
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not positive, but moving the history changing to PageView might have implications for the native adapters for iOS and Android. The adapters will still be calling visit.changeHistory themselves, so it will be called twice:

The second should be a no-op because history will have been flagged as changed, but I think we'll still have the same problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how do we avoid calling it twice now?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's only called once by each adapter now. If we move it out of the browser adapter, we may need to move it out of the other adapters as well. Perhaps we don't (it's been a while since I've looked at the native frameworks), but we need to make sure it behaves as expected on iOS and Android.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any tips on how to test that properly? Looking at https://github.com/manuelpuyol/turbo/blob/2eb9ea2c7282f1ec2b41c75705d7e43af7e2076a/src/core/drive/visit.ts#L173-L174 I'd expect it to not do a double update

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, it will be a no-op because we've already flagged history as having changed. My concern is that we're changing the behavior by moving this out of the adapter, so we just need to ensure it's all good on iOS and Android 👍

Copy link
Member

Choose a reason for hiding this comment

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

@jayohms Can you verify whether this would bring any complication on native?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't foresee any issues on the mobile side, since as mentioned, the second visit.changeHistory call will be a no-op, so the mobile adapters will continue to work as before. It'd be nice to update the mobile adapters to reflect this change, however, we've maintained backwards/forwards compatibility for all iterations of turbo/links API changes.

I don't think we'll be able to reasonably make corresponding changes in the mobile adapters without exposing an additional API, so the adapters can know to conditionally change history (or not). This shouldn't be a problem in the short-term, but it may be a nuance that's difficult for others to understand as time goes on.

visit.goToSamePageAnchor()
}

Expand Down Expand Up @@ -121,7 +122,10 @@ export class BrowserAdapter implements Adapter {

reload(reason: ReloadReason) {
dispatch("turbo:reload", { detail: reason })
window.location.reload()

if (!this.location) return

window.location.href = this.location.toString()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any potential that this causes issues with or prevents reloads for same-page anchors? I'm not clear if that situation is possible here.

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 can't imagine a case where a same-page anchor causes a full page reload

}

get navigator() {
Expand Down
1 change: 1 addition & 0 deletions src/tests/fixtures/rendering.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ <h1>Rendering</h1>
<button id="permanent-video-button" type="button">Play</button>

<hr class="push-off-screen">
<p><a id="below-the-fold-visit-control-reload-link-middle" href="/src/tests/fixtures/visit_control_reload.html#middle">Visit control: reload - middle</a></p>
<p><a id="below-the-fold-visit-control-reload-link" href="/src/tests/fixtures/visit_control_reload.html">Visit control: reload</a></p>
</body>
</html>
15 changes: 12 additions & 3 deletions src/tests/fixtures/visit_control_reload.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,17 @@
<script src="/dist/turbo.es2017-umd.js" data-turbo-track="reload"></script>
<script src="/src/tests/fixtures/test.js"></script>
<meta name="turbo-visit-control" content="reload">
</head>
<body>
<h1>Visit control: reload</h1>
<style>
.push-off-screen { margin-top: 1000px; }
</style>
</head>
<body>
<h1>Visit control: reload</h1>

<hr class="push-off-screen">

<h2 id="middle">Middle the page</h2>
<hr class="push-off-screen">
<h2>Down the page</h2>
Comment on lines +16 to +20
Copy link
Contributor

Choose a reason for hiding this comment

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

Very nit-pick but:

Suggested change
<hr class="push-off-screen">
<h2 id="middle">Middle the page</h2>
<hr class="push-off-screen">
<h2>Down the page</h2>
<hr class="push-off-screen">
<h2 id="middle">Middle the page</h2>
<hr class="push-off-screen">
<h2>Down the page</h2>

</body>
</html>
2 changes: 2 additions & 0 deletions src/tests/functional/visit_tests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ export class VisitTests extends TurboDriveTestCase {
const urlBeforeVisit = await this.location
await this.visitLocation("/src/tests/fixtures/one.html")

await this.nextBeat

const urlAfterVisit = await this.location
this.assert.notEqual(urlBeforeVisit, urlAfterVisit)
this.assert.equal(await this.visitAction, "advance")
Expand Down