From 3a30958ce7c47653be36ab0f399cc9835e983edb Mon Sep 17 00:00:00 2001 From: Sean Doyle Date: Mon, 25 Oct 2021 19:22:25 -0400 Subject: [PATCH] Extract `FrameVisit` to drive `FrameController` The problem --- Programmatically driving a `` element when its `[src]` attribute changes is a suitable end-user experience in consumer applications. It's a fitting black-box interface for the outside world: change the value of the attribute and let Turbo handle the rest. However, internally, it's a lossy abstraction. For example, when the `FrameRedirector` class listens for page-wide `click` and `submit` events, it determines if their targets are meant to drive a `` element by: 1. finding an element that matches a clicked `` element's `[data-turbo-frame]` attribute 2. finding an element that matches a submitted `
` element's `[data-turbo-frame]` attribute 3. finding an element that matches a submitted `` element's _submitter's_ `[data-turbo-frame]` attribute 4. finding the closest `` ancestor to the `` or `` Once it finds the matching frame element, it disposes of all that additional context and navigates the `` by updating its `[src]` attribute. This makes it impossible to control various aspects of the frame navigation (like its "rendering" explored in [hotwired/turbo#146][]) outside of its destination URL. Similarly, since a `` and submitter pairing have an impact on which `` is navigated, the `FrameController` implementation passes around a `HTMLFormElement` and `HTMLSubmitter?` data clump and constantly re-fetches a matching `` instance. Outside of frames, page-wide navigation is driven by a `Visit` instance that manages the HTTP life cycle and delegates along the way to a `VisitDelegate`. It also pairs calls to visit with a `VisitOption` object to capture additional context. The proposal --- This commit introduces the `FrameVisit` class. It serves as an encapsulation of the `FetchRequest` and `FormSubmission` lifecycle events involved in navigating a frame. It's implementation draws inspiration from the `Visit`, `VisitDelegate`, and `VisitOptions` pairing. Since the `FrameVisit` knows how to unify both `FetchRequest` and `FormSubmission` hooks, the resulting callbacks fired from within the `FrameController` are flat and consistent. Extra benefits --- The biggest benefit is the introduction of a DRY abstraction to manage the behind the scenes HTTP calls necessary to drive a ``. With the introduction of the `FrameVisit` concept, we can also declare a `visit()` and `submit()` method for `FrameElementDelegate` implementations in the place of other implementation-specific methods like `loadResponse()` and `formSubmissionIntercepted()`. In addition, these changes have the potential to close [hotwired/turbo#326][], since we can consistently invoke `loadResponse()` across ``-click-initiated and ``-submission-initiated visits. To ensure that's the case, this commit adds test coverage for navigating a `` by making a `GET` request to an endpoint that responds with a `500` status. [hotwired/turbo#146]: https://github.com/hotwired/turbo/pull/146 [hotwired/turbo#326]: https://github.com/hotwired/turbo/issues/326 --- src/core/frames/frame_controller.ts | 203 ++++++++-------------------- src/core/frames/frame_redirector.ts | 8 +- src/core/frames/frame_visit.ts | 181 +++++++++++++++++++++++++ src/core/session.ts | 3 +- src/elements/frame_element.ts | 12 +- 5 files changed, 247 insertions(+), 160 deletions(-) create mode 100644 src/core/frames/frame_visit.ts diff --git a/src/core/frames/frame_controller.ts b/src/core/frames/frame_controller.ts index 86fa98aa8..79bd3f7de 100644 --- a/src/core/frames/frame_controller.ts +++ b/src/core/frames/frame_controller.ts @@ -4,20 +4,17 @@ import { FrameLoadingStyle, FrameElementObservedAttribute, } from "../../elements/frame_element" -import { FetchMethod, FetchRequest, FetchRequestDelegate, FetchRequestHeaders } from "../../http/fetch_request" +import { FetchRequest, TurboFetchRequestErrorEvent } from "../../http/fetch_request" import { FetchResponse } from "../../http/fetch_response" import { AppearanceObserver, AppearanceObserverDelegate } from "../../observers/appearance_observer" import { - clearBusyState, dispatch, getAttribute, parseHTMLDocument, - markAsBusy, uuid, getHistoryMethodForAction, getVisitAction, } from "../../util" -import { FormSubmission, FormSubmissionDelegate } from "../drive/form_submission" import { Snapshot } from "../snapshot" import { ViewDelegate, ViewRenderOptions } from "../view" import { Locatable, getAction, expandURL, urlsAreEqual, locationIsVisitable } from "../url" @@ -27,11 +24,10 @@ import { LinkInterceptor, LinkInterceptorDelegate } from "./link_interceptor" import { FormLinkClickObserver, FormLinkClickObserverDelegate } from "../../observers/form_link_click_observer" import { FrameRenderer } from "./frame_renderer" import { session } from "../index" -import { isAction, Action } from "../types" +import { Action } from "../types" +import { FrameVisit, FrameVisitDelegate, FrameVisitOptions } from "./frame_visit" import { VisitOptions } from "../drive/visit" import { TurboBeforeFrameRenderEvent } from "../session" -import { StreamMessage } from "../streams/stream_message" -import { PageSnapshot } from "../drive/page_snapshot" type VisitFallback = (location: Response | Locatable, options: Partial) => Promise export type TurboFrameMissingEvent = CustomEvent<{ response: Response; visit: VisitFallback }> @@ -39,11 +35,10 @@ export type TurboFrameMissingEvent = CustomEvent<{ response: Response; visit: Vi export class FrameController implements AppearanceObserverDelegate, - FetchRequestDelegate, FormSubmitObserverDelegate, - FormSubmissionDelegate, FrameElementDelegate, FormLinkClickObserverDelegate, + FrameVisitDelegate, LinkInterceptorDelegate, ViewDelegate> { @@ -53,18 +48,12 @@ export class FrameController readonly formLinkClickObserver: FormLinkClickObserver readonly linkInterceptor: LinkInterceptor readonly formSubmitObserver: FormSubmitObserver - formSubmission?: FormSubmission - fetchResponseLoaded = (_fetchResponse: FetchResponse) => {} - private currentFetchRequest: FetchRequest | null = null - private resolveVisitPromise = () => {} + frameVisit?: FrameVisit private connected = false private hasBeenLoaded = false private ignoredAttributes: Set = new Set() - private action: Action | null = null readonly restorationIdentifier: string private previousFrameElement?: FrameElement - private currentNavigationElement?: Element - pageSnapshot?: PageSnapshot constructor(element: FrameElement) { this.element = element @@ -100,6 +89,12 @@ export class FrameController } } + visit(options: FrameVisitOptions): Promise { + const action = getVisitAction(this.element) + const frameVisit = new FrameVisit(this, this.element, { action, ...options }) + return frameVisit.start() + } + disabledChanged() { if (this.loadingStyle == FrameLoadingStyle.eager) { this.loadSourceURL() @@ -145,14 +140,11 @@ export class FrameController private async loadSourceURL() { if (this.enabled && this.isActive && !this.complete && this.sourceURL) { - this.element.loaded = this.visit(expandURL(this.sourceURL)) - this.appearanceObserver.stop() - await this.element.loaded - this.hasBeenLoaded = true + await this.visit({ url: this.sourceURL }) } } - async loadResponse(fetchResponse: FetchResponse) { + async loadResponse(fetchResponse: FetchResponse, frameVisit: FrameVisit) { if (fetchResponse.redirected || (fetchResponse.succeeded && fetchResponse.isHTML)) { this.sourceURL = fetchResponse.response.url } @@ -174,13 +166,13 @@ export class FrameController false ) if (this.view.renderPromise) await this.view.renderPromise - this.changeHistory() + this.changeHistory(frameVisit.action) await this.view.render(renderer) this.complete = true session.frameRendered(fetchResponse, this.element) session.frameLoaded(this.element) - this.fetchResponseLoaded(fetchResponse) + this.proposeVisitIfNavigatedWithAction(frameVisit, fetchResponse) } else if (this.willHandleFrameMissingFromResponse(fetchResponse)) { console.warn( `A matching frame for #${this.element.id} was missing from the response, transforming into full-page Visit.` @@ -191,16 +183,12 @@ export class FrameController } catch (error) { console.error(error) this.view.invalidate() - } finally { - this.fetchResponseLoaded = () => {} } } // Appearance observer delegate - elementAppearedInViewport(element: FrameElement) { - this.pageSnapshot = PageSnapshot.fromElement(element).clone() - this.proposeVisitIfNavigatedWithAction(element, element) + elementAppearedInViewport(_element: FrameElement) { this.loadSourceURL() } @@ -232,78 +220,39 @@ export class FrameController } formSubmitted(element: HTMLFormElement, submitter?: HTMLElement) { - if (this.formSubmission) { - this.formSubmission.stop() - } - - this.formSubmission = new FormSubmission(this, element, submitter) - const { fetchRequest } = this.formSubmission - this.prepareHeadersForRequest(fetchRequest.headers, fetchRequest) - this.formSubmission.start() + const frame = this.findFrameElement(element, submitter) + frame.delegate.visit(FrameVisit.optionsForSubmit(element, submitter)) } - // Fetch request delegate + // Frame visit delegate - prepareHeadersForRequest(headers: FetchRequestHeaders, request: FetchRequest) { - headers["Turbo-Frame"] = this.id - - if (this.currentNavigationElement?.hasAttribute("data-turbo-stream")) { - request.acceptResponseType(StreamMessage.contentType) - } - } - - requestStarted(_request: FetchRequest) { - markAsBusy(this.element) + shouldVisit(_frameVisit: FrameVisit) { + return this.enabled && this.isActive } - requestPreventedHandlingResponse(_request: FetchRequest, _response: FetchResponse) { - this.resolveVisitPromise() + visitStarted(frameVisit: FrameVisit) { + this.frameVisit?.stop() + this.frameVisit = frameVisit } - async requestSucceededWithResponse(request: FetchRequest, response: FetchResponse) { - await this.loadResponse(response) - this.resolveVisitPromise() + async visitSucceededWithResponse(frameVisit: FrameVisit, response: FetchResponse) { + await this.loadResponse(response, frameVisit) } - async requestFailedWithResponse(request: FetchRequest, response: FetchResponse) { - console.error(response) - await this.loadResponse(response) - this.resolveVisitPromise() + async visitFailedWithResponse(frameVisit: FrameVisit, response: FetchResponse) { + await this.loadResponse(response, frameVisit) } - requestErrored(request: FetchRequest, error: Error) { - console.error(error) - this.resolveVisitPromise() - } - - requestFinished(_request: FetchRequest) { - clearBusyState(this.element) - } - - // Form submission delegate - - formSubmissionStarted({ formElement }: FormSubmission) { - markAsBusy(formElement, this.findFrameElement(formElement)) - } - - formSubmissionSucceededWithResponse(formSubmission: FormSubmission, response: FetchResponse) { - const frame = this.findFrameElement(formSubmission.formElement, formSubmission.submitter) - - frame.delegate.proposeVisitIfNavigatedWithAction(frame, formSubmission.formElement, formSubmission.submitter) - - frame.delegate.loadResponse(response) - } - - formSubmissionFailedWithResponse(formSubmission: FormSubmission, fetchResponse: FetchResponse) { - this.element.delegate.loadResponse(fetchResponse) - } - - formSubmissionErrored(formSubmission: FormSubmission, error: Error) { + visitErrored(frameVisit: FrameVisit, request: FetchRequest, error: Error) { console.error(error) + dispatch("turbo:fetch-request-error", { + target: this.element, + detail: { request, error }, + }) } - formSubmissionFinished({ formElement }: FormSubmission) { - clearBusyState(formElement, this.findFrameElement(formElement)) + visitCompleted(_frameVisit: FrameVisit) { + this.hasBeenLoaded = true } // View delegate @@ -351,64 +300,32 @@ export class FrameController // Private - private async visit(url: URL) { - const request = new FetchRequest(this, FetchMethod.get, url, new URLSearchParams(), this.element) - - this.currentFetchRequest?.cancel() - this.currentFetchRequest = request - - return new Promise((resolve) => { - this.resolveVisitPromise = () => { - this.resolveVisitPromise = () => {} - this.currentFetchRequest = null - resolve() + private navigateFrame(element: Element, url: string) { + const frame = this.findFrameElement(element) + frame.delegate.visit(FrameVisit.optionsForClick(element, expandURL(url))) + } + + private proposeVisitIfNavigatedWithAction({ action, element, snapshot }: FrameVisit, fetchResponse: FetchResponse) { + if (element.src && action) { + const { statusCode, redirected } = fetchResponse + const responseHTML = element.ownerDocument.documentElement.outerHTML + const options: Partial = { + action, + snapshot, + response: { statusCode, redirected, responseHTML }, + restorationIdentifier: this.restorationIdentifier, + updateHistory: false, + visitCachedSnapshot: this.visitCachedSnapshot, + willRender: false, } - request.perform() - }) - } - - private navigateFrame(element: Element, url: string, submitter?: HTMLElement) { - const frame = this.findFrameElement(element, submitter) - this.pageSnapshot = PageSnapshot.fromElement(frame).clone() - - frame.delegate.proposeVisitIfNavigatedWithAction(frame, element, submitter) - - this.withCurrentNavigationElement(element, () => { - frame.src = url - }) - } - - proposeVisitIfNavigatedWithAction(frame: FrameElement, element: Element, submitter?: HTMLElement) { - this.action = getVisitAction(submitter, element, frame) - - if (isAction(this.action)) { - const { visitCachedSnapshot } = frame.delegate - - frame.delegate.fetchResponseLoaded = (fetchResponse: FetchResponse) => { - if (frame.src) { - const { statusCode, redirected } = fetchResponse - const responseHTML = frame.ownerDocument.documentElement.outerHTML - const response = { statusCode, redirected, responseHTML } - const options: Partial = { - response, - visitCachedSnapshot, - willRender: false, - updateHistory: false, - restorationIdentifier: this.restorationIdentifier, - snapshot: this.pageSnapshot, - } - - if (this.action) options.action = this.action - session.visit(frame.src, options) - } - } + session.visit(element.src, options) } } - changeHistory() { - if (this.action) { - const method = getHistoryMethodForAction(this.action) + changeHistory(action: Action | null) { + if (action) { + const method = getHistoryMethodForAction(action) session.history.update(method, expandURL(this.element.src || ""), this.restorationIdentifier) } } @@ -532,7 +449,7 @@ export class FrameController } get isLoading() { - return this.formSubmission !== undefined || this.resolveVisitPromise() !== undefined + return this.frameVisit !== undefined } get complete() { @@ -568,12 +485,6 @@ 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/frames/frame_redirector.ts b/src/core/frames/frame_redirector.ts index 3175e9fe0..c4840d79f 100644 --- a/src/core/frames/frame_redirector.ts +++ b/src/core/frames/frame_redirector.ts @@ -3,6 +3,8 @@ import { FrameElement } from "../../elements/frame_element" import { LinkInterceptor, LinkInterceptorDelegate } from "./link_interceptor" import { expandURL, getAction, locationIsVisitable } from "../url" import { Session } from "../session" +import { FrameVisit } from "./frame_visit" + export class FrameRedirector implements LinkInterceptorDelegate, FormSubmitObserverDelegate { readonly session: Session readonly element: Element @@ -30,10 +32,10 @@ export class FrameRedirector implements LinkInterceptorDelegate, FormSubmitObser return this.shouldRedirect(element) } - linkClickIntercepted(element: Element, url: string, event: MouseEvent) { + linkClickIntercepted(element: Element, url: string, _event: MouseEvent) { const frame = this.findFrameElement(element) if (frame) { - frame.delegate.linkClickIntercepted(element, url, event) + frame.delegate.visit(FrameVisit.optionsForClick(element, expandURL(url))) } } @@ -48,7 +50,7 @@ export class FrameRedirector implements LinkInterceptorDelegate, FormSubmitObser formSubmitted(element: HTMLFormElement, submitter?: HTMLElement) { const frame = this.findFrameElement(element, submitter) if (frame) { - frame.delegate.formSubmitted(element, submitter) + frame.delegate.visit(FrameVisit.optionsForSubmit(element, submitter)) } } diff --git a/src/core/frames/frame_visit.ts b/src/core/frames/frame_visit.ts new file mode 100644 index 000000000..30bbd3aec --- /dev/null +++ b/src/core/frames/frame_visit.ts @@ -0,0 +1,181 @@ +import { Locatable, expandURL } from "../url" +import { Action } from "../types" +import { clearBusyState, getVisitAction, markAsBusy } from "../../util" +import { FrameElement } from "../../elements/frame_element" +import { FetchRequest, FetchRequestDelegate, FetchRequestHeaders, FetchMethod } from "../../http/fetch_request" +import { FetchResponse } from "../../http/fetch_response" +import { FormSubmission, FormSubmissionDelegate } from "../drive/form_submission" +import { PageSnapshot } from "../drive/page_snapshot" +import { StreamMessage } from "../streams/stream_message" + +type Options = { + action: Action | null + acceptsStreamResponse: boolean + submit: { form: HTMLFormElement; submitter?: HTMLElement } + url: Locatable +} +type ClickFrameVisitOptions = Partial & { url: Options["url"] } +type SubmitFrameVisitOptions = Partial & { submit: Options["submit"] } + +export type FrameVisitOptions = ClickFrameVisitOptions | SubmitFrameVisitOptions + +export interface FrameVisitDelegate { + shouldVisit(frameVisit: FrameVisit): boolean + visitStarted(frameVisit: FrameVisit): void + visitSucceededWithResponse(frameVisit: FrameVisit, response: FetchResponse): void + visitFailedWithResponse(frameVisit: FrameVisit, response: FetchResponse): void + visitErrored(frameVisit: FrameVisit, request: FetchRequest, error: Error): void + visitCompleted(frameVisit: FrameVisit): void +} + +export class FrameVisit implements FetchRequestDelegate, FormSubmissionDelegate { + readonly delegate: FrameVisitDelegate + readonly element: FrameElement + readonly action: Action | null + readonly previousURL: string | null + readonly options: FrameVisitOptions + readonly isFormSubmission: boolean = false + readonly acceptsStreamResponse: boolean + snapshot?: PageSnapshot + + private readonly fetchRequest?: FetchRequest + private readonly formSubmission?: FormSubmission + private resolveVisitPromise = () => {} + + static optionsForClick(element: Element, url: URL): ClickFrameVisitOptions { + const action = getVisitAction(element) + const acceptsStreamResponse = element.hasAttribute("data-turbo-stream") + + return { acceptsStreamResponse, action, url } + } + + static optionsForSubmit(form: HTMLFormElement, submitter?: HTMLElement): SubmitFrameVisitOptions { + const action = getVisitAction(form, submitter) + + return { action, submit: { form, submitter } } + } + + constructor(delegate: FrameVisitDelegate, element: FrameElement, options: FrameVisitOptions) { + this.delegate = delegate + this.element = element + this.previousURL = this.element.src + + const { acceptsStreamResponse, action, url, submit } = (this.options = options) + + this.acceptsStreamResponse = acceptsStreamResponse || false + this.action = action || getVisitAction(this.element) + + if (url) { + this.fetchRequest = new FetchRequest(this, FetchMethod.get, expandURL(url), new URLSearchParams(), this.element) + } else if (submit) { + const { fetchRequest } = (this.formSubmission = new FormSubmission(this, submit.form, submit.submitter)) + this.prepareHeadersForRequest(fetchRequest.headers, fetchRequest) + this.isFormSubmission = true + } + } + + async start(): Promise { + if (this.delegate.shouldVisit(this)) { + this.snapshot = PageSnapshot.fromElement(this.element).clone() + + this.element.removeAttribute("complete") + + if (this.formSubmission) { + await this.formSubmission.start() + } else { + await this.performRequest() + } + + return this.element.loaded + } else { + return Promise.resolve() + } + } + + stop() { + this.fetchRequest?.cancel() + this.formSubmission?.stop() + } + + // Fetch request delegate + + prepareHeadersForRequest(headers: FetchRequestHeaders, request: FetchRequest) { + request.headers["Turbo-Frame"] = this.element.id + + if (this.acceptsStreamResponse || this.isFormSubmission) { + request.acceptResponseType(StreamMessage.contentType) + } + } + + requestStarted(request: FetchRequest) { + if (request.target instanceof HTMLFormElement) { + markAsBusy(request.target) + } + + markAsBusy(this.element) + + this.delegate.visitStarted(this) + } + + requestPreventedHandlingResponse(_request: FetchRequest, _response: FetchResponse) { + this.resolveVisitPromise() + } + + requestFinished(request: FetchRequest) { + clearBusyState(this.element) + + if (request.target instanceof HTMLFormElement) { + clearBusyState(request.target) + } + + this.delegate.visitCompleted(this) + } + + async requestSucceededWithResponse(fetchRequest: FetchRequest, fetchResponse: FetchResponse) { + await this.delegate.visitSucceededWithResponse(this, fetchResponse) + this.resolveVisitPromise() + } + + async requestFailedWithResponse(request: FetchRequest, fetchResponse: FetchResponse) { + console.error(fetchResponse) + await this.delegate.visitFailedWithResponse(this, fetchResponse) + this.resolveVisitPromise() + } + + requestErrored(request: FetchRequest, error: Error) { + this.delegate.visitErrored(this, request, error) + this.resolveVisitPromise() + } + + // Form submission delegate + + formSubmissionStarted({ fetchRequest }: FormSubmission) { + this.requestStarted(fetchRequest) + } + + async formSubmissionSucceededWithResponse({ fetchRequest }: FormSubmission, response: FetchResponse) { + await this.requestSucceededWithResponse(fetchRequest, response) + } + + async formSubmissionFailedWithResponse({ fetchRequest }: FormSubmission, fetchResponse: FetchResponse) { + await this.requestFailedWithResponse(fetchRequest, fetchResponse) + } + + formSubmissionErrored({ fetchRequest }: FormSubmission, error: Error) { + this.requestErrored(fetchRequest, error) + } + + formSubmissionFinished({ fetchRequest }: FormSubmission) { + this.requestFinished(fetchRequest) + } + + private performRequest() { + this.element.loaded = new Promise((resolve) => { + this.resolveVisitPromise = () => { + this.resolveVisitPromise = () => {} + resolve() + } + this.fetchRequest?.perform() + }) + } +} diff --git a/src/core/session.ts b/src/core/session.ts index 2d1cb3685..06897b277 100644 --- a/src/core/session.ts +++ b/src/core/session.ts @@ -113,8 +113,7 @@ export class Session const frameElement = options.frame ? document.getElementById(options.frame) : null if (frameElement instanceof FrameElement) { - frameElement.src = location.toString() - frameElement.loaded + frameElement.delegate.visit({ url: location.toString() }) } else { this.navigator.proposeVisit(expandURL(location), options) } diff --git a/src/elements/frame_element.ts b/src/elements/frame_element.ts index 8a177a6d7..320a51c30 100644 --- a/src/elements/frame_element.ts +++ b/src/elements/frame_element.ts @@ -1,7 +1,4 @@ -import { FetchResponse } from "../http/fetch_response" -import { Snapshot } from "../core/snapshot" -import { LinkInterceptorDelegate } from "../core/frames/link_interceptor" -import { FormSubmitObserverDelegate } from "../observers/form_submit_observer" +import { FrameVisitOptions } from "../core/frames/frame_visit" export enum FrameLoadingStyle { eager = "eager", @@ -10,18 +7,15 @@ export enum FrameLoadingStyle { export type FrameElementObservedAttribute = keyof FrameElement & ("disabled" | "complete" | "loading" | "src") -export interface FrameElementDelegate extends LinkInterceptorDelegate, FormSubmitObserverDelegate { +export interface FrameElementDelegate { connect(): void disconnect(): void + visit(options: Partial): Promise completeChanged(): void loadingStyleChanged(): void sourceURLChanged(): void sourceURLReloaded(): Promise disabledChanged(): void - loadResponse(response: FetchResponse): void - proposeVisitIfNavigatedWithAction(frame: FrameElement, element: Element, submitter?: HTMLElement): void - fetchResponseLoaded: (fetchResponse: FetchResponse) => void - visitCachedSnapshot: (snapshot: Snapshot) => void isLoading: boolean }