From e07639f1907a9da6eca344e0afcd37ca9e73c0df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alberto=20Fern=C3=A1ndez-Capel?= Date: Fri, 3 Nov 2023 14:39:40 +0000 Subject: [PATCH] Move cache snapshot loading to Visit#start() (#1056) * Move cache snapshot loading to Visit#start() The cache interface changed in https://github.com/hotwired/turbo/pull/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 --- src/core/drive/navigator.js | 2 +- src/core/drive/visit.js | 14 +++--- .../unit/deprecated_adapter_support_tests.js | 2 +- .../unit/native_adapter_support_tests.js | 47 +++++++++++-------- 4 files changed, 37 insertions(+), 28 deletions(-) diff --git a/src/core/drive/navigator.js b/src/core/drive/navigator.js index 00a743b8b..0d7259913 100644 --- a/src/core/drive/navigator.js +++ b/src/core/drive/navigator.js @@ -21,7 +21,7 @@ export class Navigator { referrer: this.location, ...options }) - this.currentVisit.start() + return this.currentVisit.start() } submitForm(form, submitter) { diff --git a/src/core/drive/visit.js b/src/core/drive/visit.js index 6cf1b52c7..d67258cdc 100644 --- a/src/core/drive/visit.js +++ b/src/core/drive/visit.js @@ -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) } @@ -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() @@ -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) { @@ -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 } diff --git a/src/tests/unit/deprecated_adapter_support_tests.js b/src/tests/unit/deprecated_adapter_support_tests.js index 799bc6100..8d93650ef 100644 --- a/src/tests/unit/deprecated_adapter_support_tests.js +++ b/src/tests/unit/deprecated_adapter_support_tests.js @@ -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 diff --git a/src/tests/unit/native_adapter_support_tests.js b/src/tests/unit/native_adapter_support_tests.js index c199c1f3a..3926b0823 100644 --- a/src/tests/unit/native_adapter_support_tests.js +++ b/src/tests/unit/native_adapter_support_tests.js @@ -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) @@ -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) @@ -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() @@ -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() @@ -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 }) @@ -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 }) @@ -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() @@ -160,7 +169,7 @@ 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) @@ -168,7 +177,7 @@ test("test form submission started notifies adapter", async () => { 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) @@ -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