From 1ec72ac3236038af50a0010f0821e0664eeac087 Mon Sep 17 00:00:00 2001 From: Kevin McConnell Date: Mon, 19 Sep 2022 13:06:01 +0100 Subject: [PATCH] Revert "Return `Promise` from `Turbo.visit` (#650)" (#724) Since #650, errors are present in the console whenever initiating a visit causes cancellation of the previous visit. Reported in: - https://github.com/hotwired/turbo/issues/706 - https://github.com/hotwired/turbo/issues/716 Reverting until a solution is found for the errors. This reverts commit aeeaae8edb5f6ec6ef6b19eaf93dc060a1e67b10. --- src/core/drive/navigator.ts | 10 +--- src/core/drive/visit.ts | 12 +---- src/core/index.ts | 4 +- src/core/native/adapter.ts | 2 +- src/core/native/browser_adapter.ts | 2 +- src/core/renderer.ts | 6 ++- src/core/session.ts | 8 +-- src/core/types.ts | 5 -- src/tests/functional/visit_tests.ts | 51 ------------------- .../unit/deprecated_adapter_support_test.ts | 4 +- 10 files changed, 17 insertions(+), 87 deletions(-) diff --git a/src/core/drive/navigator.ts b/src/core/drive/navigator.ts index e52e63b92..1b8f00d42 100644 --- a/src/core/drive/navigator.ts +++ b/src/core/drive/navigator.ts @@ -9,7 +9,7 @@ import { PageSnapshot } from "./page_snapshot" export type NavigatorDelegate = VisitDelegate & { allowsVisitingLocationWithAction(location: URL, action?: Action): boolean - visitProposedToLocation(location: URL, options: Partial): Promise + visitProposedToLocation(location: URL, options: Partial): void notifyApplicationAfterVisitingSamePageLocation(oldURL: URL, newURL: URL): void } @@ -26,14 +26,10 @@ export class Navigator { proposeVisit(location: URL, options: Partial = {}) { if (this.delegate.allowsVisitingLocationWithAction(location, options.action)) { if (locationIsVisitable(location, this.view.snapshot.rootLocation)) { - return this.delegate.visitProposedToLocation(location, options) + this.delegate.visitProposedToLocation(location, options) } else { window.location.href = location.toString() - - return Promise.resolve() } - } else { - return Promise.reject() } } @@ -45,8 +41,6 @@ export class Navigator { ...options, }) this.currentVisit.start() - - return this.currentVisit.promise } submitForm(form: HTMLFormElement, submitter?: HTMLElement) { diff --git a/src/core/drive/visit.ts b/src/core/drive/visit.ts index b4237b7ad..989f105fa 100644 --- a/src/core/drive/visit.ts +++ b/src/core/drive/visit.ts @@ -5,7 +5,7 @@ import { History } from "./history" import { getAnchor } from "../url" import { Snapshot } from "../snapshot" import { PageSnapshot } from "./page_snapshot" -import { Action, ResolvingFunctions } from "../types" +import { Action } from "../types" import { getHistoryMethodForAction, uuid } from "../../util" import { PageView } from "./page_view" import { StreamMessage } from "../streams/stream_message" @@ -85,9 +85,6 @@ export class Visit implements FetchRequestDelegate { readonly visitCachedSnapshot: (snapshot: Snapshot) => void readonly willRender: boolean readonly updateHistory: boolean - readonly promise: Promise - - private resolvingFunctions!: ResolvingFunctions followedRedirect = false frame?: number @@ -113,7 +110,6 @@ export class Visit implements FetchRequestDelegate { this.delegate = delegate this.location = location this.restorationIdentifier = restorationIdentifier || uuid() - this.promise = new Promise((resolve, reject) => (this.resolvingFunctions = { resolve, reject })) const { action, @@ -180,8 +176,6 @@ export class Visit implements FetchRequestDelegate { } this.cancelRender() this.state = VisitState.canceled - - this.resolvingFunctions.reject() } } @@ -195,8 +189,6 @@ export class Visit implements FetchRequestDelegate { this.adapter.visitCompleted(this) this.delegate.visitCompleted(this) } - - this.resolvingFunctions.resolve() } } @@ -204,8 +196,6 @@ export class Visit implements FetchRequestDelegate { if (this.state == VisitState.started) { this.state = VisitState.failed this.adapter.visitFailed(this) - - this.resolvingFunctions.reject() } } diff --git a/src/core/index.ts b/src/core/index.ts index cd39d7e64..3ab8d8ff5 100644 --- a/src/core/index.ts +++ b/src/core/index.ts @@ -64,8 +64,8 @@ export function registerAdapter(adapter: Adapter) { * @param options.snapshotHTML Cached snapshot to render * @param options.response Response of the specified location */ -export function visit(location: Locatable, options?: Partial): Promise { - return session.visit(location, options) +export function visit(location: Locatable, options?: Partial) { + session.visit(location, options) } /** diff --git a/src/core/native/adapter.ts b/src/core/native/adapter.ts index ddf577dc2..3ecfd9b22 100644 --- a/src/core/native/adapter.ts +++ b/src/core/native/adapter.ts @@ -3,7 +3,7 @@ import { FormSubmission } from "../drive/form_submission" import { ReloadReason } from "./browser_adapter" export interface Adapter { - visitProposedToLocation(location: URL, options?: Partial): Promise + visitProposedToLocation(location: URL, options?: Partial): void visitStarted(visit: Visit): void visitCompleted(visit: Visit): void visitFailed(visit: Visit): void diff --git a/src/core/native/browser_adapter.ts b/src/core/native/browser_adapter.ts index c7e53a4e1..9b5988e3c 100644 --- a/src/core/native/browser_adapter.ts +++ b/src/core/native/browser_adapter.ts @@ -24,7 +24,7 @@ export class BrowserAdapter implements Adapter { } visitProposedToLocation(location: URL, options?: Partial) { - return this.navigator.startVisit(location, options?.restorationIdentifier || uuid(), options) + this.navigator.startVisit(location, options?.restorationIdentifier || uuid(), options) } visitStarted(visit: Visit) { diff --git a/src/core/renderer.ts b/src/core/renderer.ts index 91d917aa0..5e199999c 100644 --- a/src/core/renderer.ts +++ b/src/core/renderer.ts @@ -1,8 +1,12 @@ -import { ResolvingFunctions } from "./types" import { Bardo, BardoDelegate } from "./bardo" import { Snapshot } from "./snapshot" import { ReloadReason } from "./native/browser_adapter" +type ResolvingFunctions = { + resolve(value: T | PromiseLike): void + reject(reason?: any): void +} + export type Render = (newElement: E, currentElement: E) => void export abstract class Renderer = Snapshot> implements BardoDelegate { diff --git a/src/core/session.ts b/src/core/session.ts index 7471e7ec0..2d1cb3685 100644 --- a/src/core/session.ts +++ b/src/core/session.ts @@ -109,14 +109,14 @@ export class Session this.adapter = adapter } - visit(location: Locatable, options: Partial = {}): Promise { + visit(location: Locatable, options: Partial = {}) { const frameElement = options.frame ? document.getElementById(options.frame) : null if (frameElement instanceof FrameElement) { frameElement.src = location.toString() - return frameElement.loaded + frameElement.loaded } else { - return this.navigator.proposeVisit(expandURL(location), options) + this.navigator.proposeVisit(expandURL(location), options) } } @@ -206,7 +206,7 @@ export class Session visitProposedToLocation(location: URL, options: Partial) { extendURLWithDeprecatedProperties(location) - return this.adapter.visitProposedToLocation(location, options) + this.adapter.visitProposedToLocation(location, options) } visitStarted(visit: Visit) { diff --git a/src/core/types.ts b/src/core/types.ts index de13bbc3f..673ebdfff 100644 --- a/src/core/types.ts +++ b/src/core/types.ts @@ -18,8 +18,3 @@ export type StreamSource = { options?: boolean | EventListenerOptions ): void } - -export type ResolvingFunctions = { - resolve(value: T | PromiseLike): void - reject(reason?: any): void -} diff --git a/src/tests/functional/visit_tests.ts b/src/tests/functional/visit_tests.ts index 084d7f55d..0729dbff1 100644 --- a/src/tests/functional/visit_tests.ts +++ b/src/tests/functional/visit_tests.ts @@ -40,24 +40,6 @@ test("test programmatically visiting a same-origin location", async ({ page }) = assert.ok(timing) }) -test("test programmatically Turbo.visit-ing a same-origin location", async ({ page }) => { - const urlBeforeVisit = page.url() - await page.evaluate(async () => await window.Turbo.visit("/src/tests/fixtures/one.html")) - - const urlAfterVisit = page.url() - assert.notEqual(urlBeforeVisit, urlAfterVisit) - assert.equal(await visitAction(page), "advance") - - const { url: urlFromBeforeVisitEvent } = await nextEventNamed(page, "turbo:before-visit") - assert.equal(urlFromBeforeVisitEvent, urlAfterVisit) - - const { url: urlFromVisitEvent } = await nextEventNamed(page, "turbo:visit") - assert.equal(urlFromVisitEvent, urlAfterVisit) - - const { timing } = await nextEventNamed(page, "turbo:load") - assert.ok(timing) -}) - test("skip programmatically visiting a cross-origin location falls back to window.location", async ({ page }) => { const urlBeforeVisit = page.url() await visitLocation(page, "about:blank") @@ -67,17 +49,6 @@ test("skip programmatically visiting a cross-origin location falls back to windo assert.equal(await visitAction(page), "load") }) -test("skip programmatically Turbo.visit-ing a cross-origin location falls back to window.location", async ({ - page, -}) => { - const urlBeforeVisit = page.url() - await page.evaluate(async () => await window.Turbo.visit("about:blank")) - - const urlAfterVisit = page.url() - assert.notEqual(urlBeforeVisit, urlAfterVisit) - assert.equal(await visitAction(page), "load") -}) - test("test visiting a location served with a non-HTML content type", async ({ page }) => { const urlBeforeVisit = page.url() await visitLocation(page, "/src/tests/fixtures/svg.svg") @@ -107,28 +78,6 @@ test("test canceling a before-visit event prevents navigation", async ({ page }) assert.equal(urlAfterVisit, urlBeforeVisit) }) -test("test canceling a before-visit event prevents a Turbo.visit-initiated navigation", async ({ page }) => { - await cancelNextVisit(page) - const urlBeforeVisit = page.url() - - assert.notOk( - await willChangeBody(page, async () => { - await page.evaluate(async () => { - try { - return await window.Turbo.visit("/src/tests/fixtures/one.html") - } catch { - const title = document.querySelector("h1") - if (title) title.innerHTML = "Visit canceled" - } - }) - }) - ) - - const urlAfterVisit = page.url() - assert.equal(urlAfterVisit, urlBeforeVisit) - assert.equal(await page.textContent("h1"), "Visit canceled") -}) - test("test navigation by history is not cancelable", async ({ page }) => { await page.click("#same-origin-link") await nextEventNamed(page, "turbo:load") diff --git a/src/tests/unit/deprecated_adapter_support_test.ts b/src/tests/unit/deprecated_adapter_support_test.ts index fa931b404..071ffe4f4 100644 --- a/src/tests/unit/deprecated_adapter_support_test.ts +++ b/src/tests/unit/deprecated_adapter_support_test.ts @@ -34,10 +34,8 @@ export class DeprecatedAdapterSupportTest extends DOMTestCase implements Adapter // Adapter interface - visitProposedToLocation(location: URL, _options?: Partial): Promise { + visitProposedToLocation(location: URL, _options?: Partial): void { this.locations.push(location) - - return Promise.resolve() } visitStarted(visit: Visit): void {