Skip to content

Commit

Permalink
Move cache snapshot loading to Visit#start() (#1056)
Browse files Browse the repository at this point in the history 
* Move cache snapshot loading to Visit#start()

The cache interface changed in #949

Since the cache can be loaded from disk using the browser Cache API, it is
possible that the cache snapshot will be loaded asynchronously. This means
that the calls to load cache snapshots need to be awaited.

We also changed visit.hasCachedSnapshot() to be async and return a Promise.
However, [hasCachedSnapshot is used in the iOS adapter](https://github.com/hotwired/turbo-ios/blob/c476bac66f260adbfe930ed9a151e7637973ff99/Source/WebView/turbo.js#L119)
and serialized into and data object we send to the native side via
postMessage. When postMessagereceives a Promise instead of a boolean
value it fails with a DataCloneError since it can't serialize the Promise.

This commit moves the cache snapshot loading to Visit#start() and stores
the result in a cachedSnapshot property. This allows us to keep the
hasCachedSnapshot() method synchronous and return a boolean value.

It means that Visit.start is now async, but I haven't found any callers
that rely on it being synchronous.

* Explicitly test that visit.hasCachedSnapshot() returns a boolean, which is what the native adapters expect

* Remove the "test " prefix from the native adapter unit tests, since other tests were recently updated to remove the prefix

---------

Co-authored-by: Jay Ohms <jay.ohms@gmail.com>
  • Loading branch information
@jayohms
Alberto Fernández-Capel and jayohms committed Nov 3, 2023
1 parent 3d48c86 commit e07639f
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 28 deletions.
2 changes: 1 addition & 1 deletion src/core/drive/navigator.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export class Navigator {
referrer: this.location,
...options
})
this.currentVisit.start()
return this.currentVisit.start()
}

submitForm(form, submitter) {
Expand Down
14 changes: 7 additions & 7 deletions src/core/drive/visit.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,10 +105,11 @@ export class Visit {
return this.isSamePage
}

start() {
async start() {
if (this.state == VisitState.initialized) {
this.recordTimingMetric(TimingMetric.visitStart)
this.state = VisitState.started
this.cachedSnapshot = await this.getCachedSnapshot()
this.adapter.visitStarted(this)
this.delegate.visitStarted(this)
}
Expand Down Expand Up @@ -231,13 +232,12 @@ export class Visit {
}
}

async hasCachedSnapshot() {
return (await this.getCachedSnapshot()) != null
hasCachedSnapshot() {
return this.cachedSnapshot != null
}

async loadCachedSnapshot() {
const snapshot = await this.getCachedSnapshot()
if (snapshot) {
if (this.cachedSnapshot) {
const isPreview = await this.shouldIssueRequest()
this.render(async () => {
this.cacheSnapshot()
Expand All @@ -246,7 +246,7 @@ export class Visit {
} else {
if (this.view.renderPromise) await this.view.renderPromise

await this.renderPageSnapshot(snapshot, isPreview)
await this.renderPageSnapshot(this.cachedSnapshot, isPreview)

this.adapter.visitRendered(this)
if (!isPreview) {
Expand Down Expand Up @@ -395,7 +395,7 @@ export class Visit {
if (this.isSamePage) {
return false
} else if (this.action === "restore") {
return !(await this.hasCachedSnapshot())
return !this.hasCachedSnapshot()
} else {
return this.willRender
}
Expand Down
2 changes: 1 addition & 1 deletion src/tests/unit/deprecated_adapter_support_tests.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ test("visit proposal location includes deprecated absoluteURL property", async (
})

test("visit start location includes deprecated absoluteURL property", async () => {
Turbo.navigator.startVisit(window.location.toString(), "123")
await Turbo.navigator.startVisit(window.location.toString(), "123")
assert.equal(adapter.locations.length, 1)

const [location] = adapter.locations
Expand Down
47 changes: 28 additions & 19 deletions src/tests/unit/native_adapter_support_tests.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,11 @@ setup(() => {
Turbo.registerAdapter(adapter)
})

test("test navigator adapter is native adapter", async () => {
test("navigator adapter is native adapter", async () => {
assert.equal(adapter, Turbo.navigator.adapter)
})

test("test visit proposal location is proposed to adapter", async () => {
test("visit proposal location is proposed to adapter", async () => {
const url = new URL(window.location.toString())

Turbo.navigator.proposeVisit(url)
Expand All @@ -76,7 +76,7 @@ test("test visit proposal location is proposed to adapter", async () => {
assert.equal(visit.location, url)
})

test("test visit proposal external location is proposed to adapter", async () => {
test("visit proposal external location is proposed to adapter", async () => {
const url = new URL("https://example.com/")

Turbo.navigator.proposeVisit(url)
Expand All @@ -86,20 +86,29 @@ test("test visit proposal external location is proposed to adapter", async () =>
assert.equal(visit.location, url)
})

test("test visit started notifies adapter", async () => {
test("visit started notifies adapter", async () => {
const locatable = window.location.toString()

Turbo.navigator.startVisit(locatable)
await Turbo.navigator.startVisit(locatable)
assert.equal(adapter.startedVisits.length, 1)

const [visit] = adapter.startedVisits
assert.equal(visit.location, locatable)
})

test("test visit completed notifies adapter", async () => {
test("visit has cached snapshot returns boolean", async () => {
const locatable = window.location.toString()

Turbo.navigator.startVisit(locatable)
await Turbo.navigator.startVisit(locatable)

const [visit] = adapter.startedVisits
assert.equal(visit.hasCachedSnapshot(), false)
})

test("visit completed notifies adapter", async () => {
const locatable = window.location.toString()

await Turbo.navigator.startVisit(locatable)

const [startedVisit] = adapter.startedVisits
startedVisit.complete()
Expand All @@ -108,10 +117,10 @@ test("test visit completed notifies adapter", async () => {
assert.equal(completedVisit.location, locatable)
})

test("test visit request started notifies adapter", async () => {
test("visit request started notifies adapter", async () => {
const locatable = window.location.toString()

Turbo.navigator.startVisit(locatable)
await Turbo.navigator.startVisit(locatable)

const [startedVisit] = adapter.startedVisits
startedVisit.startRequest()
Expand All @@ -121,10 +130,10 @@ test("test visit request started notifies adapter", async () => {
assert.equal(startedVisitRequest.location, locatable)
})

test("test visit request completed notifies adapter", async () => {
test("visit request completed notifies adapter", async () => {
const locatable = window.location.toString()

Turbo.navigator.startVisit(locatable)
await Turbo.navigator.startVisit(locatable)

const [startedVisit] = adapter.startedVisits
startedVisit.recordResponse({ statusCode: 200, responseHTML: "responseHtml", redirected: false })
Expand All @@ -134,10 +143,10 @@ test("test visit request completed notifies adapter", async () => {
assert.equal(completedVisitRequest.location, locatable)
})

test("test visit request failed notifies adapter", async () => {
test("visit request failed notifies adapter", async () => {
const locatable = window.location.toString()

Turbo.navigator.startVisit(locatable)
await Turbo.navigator.startVisit(locatable)

const [startedVisit] = adapter.startedVisits
startedVisit.recordResponse({ statusCode: 404, responseHTML: "responseHtml", redirected: false })
Expand All @@ -147,10 +156,10 @@ test("test visit request failed notifies adapter", async () => {
assert.equal(failedVisitRequest.location, locatable)
})

test("test visit request finished notifies adapter", async () => {
test("visit request finished notifies adapter", async () => {
const locatable = window.location.toString()

Turbo.navigator.startVisit(locatable)
await Turbo.navigator.startVisit(locatable)

const [startedVisit] = adapter.startedVisits
startedVisit.finishRequest()
Expand All @@ -160,15 +169,15 @@ test("test visit request finished notifies adapter", async () => {
assert.equal(finishedVisitRequest.location, locatable)
})

test("test form submission started notifies adapter", async () => {
test("form submission started notifies adapter", async () => {
Turbo.navigator.formSubmissionStarted("formSubmissionStub")
assert.equal(adapter.startedFormSubmissions.length, 1)

const [startedFormSubmission] = adapter.startedFormSubmissions
assert.equal(startedFormSubmission, "formSubmissionStub")
})

test("test form submission finished notifies adapter", async () => {
test("form submission finished notifies adapter", async () => {
Turbo.navigator.formSubmissionFinished("formSubmissionStub")
assert.equal(adapter.finishedFormSubmissions.length, 1)

Expand All @@ -177,11 +186,11 @@ test("test form submission finished notifies adapter", async () => {
})


test("test visit follows redirect and proposes replace visit to adapter", async () => {
test("visit follows redirect and proposes replace visit to adapter", async () => {
const locatable = window.location.toString()
const redirectedLocation = "https://example.com"

Turbo.navigator.startVisit(locatable)
await Turbo.navigator.startVisit(locatable)

const [startedVisit] = adapter.startedVisits
startedVisit.redirectedToLocation = redirectedLocation
Expand Down

0 comments on commit e07639f

Please sign in to comment.