Skip to content

Commit

Permalink
Revert "Return Promise<void> from Turbo.visit (#650)" (#724)
Browse files Browse the repository at this point in the history
Since #650, errors are present in the console whenever initiating a
visit causes cancellation of the previous visit.

Reported in:
- #706
- #716

Reverting until a solution is found for the errors.

This reverts commit aeeaae8.
  • Loading branch information
kevinmcconnell authored Sep 19, 2022
1 parent ade31a6 commit 1ec72ac
Show file tree
Hide file tree
Showing 10 changed files with 17 additions and 87 deletions.
10 changes: 2 additions & 8 deletions src/core/drive/navigator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { PageSnapshot } from "./page_snapshot"

export type NavigatorDelegate = VisitDelegate & {
allowsVisitingLocationWithAction(location: URL, action?: Action): boolean
visitProposedToLocation(location: URL, options: Partial<VisitOptions>): Promise<void>
visitProposedToLocation(location: URL, options: Partial<VisitOptions>): void
notifyApplicationAfterVisitingSamePageLocation(oldURL: URL, newURL: URL): void
}

Expand All @@ -26,14 +26,10 @@ export class Navigator {
proposeVisit(location: URL, options: Partial<VisitOptions> = {}) {
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()
}
}

Expand All @@ -45,8 +41,6 @@ export class Navigator {
...options,
})
this.currentVisit.start()

return this.currentVisit.promise
}

submitForm(form: HTMLFormElement, submitter?: HTMLElement) {
Expand Down
12 changes: 1 addition & 11 deletions src/core/drive/visit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -85,9 +85,6 @@ export class Visit implements FetchRequestDelegate {
readonly visitCachedSnapshot: (snapshot: Snapshot) => void
readonly willRender: boolean
readonly updateHistory: boolean
readonly promise: Promise<void>

private resolvingFunctions!: ResolvingFunctions<void>

followedRedirect = false
frame?: number
Expand All @@ -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,
Expand Down Expand Up @@ -180,8 +176,6 @@ export class Visit implements FetchRequestDelegate {
}
this.cancelRender()
this.state = VisitState.canceled

this.resolvingFunctions.reject()
}
}

Expand All @@ -195,17 +189,13 @@ export class Visit implements FetchRequestDelegate {
this.adapter.visitCompleted(this)
this.delegate.visitCompleted(this)
}

this.resolvingFunctions.resolve()
}
}

fail() {
if (this.state == VisitState.started) {
this.state = VisitState.failed
this.adapter.visitFailed(this)

this.resolvingFunctions.reject()
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/core/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<VisitOptions>): Promise<void> {
return session.visit(location, options)
export function visit(location: Locatable, options?: Partial<VisitOptions>) {
session.visit(location, options)
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/core/native/adapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { FormSubmission } from "../drive/form_submission"
import { ReloadReason } from "./browser_adapter"

export interface Adapter {
visitProposedToLocation(location: URL, options?: Partial<VisitOptions>): Promise<void>
visitProposedToLocation(location: URL, options?: Partial<VisitOptions>): void
visitStarted(visit: Visit): void
visitCompleted(visit: Visit): void
visitFailed(visit: Visit): void
Expand Down
2 changes: 1 addition & 1 deletion src/core/native/browser_adapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export class BrowserAdapter implements Adapter {
}

visitProposedToLocation(location: URL, options?: Partial<VisitOptions>) {
return this.navigator.startVisit(location, options?.restorationIdentifier || uuid(), options)
this.navigator.startVisit(location, options?.restorationIdentifier || uuid(), options)
}

visitStarted(visit: Visit) {
Expand Down
6 changes: 5 additions & 1 deletion src/core/renderer.ts
Original file line number Diff line number Diff line change
@@ -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<T = unknown> = {
resolve(value: T | PromiseLike<T>): void
reject(reason?: any): void
}

export type Render<E> = (newElement: E, currentElement: E) => void

export abstract class Renderer<E extends Element, S extends Snapshot<E> = Snapshot<E>> implements BardoDelegate {
Expand Down
8 changes: 4 additions & 4 deletions src/core/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,14 +109,14 @@ export class Session
this.adapter = adapter
}

visit(location: Locatable, options: Partial<VisitOptions> = {}): Promise<void> {
visit(location: Locatable, options: Partial<VisitOptions> = {}) {
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)
}
}

Expand Down Expand Up @@ -206,7 +206,7 @@ export class Session

visitProposedToLocation(location: URL, options: Partial<VisitOptions>) {
extendURLWithDeprecatedProperties(location)
return this.adapter.visitProposedToLocation(location, options)
this.adapter.visitProposedToLocation(location, options)
}

visitStarted(visit: Visit) {
Expand Down
5 changes: 0 additions & 5 deletions src/core/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,3 @@ export type StreamSource = {
options?: boolean | EventListenerOptions
): void
}

export type ResolvingFunctions<T = unknown> = {
resolve(value: T | PromiseLike<T>): void
reject(reason?: any): void
}
51 changes: 0 additions & 51 deletions src/tests/functional/visit_tests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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")
Expand Down Expand Up @@ -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<boolean>(
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")
Expand Down
4 changes: 1 addition & 3 deletions src/tests/unit/deprecated_adapter_support_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,8 @@ export class DeprecatedAdapterSupportTest extends DOMTestCase implements Adapter

// Adapter interface

visitProposedToLocation(location: URL, _options?: Partial<VisitOptions>): Promise<void> {
visitProposedToLocation(location: URL, _options?: Partial<VisitOptions>): void {
this.locations.push(location)

return Promise.resolve()
}

visitStarted(visit: Visit): void {
Expand Down

0 comments on commit 1ec72ac

Please sign in to comment.