From bbd80534cbcedfbcc9b6bb30f5eb56210a59fa7b Mon Sep 17 00:00:00 2001 From: Kevin McConnell Date: Mon, 1 Aug 2022 19:04:09 +0100 Subject: [PATCH 1/4] Don't convert `data-turbo-stream` links to forms (#647) The initial implementation of the `data-turbo-stream` attribute worked by treating requests with that attribute as form submissions (which would usually be `GET` form submissions, unless a different method type was indicated). This allowed us to include the `data-turbo-stream` logic in `FormSubmission`, which was responsible for properly setting the `Accept` header. However, there are some downsides to submitting the requests as form submissions. For example, a `GET` form submission does not include the search portion of the URL. Additionally, servers are free to respond to `data-turbo-stream` requests with plain HTML responses, and in that case we don't want Turbo's handling of the response to differ from what it would have been if the `data-turbo-stream` attribute wasn't present. This commit takes a different approach. In this version, elements that have `data-turbo-stream` continue to be processed by the same object that they would otherwise, whether that's a `FormSubmission`, a `Visit` or a `FrameController`. However each of these objects is now aware of the `data-turbo-stream` attribute, and each will include the Turbo Stream MIME type when appropriate. This minimizes the impact of `data-turbo-stream` so that the only effect it has is on the inclusion of that MIME type. --- src/core/drive/form_submission.ts | 2 +- src/core/drive/visit.ts | 14 +++++++++++++- src/core/frames/frame_controller.ts | 18 ++++++++++++++++-- src/core/session.ts | 4 +++- src/http/fetch_request.ts | 4 ++++ src/observers/form_link_click_observer.ts | 2 +- src/tests/fixtures/form.html | 1 - src/tests/fixtures/visit.html | 1 + src/tests/functional/form_submission_tests.ts | 11 ++--------- src/tests/functional/visit_tests.ts | 9 +++++++++ 10 files changed, 50 insertions(+), 16 deletions(-) diff --git a/src/core/drive/form_submission.ts b/src/core/drive/form_submission.ts index babeb1259..6ff0d336e 100644 --- a/src/core/drive/form_submission.ts +++ b/src/core/drive/form_submission.ts @@ -161,7 +161,7 @@ export class FormSubmission { } if (this.requestAcceptsTurboStreamResponse(request)) { - headers["Accept"] = [StreamMessage.contentType, headers["Accept"]].join(", ") + request.acceptResponseType(StreamMessage.contentType) } } diff --git a/src/core/drive/visit.ts b/src/core/drive/visit.ts index a0f92293c..6d709f2ff 100644 --- a/src/core/drive/visit.ts +++ b/src/core/drive/visit.ts @@ -1,5 +1,5 @@ import { Adapter } from "../native/adapter" -import { FetchMethod, FetchRequest, FetchRequestDelegate } from "../../http/fetch_request" +import { FetchMethod, FetchRequest, FetchRequestDelegate, FetchRequestHeaders } from "../../http/fetch_request" import { FetchResponse } from "../../http/fetch_response" import { History } from "./history" import { getAnchor } from "../url" @@ -8,6 +8,7 @@ import { PageSnapshot } from "./page_snapshot" import { Action, ResolvingFunctions } from "../types" import { getHistoryMethodForAction, uuid } from "../../util" import { PageView } from "./page_view" +import { StreamMessage } from "../streams/stream_message" export interface VisitDelegate { readonly adapter: Adapter @@ -49,6 +50,7 @@ export type VisitOptions = { restorationIdentifier?: string shouldCacheSnapshot: boolean frame?: string + acceptsStreamResponse: boolean } const defaultOptions: VisitOptions = { @@ -58,6 +60,7 @@ const defaultOptions: VisitOptions = { willRender: true, updateHistory: true, shouldCacheSnapshot: true, + acceptsStreamResponse: false, } export type VisitResponse = { @@ -96,6 +99,7 @@ export class Visit implements FetchRequestDelegate { response?: VisitResponse scrolled = false shouldCacheSnapshot = true + acceptsStreamResponse = false snapshotHTML?: string snapshotCached = false state = VisitState.initialized @@ -121,6 +125,7 @@ export class Visit implements FetchRequestDelegate { willRender, updateHistory, shouldCacheSnapshot, + acceptsStreamResponse, } = { ...defaultOptions, ...options, @@ -136,6 +141,7 @@ export class Visit implements FetchRequestDelegate { this.updateHistory = updateHistory this.scrolled = !willRender this.shouldCacheSnapshot = shouldCacheSnapshot + this.acceptsStreamResponse = acceptsStreamResponse } get adapter() { @@ -335,6 +341,12 @@ export class Visit implements FetchRequestDelegate { // Fetch request delegate + prepareHeadersForRequest(headers: FetchRequestHeaders, request: FetchRequest) { + if (this.acceptsStreamResponse) { + request.acceptResponseType(StreamMessage.contentType) + } + } + requestStarted() { this.startRequest() } diff --git a/src/core/frames/frame_controller.ts b/src/core/frames/frame_controller.ts index 85254930d..94f8c42e6 100644 --- a/src/core/frames/frame_controller.ts +++ b/src/core/frames/frame_controller.ts @@ -30,6 +30,7 @@ import { session } from "../index" import { isAction, Action } from "../types" import { VisitOptions } from "../drive/visit" import { TurboBeforeFrameRenderEvent } from "../session" +import { StreamMessage } from "../streams/stream_message" export class FrameController implements @@ -59,6 +60,7 @@ export class FrameController private frame?: FrameElement readonly restorationIdentifier: string private previousFrameElement?: FrameElement + private currentNavigationElement?: Element constructor(element: FrameElement) { this.element = element @@ -217,8 +219,12 @@ export class FrameController // Fetch request delegate - prepareHeadersForRequest(headers: FetchRequestHeaders, _request: FetchRequest) { + prepareHeadersForRequest(headers: FetchRequestHeaders, request: FetchRequest) { headers["Turbo-Frame"] = this.id + + if (this.currentNavigationElement?.hasAttribute("data-turbo-stream")) { + request.acceptResponseType(StreamMessage.contentType) + } } requestStarted(_request: FetchRequest) { @@ -340,7 +346,9 @@ export class FrameController this.proposeVisitIfNavigatedWithAction(frame, element, submitter) - frame.src = url + this.withCurrentNavigationElement(element, () => { + frame.src = url + }) } private proposeVisitIfNavigatedWithAction(frame: FrameElement, element: Element, submitter?: HTMLElement) { @@ -505,6 +513,12 @@ export class FrameController callback() this.ignoredAttributes.delete(attributeName) } + + private withCurrentNavigationElement(element: Element, callback: () => void) { + this.currentNavigationElement = element + callback() + delete this.currentNavigationElement + } } function getFrameElementById(id: string | null) { diff --git a/src/core/session.ts b/src/core/session.ts index 23fcc1eaa..ac8408ca9 100644 --- a/src/core/session.ts +++ b/src/core/session.ts @@ -191,7 +191,9 @@ export class Session followedLinkToLocation(link: Element, location: URL) { const action = this.getActionForLink(link) - this.visit(location.href, { action }) + const acceptsStreamResponse = link.hasAttribute("data-turbo-stream") + + this.visit(location.href, { action, acceptsStreamResponse }) } // Navigator delegate diff --git a/src/http/fetch_request.ts b/src/http/fetch_request.ts index 35952fc7e..459a9b786 100644 --- a/src/http/fetch_request.ts +++ b/src/http/fetch_request.ts @@ -158,6 +158,10 @@ export class FetchRequest { return this.abortController.signal } + acceptResponseType(mimeType: string) { + this.headers["Accept"] = [mimeType, this.headers["Accept"]].join(", ") + } + private async allowRequestToBeIntercepted(fetchOptions: RequestInit) { const requestInterception = new Promise((resolve) => (this.resolveRequestPromise = resolve)) const event = dispatch("turbo:before-fetch-request", { diff --git a/src/observers/form_link_click_observer.ts b/src/observers/form_link_click_observer.ts index 6ff364de8..c6949a851 100644 --- a/src/observers/form_link_click_observer.ts +++ b/src/observers/form_link_click_observer.ts @@ -25,7 +25,7 @@ export class FormLinkClickObserver implements LinkClickObserverDelegate { willFollowLinkToLocation(link: Element, location: URL, originalEvent: MouseEvent): boolean { return ( this.delegate.willSubmitFormLinkToLocation(link, location, originalEvent) && - (link.hasAttribute("data-turbo-method") || link.hasAttribute("data-turbo-stream")) + link.hasAttribute("data-turbo-method") ) } diff --git a/src/tests/fixtures/form.html b/src/tests/fixtures/form.html index b149d1811..32a20c7ae 100644 --- a/src/tests/fixtures/form.html +++ b/src/tests/fixtures/form.html @@ -296,7 +296,6 @@

Frame: Form

Method link outside frame
Stream link outside frame - Stream link (no method) outside frame
Method link within form outside frame
Stream link within form outside frame diff --git a/src/tests/fixtures/visit.html b/src/tests/fixtures/visit.html index 138a72c36..d138fd384 100644 --- a/src/tests/fixtures/visit.html +++ b/src/tests/fixtures/visit.html @@ -19,6 +19,7 @@

Visit

one.html

+

Stream link with ?key=value

diff --git a/src/tests/functional/form_submission_tests.ts b/src/tests/functional/form_submission_tests.ts index 4957068ae..ed5ea5a67 100644 --- a/src/tests/functional/form_submission_tests.ts +++ b/src/tests/functional/form_submission_tests.ts @@ -886,17 +886,10 @@ test("test stream link GET method form submission inside frame", async ({ page } test("test stream link inside frame", async ({ page }) => { await page.click("#stream-link-inside-frame") - const { fetchOptions } = await nextEventNamed(page, "turbo:before-fetch-request") - - assert.ok(fetchOptions.headers["Accept"].includes("text/vnd.turbo-stream.html")) -}) - -test("test stream link outside frame", async ({ page }) => { - await page.click("#stream-link-outside-frame") - - const { fetchOptions } = await nextEventNamed(page, "turbo:before-fetch-request") + const { fetchOptions, url } = await nextEventNamed(page, "turbo:before-fetch-request") assert.ok(fetchOptions.headers["Accept"].includes("text/vnd.turbo-stream.html")) + assert.equal(getSearchParam(url, "content"), "Link!") }) test("test link method form submission within form inside frame", async ({ page }) => { diff --git a/src/tests/functional/visit_tests.ts b/src/tests/functional/visit_tests.ts index c804adeb3..e03fbc65c 100644 --- a/src/tests/functional/visit_tests.ts +++ b/src/tests/functional/visit_tests.ts @@ -2,6 +2,7 @@ import { Page, test } from "@playwright/test" import { assert } from "chai" import { get } from "http" import { + getSearchParam, isScrolledToSelector, isScrolledToTop, nextBeat, @@ -178,6 +179,14 @@ test("test turbo:before-fetch-response open new site", async ({ page }) => { assert.isTrue(fetchResponseResult.responseHTML.indexOf("An element with an ID") > -1) }) +test("test visits with data-turbo-stream include MIME type & search params", async ({ page }) => { + await page.click("#stream-link") + const { fetchOptions, url } = await nextEventNamed(page, "turbo:before-fetch-request") + + assert.ok(fetchOptions.headers["Accept"].includes("text/vnd.turbo-stream.html")) + assert.equal(getSearchParam(url, "key"), "value") +}) + test("test cache does not override response after redirect", async ({ page }) => { await page.evaluate(() => { const cachedElement = document.createElement("some-cached-element") From 11591fdb51ae8718c8def664ebb66cc40fb3604f Mon Sep 17 00:00:00 2001 From: Sean Doyle Date: Mon, 1 Aug 2022 14:05:53 -0400 Subject: [PATCH 2/4] Replace LinkInterceptor with LinkClickObserver (#412) Follow-up to https://github.com/hotwired/turbo/pull/382 --- In the same style as [hotwired/turbo#382][], replace instances of `LinkInterceptor` with `LinkClickObserver`, making the necessary interface changes in classes that used to extend `LinkInterceptorDelegate`. Conditional logic that was once covered in the `LinkInterceptor` is moved into the predicate methods of the delegates (for example, the `FrameController.willFollowLinkToLocation` and `FrameRedirector.willFollowLinkToLocation`). Since the recently introduced [FormLinkInterceptor][] was named after the `LinkInterceptor`, this commit also renames that class (and its call sites) to match the `LinkClickObserver`-suffix, along with the `LinkInterceptorDelegate` method structure. [hotwired/turbo#382]: https://github.com/hotwired/turbo/pull/382 [FormLinkInterceptor]: https://github.com/hotwired/turbo/commit/f8a94c5f761c4f080d68a87459d3916eb4fb6993 --- src/core/frames/frame_controller.ts | 22 ++++----- src/core/frames/frame_redirector.ts | 36 +++++++++----- src/core/frames/link_interceptor.ts | 57 ----------------------- src/core/session.ts | 2 +- src/elements/frame_element.ts | 4 +- src/observers/form_link_click_observer.ts | 8 ++-- 6 files changed, 42 insertions(+), 87 deletions(-) delete mode 100644 src/core/frames/link_interceptor.ts diff --git a/src/core/frames/frame_controller.ts b/src/core/frames/frame_controller.ts index 94f8c42e6..d5ddd45fe 100644 --- a/src/core/frames/frame_controller.ts +++ b/src/core/frames/frame_controller.ts @@ -23,7 +23,7 @@ import { ViewDelegate, ViewRenderOptions } from "../view" import { getAction, expandURL, urlsAreEqual, locationIsVisitable } from "../url" import { FormSubmitObserver, FormSubmitObserverDelegate } from "../../observers/form_submit_observer" import { FrameView } from "./frame_view" -import { LinkInterceptor, LinkInterceptorDelegate } from "./link_interceptor" +import { LinkClickObserver, LinkClickObserverDelegate } from "../../observers/link_click_observer" import { FormLinkClickObserver, FormLinkClickObserverDelegate } from "../../observers/form_link_click_observer" import { FrameRenderer } from "./frame_renderer" import { session } from "../index" @@ -40,14 +40,14 @@ export class FrameController FormSubmissionDelegate, FrameElementDelegate, FormLinkClickObserverDelegate, - LinkInterceptorDelegate, + LinkClickObserverDelegate, ViewDelegate> { readonly element: FrameElement readonly view: FrameView readonly appearanceObserver: AppearanceObserver readonly formLinkClickObserver: FormLinkClickObserver - readonly linkInterceptor: LinkInterceptor + readonly linkClickObserver: LinkClickObserver readonly formSubmitObserver: FormSubmitObserver formSubmission?: FormSubmission fetchResponseLoaded = (_fetchResponse: FetchResponse) => {} @@ -67,7 +67,7 @@ export class FrameController this.view = new FrameView(this, this.element) this.appearanceObserver = new AppearanceObserver(this, this.element) this.formLinkClickObserver = new FormLinkClickObserver(this, this.element) - this.linkInterceptor = new LinkInterceptor(this, this.element) + this.linkClickObserver = new LinkClickObserver(this, this.element) this.restorationIdentifier = uuid() this.formSubmitObserver = new FormSubmitObserver(this, this.element) } @@ -81,7 +81,7 @@ export class FrameController this.loadSourceURL() } this.formLinkClickObserver.start() - this.linkInterceptor.start() + this.linkClickObserver.start() this.formSubmitObserver.start() } } @@ -91,7 +91,7 @@ export class FrameController this.connected = false this.appearanceObserver.stop() this.formLinkClickObserver.stop() - this.linkInterceptor.stop() + this.linkClickObserver.stop() this.formSubmitObserver.stop() } } @@ -182,7 +182,7 @@ export class FrameController // Form link click observer delegate willSubmitFormLinkToLocation(link: Element): boolean { - return this.shouldInterceptNavigation(link) + return link.closest("turbo-frame") == this.element && this.shouldInterceptNavigation(link) } submittedFormLinkToLocation(link: Element, _location: URL, form: HTMLFormElement): void { @@ -190,14 +190,14 @@ export class FrameController if (frame) form.setAttribute("data-turbo-frame", frame.id) } - // Link interceptor delegate + // Link click observer delegate - shouldInterceptLinkClick(element: Element, _url: string) { + willFollowLinkToLocation(element: Element) { return this.shouldInterceptNavigation(element) } - linkClickIntercepted(element: Element, url: string) { - this.navigateFrame(element, url) + followedLinkToLocation(element: Element, location: URL) { + this.navigateFrame(element, location.href) } // Form submit observer delegate diff --git a/src/core/frames/frame_redirector.ts b/src/core/frames/frame_redirector.ts index 93c9bbc5f..8019f01ad 100644 --- a/src/core/frames/frame_redirector.ts +++ b/src/core/frames/frame_redirector.ts @@ -1,37 +1,40 @@ import { FormSubmitObserver, FormSubmitObserverDelegate } from "../../observers/form_submit_observer" import { FrameElement } from "../../elements/frame_element" -import { LinkInterceptor, LinkInterceptorDelegate } from "./link_interceptor" import { expandURL, getAction, locationIsVisitable } from "../url" +import { LinkClickObserver, LinkClickObserverDelegate } from "../../observers/link_click_observer" +import { Session } from "../session" -export class FrameRedirector implements LinkInterceptorDelegate, FormSubmitObserverDelegate { +export class FrameRedirector implements LinkClickObserverDelegate, FormSubmitObserverDelegate { + readonly session: Session readonly element: Element - readonly linkInterceptor: LinkInterceptor + readonly linkClickObserver: LinkClickObserver readonly formSubmitObserver: FormSubmitObserver - constructor(element: Element) { + constructor(session: Session, element: Element) { + this.session = session this.element = element - this.linkInterceptor = new LinkInterceptor(this, element) + this.linkClickObserver = new LinkClickObserver(this, element) this.formSubmitObserver = new FormSubmitObserver(this, element) } start() { - this.linkInterceptor.start() + this.linkClickObserver.start() this.formSubmitObserver.start() } stop() { - this.linkInterceptor.stop() + this.linkClickObserver.stop() this.formSubmitObserver.stop() } - shouldInterceptLinkClick(element: Element, _url: string) { + willFollowLinkToLocation(element: Element) { return this.shouldRedirect(element) } - linkClickIntercepted(element: Element, url: string) { + followedLinkToLocation(element: Element, url: URL) { const frame = this.findFrameElement(element) if (frame) { - frame.delegate.linkClickIntercepted(element, url) + frame.delegate.followedLinkToLocation(element, url) } } @@ -59,8 +62,17 @@ export class FrameRedirector implements LinkInterceptorDelegate, FormSubmitObser } private shouldRedirect(element: Element, submitter?: HTMLElement) { - const frame = this.findFrameElement(element, submitter) - return frame ? frame != element.closest("turbo-frame") : false + const isNavigatable = + element instanceof HTMLFormElement + ? this.session.submissionIsNavigatable(element, submitter) + : this.session.elementIsNavigatable(element) + + if (isNavigatable) { + const frame = this.findFrameElement(element, submitter) + return frame ? frame != element.closest("turbo-frame") : false + } else { + return false + } } private findFrameElement(element: Element, submitter?: HTMLElement) { diff --git a/src/core/frames/link_interceptor.ts b/src/core/frames/link_interceptor.ts deleted file mode 100644 index 65ff1066f..000000000 --- a/src/core/frames/link_interceptor.ts +++ /dev/null @@ -1,57 +0,0 @@ -import { TurboClickEvent, TurboBeforeVisitEvent } from "../session" - -export interface LinkInterceptorDelegate { - shouldInterceptLinkClick(element: Element, url: string): boolean - linkClickIntercepted(element: Element, url: string): void -} - -export class LinkInterceptor { - readonly delegate: LinkInterceptorDelegate - readonly element: Element - private clickEvent?: Event - - constructor(delegate: LinkInterceptorDelegate, element: Element) { - this.delegate = delegate - this.element = element - } - - start() { - this.element.addEventListener("click", this.clickBubbled) - document.addEventListener("turbo:click", this.linkClicked) - document.addEventListener("turbo:before-visit", this.willVisit) - } - - stop() { - this.element.removeEventListener("click", this.clickBubbled) - document.removeEventListener("turbo:click", this.linkClicked) - document.removeEventListener("turbo:before-visit", this.willVisit) - } - - clickBubbled = (event: Event) => { - if (this.respondsToEventTarget(event.target)) { - this.clickEvent = event - } else { - delete this.clickEvent - } - } - - linkClicked = ((event: TurboClickEvent) => { - if (this.clickEvent && this.respondsToEventTarget(event.target) && event.target instanceof Element) { - if (this.delegate.shouldInterceptLinkClick(event.target, event.detail.url)) { - this.clickEvent.preventDefault() - event.preventDefault() - this.delegate.linkClickIntercepted(event.target, event.detail.url) - } - } - delete this.clickEvent - }) - - willVisit = ((_event: TurboBeforeVisitEvent) => { - delete this.clickEvent - }) - - respondsToEventTarget(target: EventTarget | null) { - const element = target instanceof Element ? target : target instanceof Node ? target.parentElement : null - return element && element.closest("turbo-frame, html") == this.element - } -} diff --git a/src/core/session.ts b/src/core/session.ts index ac8408ca9..992f49deb 100644 --- a/src/core/session.ts +++ b/src/core/session.ts @@ -59,7 +59,7 @@ export class Session readonly scrollObserver = new ScrollObserver(this) readonly streamObserver = new StreamObserver(this) readonly formLinkClickObserver = new FormLinkClickObserver(this, document.documentElement) - readonly frameRedirector = new FrameRedirector(document.documentElement) + readonly frameRedirector = new FrameRedirector(this, document.documentElement) drive = true enabled = true diff --git a/src/elements/frame_element.ts b/src/elements/frame_element.ts index 3e7676cad..ca9218c9b 100644 --- a/src/elements/frame_element.ts +++ b/src/elements/frame_element.ts @@ -1,6 +1,6 @@ import { FetchResponse } from "../http/fetch_response" import { Snapshot } from "../core/snapshot" -import { LinkInterceptorDelegate } from "../core/frames/link_interceptor" +import { LinkClickObserverDelegate } from "../observers/link_click_observer" import { FormSubmitObserverDelegate } from "../observers/form_submit_observer" export enum FrameLoadingStyle { @@ -10,7 +10,7 @@ export enum FrameLoadingStyle { export type FrameElementObservedAttribute = keyof FrameElement & ("disabled" | "complete" | "loading" | "src") -export interface FrameElementDelegate extends LinkInterceptorDelegate, FormSubmitObserverDelegate { +export interface FrameElementDelegate extends LinkClickObserverDelegate, FormSubmitObserverDelegate { connect(): void disconnect(): void completeChanged(): void diff --git a/src/observers/form_link_click_observer.ts b/src/observers/form_link_click_observer.ts index c6949a851..0e6248bd3 100644 --- a/src/observers/form_link_click_observer.ts +++ b/src/observers/form_link_click_observer.ts @@ -6,20 +6,20 @@ export type FormLinkClickObserverDelegate = { } export class FormLinkClickObserver implements LinkClickObserverDelegate { - readonly linkInterceptor: LinkClickObserver + readonly linkClickObserver: LinkClickObserver readonly delegate: FormLinkClickObserverDelegate constructor(delegate: FormLinkClickObserverDelegate, element: HTMLElement) { this.delegate = delegate - this.linkInterceptor = new LinkClickObserver(this, element) + this.linkClickObserver = new LinkClickObserver(this, element) } start() { - this.linkInterceptor.start() + this.linkClickObserver.start() } stop() { - this.linkInterceptor.stop() + this.linkClickObserver.stop() } willFollowLinkToLocation(link: Element, location: URL, originalEvent: MouseEvent): boolean { From 60c646383b3c0355242d2e3c69f80742efb266be Mon Sep 17 00:00:00 2001 From: Sean Doyle Date: Mon, 1 Aug 2022 19:38:44 -0400 Subject: [PATCH 3/4] Return `Promise` from `FrameElement.reload` (#661) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Return `Promise` from `FrameElement.reload` Following the pattern established by `Turbo.visit(...)` and `Turbo.visit(..., { frame: "..." })`, return the `FrameElement.loaded` promise from calls to `FrameElement.reload()`. That way, callers can block on it: ```js const frame = document.getElementById("my-frame") await frame.reload() // ... ``` * Resolve flaky navigation tests Switch the tests from a time-based waiting mechanism (1200 milliseconds) to an event-based waiting mechanism (waiting for `turbo:load`). ``` 2) [firefox] › navigation_tests.ts:341:1 › test double-clicking on a link ======================== AssertionError: expected '/src/tests/fixtures/navigation.html' to equal '/__turbo/delayed_response' 344 | 345 | await nextBody(page, 1200) > 346 | assert.equal(pathname(page.url()), "/__turbo/delayed_response") | ^ 347 | assert.equal(await visitAction(page), "advance") 348 | }) 349 | at /home/runner/work/turbo/turbo/src/tests/functional/navigation_tests.ts:346:10 1) [firefox] › navigation_tests.ts:131:1 › test following a same-origin POST form button[data-turbo-action=replace] page.click: Target closed =========================== logs =========================== waiting for selector "#same-origin-replace-form-submitter-post button" selector resolved to visible attempting click action waiting for element to be visible, enabled and stable element is visible, enabled and stable scrolling into view if needed ============================================================ 130 | 131 | test("test following a same-origin POST form button[data-turbo-action=replace]", async ({ page }) => { > 132 | page.click("#same-origin-replace-form-submitter-post button") | ^ 133 | await nextBody(page) 134 | 135 | assert.equal(pathname(page.url()), "/src/tests/fixtures/one.html") at /home/runner/work/turbo/turbo/src/tests/functional/navigation_tests.ts:132:8 ``` --- src/elements/frame_element.ts | 3 ++- src/tests/functional/loading_tests.ts | 9 ++++----- src/tests/functional/navigation_tests.ts | 13 ++++++++----- 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/src/elements/frame_element.ts b/src/elements/frame_element.ts index ca9218c9b..a995da374 100644 --- a/src/elements/frame_element.ts +++ b/src/elements/frame_element.ts @@ -62,11 +62,12 @@ export class FrameElement extends HTMLElement { this.delegate.disconnect() } - reload() { + reload(): Promise { const { src } = this this.removeAttribute("complete") this.src = null this.src = src + return this.loaded } attributeChangedCallback(name: string) { diff --git a/src/tests/functional/loading_tests.ts b/src/tests/functional/loading_tests.ts index 1704a7aff..54b769fdc 100644 --- a/src/tests/functional/loading_tests.ts +++ b/src/tests/functional/loading_tests.ts @@ -3,6 +3,7 @@ import { assert } from "chai" import { attributeForSelector, hasSelector, + nextAttributeMutationNamed, nextBeat, nextBody, nextEventNamed, @@ -103,18 +104,16 @@ test("test changing src attribute on a frame with loading=eager navigates", asyn }) test("test reloading a frame reloads the content", async ({ page }) => { - await nextBeat() - await page.click("#loading-eager summary") - await nextBeat() + await nextEventOnTarget(page, "frame", "turbo:frame-load") const frameContent = "#loading-eager turbo-frame#frame h2" assert.ok(await hasSelector(page, frameContent)) - assert.ok(await hasSelector(page, "#loading-eager turbo-frame[complete]"), "has [complete] attribute") + assert.equal(await nextAttributeMutationNamed(page, "frame", "complete"), "", "has [complete] attribute") await page.evaluate(() => (document.querySelector("#loading-eager turbo-frame") as any)?.reload()) assert.ok(await hasSelector(page, frameContent)) - assert.ok(await hasSelector(page, "#loading-eager turbo-frame:not([complete])"), "clears [complete] attribute") + assert.equal(await nextAttributeMutationNamed(page, "frame", "complete"), null, "clears [complete] attribute") }) test("test navigating away from a page does not reload its frames", async ({ page }) => { diff --git a/src/tests/functional/navigation_tests.ts b/src/tests/functional/navigation_tests.ts index afaf54221..633210a11 100644 --- a/src/tests/functional/navigation_tests.ts +++ b/src/tests/functional/navigation_tests.ts @@ -11,6 +11,7 @@ import { nextEventNamed, noNextEventNamed, pathname, + readEventLogs, search, selectorHasFocus, visitAction, @@ -21,6 +22,7 @@ import { test.beforeEach(async ({ page }) => { await page.goto("/src/tests/fixtures/navigation.html") + await readEventLogs(page) }) test("test navigating renders a progress bar", async ({ page }) => { @@ -129,8 +131,8 @@ test("test following a same-origin POST form[data-turbo-action=replace]", async }) test("test following a same-origin POST form button[data-turbo-action=replace]", async ({ page }) => { - page.click("#same-origin-replace-form-submitter-post button") - await nextBody(page) + await page.click("#same-origin-replace-form-submitter-post button") + await nextEventNamed(page, "turbo:load") assert.equal(pathname(page.url()), "/src/tests/fixtures/one.html") assert.equal(await visitAction(page), "replace") @@ -339,10 +341,11 @@ test("test correct referrer header", async ({ page }) => { }) test("test double-clicking on a link", async ({ page }) => { - page.click("#delayed-link") - page.click("#delayed-link") + await page.click("#delayed-link", { clickCount: 2 }) + await nextBeat() + + await nextEventNamed(page, "turbo:load") - await nextBody(page, 1200) assert.equal(pathname(page.url()), "/__turbo/delayed_response") assert.equal(await visitAction(page), "advance") }) From 14ae82872a0c0b30e05bcf1e87b75162cd9d7400 Mon Sep 17 00:00:00 2001 From: Simon Taranto Date: Wed, 3 Aug 2022 14:23:20 -0600 Subject: [PATCH 4/4] Add turbo:fetch-request-error event on frame and form network errors (#640) * Add turbo:fetch-error event on frame and form network errors closes https://github.com/hotwired/turbo/issues/462 This PR adds a new event called `turbo:fetch-error` that dispatches when a form or frame fetch request errors. This event will let developers respond appropriately when a fetch request fails due network errors (such as on flaky wifi). At the moment, if a frame navigation fetch request fails due to a network error an error is thrown so https://github.com/hotwired/turbo/blob/2d5cdda4c030658da21965cb20d2885ca7c3e127/src/http/fetch_request.ts#L107 never runs and therefore the `turbo:before-fetch-response` event is not dispatched. * Update src/core/frames/frame_controller.ts Co-authored-by: Keith Cirkel * Update src/core/drive/form_submission.ts Co-authored-by: Keith Cirkel * Add spec * Get browser name right * Fetch project name properly * Use page.context() * remove timeout on offline * Use first page to reduce go to calls * Use tabs again since behavior is different when using turbo-action * use broader event assertion * Update event name and export type * add new event to test helper * Limit recursion in the test helper * skip the details obj instead of limiting recursion Co-authored-by: Keith Cirkel --- src/core/drive/form_submission.ts | 5 +++++ src/core/frames/frame_controller.ts | 6 +++++- src/core/index.ts | 1 + src/core/session.ts | 2 ++ src/tests/fixtures/tabs.html | 2 +- src/tests/fixtures/test.js | 1 + src/tests/functional/frame_navigation_tests.ts | 7 +++++++ 7 files changed, 22 insertions(+), 2 deletions(-) diff --git a/src/core/drive/form_submission.ts b/src/core/drive/form_submission.ts index 6ff0d336e..d1676d2bd 100644 --- a/src/core/drive/form_submission.ts +++ b/src/core/drive/form_submission.ts @@ -3,6 +3,7 @@ import { FetchResponse } from "../../http/fetch_response" import { expandURL } from "../url" import { dispatch, getMetaContent } from "../../util" import { StreamMessage } from "../streams/stream_message" +import { TurboFetchRequestErrorEvent } from "../session" export interface FormSubmissionDelegate { formSubmissionStarted(formSubmission: FormSubmission): void @@ -199,6 +200,10 @@ export class FormSubmission { requestErrored(request: FetchRequest, error: Error) { this.result = { success: false, error } + dispatch("turbo:fetch-request-error", { + target: this.formElement, + detail: { request, error }, + }) this.delegate.formSubmissionErrored(this, error) } diff --git a/src/core/frames/frame_controller.ts b/src/core/frames/frame_controller.ts index d5ddd45fe..b3bb376fb 100644 --- a/src/core/frames/frame_controller.ts +++ b/src/core/frames/frame_controller.ts @@ -29,7 +29,7 @@ import { FrameRenderer } from "./frame_renderer" import { session } from "../index" import { isAction, Action } from "../types" import { VisitOptions } from "../drive/visit" -import { TurboBeforeFrameRenderEvent } from "../session" +import { TurboBeforeFrameRenderEvent, TurboFetchRequestErrorEvent } from "../session" import { StreamMessage } from "../streams/stream_message" export class FrameController @@ -247,6 +247,10 @@ export class FrameController requestErrored(request: FetchRequest, error: Error) { console.error(error) + dispatch("turbo:fetch-request-error", { + target: this.element, + detail: { request, error }, + }) this.resolveVisitPromise() } diff --git a/src/core/index.ts b/src/core/index.ts index 721d2989c..54b033821 100644 --- a/src/core/index.ts +++ b/src/core/index.ts @@ -19,6 +19,7 @@ export { TurboBeforeRenderEvent, TurboBeforeVisitEvent, TurboClickEvent, + TurboFetchRequestErrorEvent, TurboFrameLoadEvent, TurboFrameRenderEvent, TurboLoadEvent, diff --git a/src/core/session.ts b/src/core/session.ts index 992f49deb..a1a36fb1a 100644 --- a/src/core/session.ts +++ b/src/core/session.ts @@ -21,6 +21,7 @@ import { FrameElement } from "../elements/frame_element" import { FrameViewRenderOptions } from "./frames/frame_view" import { FetchResponse } from "../http/fetch_response" import { Preloader, PreloaderDelegate } from "./drive/preloader" +import { FetchRequest } from "../http/fetch_request" export type FormMode = "on" | "off" | "optin" export type TimingData = unknown @@ -30,6 +31,7 @@ export type TurboBeforeVisitEvent = CustomEvent<{ url: string }> export type TurboClickEvent = CustomEvent<{ url: string; originalEvent: MouseEvent }> export type TurboFrameLoadEvent = CustomEvent export type TurboBeforeFrameRenderEvent = CustomEvent<{ newFrame: FrameElement } & FrameViewRenderOptions> +export type TurboFetchRequestErrorEvent = CustomEvent<{ request: FetchRequest; error: Error }> export type TurboFrameRenderEvent = CustomEvent<{ fetchResponse: FetchResponse }> export type TurboLoadEvent = CustomEvent<{ url: string; timing: TimingData }> export type TurboRenderEvent = CustomEvent diff --git a/src/tests/fixtures/tabs.html b/src/tests/fixtures/tabs.html index cc9666c29..702cea0a1 100644 --- a/src/tests/fixtures/tabs.html +++ b/src/tests/fixtures/tabs.html @@ -1,5 +1,5 @@ - + Tabs diff --git a/src/tests/fixtures/test.js b/src/tests/fixtures/test.js index acb543061..b22b09248 100644 --- a/src/tests/fixtures/test.js +++ b/src/tests/fixtures/test.js @@ -48,6 +48,7 @@ "turbo:before-fetch-response", "turbo:visit", "turbo:before-frame-render", + "turbo:fetch-request-error", "turbo:frame-load", "turbo:frame-render", "turbo:reload" diff --git a/src/tests/functional/frame_navigation_tests.ts b/src/tests/functional/frame_navigation_tests.ts index 9dc1ae98c..a38e27b85 100644 --- a/src/tests/functional/frame_navigation_tests.ts +++ b/src/tests/functional/frame_navigation_tests.ts @@ -23,6 +23,13 @@ test("test frame navigation with exterior link", async ({ page }) => { await nextEventOnTarget(page, "frame", "turbo:frame-load") }) +test("test frame navigation emits fetch-request-error event when offline", async ({ page }) => { + await page.goto("/src/tests/fixtures/tabs.html") + await page.context().setOffline(true) + await page.click("#tab-2") + await nextEventNamed(page, "turbo:fetch-request-error") +}) + test("test promoted frame navigation updates the URL before rendering", async ({ page }) => { await page.goto("/src/tests/fixtures/tabs.html")