From 5e68bd8afbadfec6adb56ce3d12600a5e6efebd4 Mon Sep 17 00:00:00 2001 From: Sean Doyle Date: Mon, 22 Nov 2021 23:06:02 -0500 Subject: [PATCH] Restore searchParams to Visit and Frame Navigation Closes [hotwired/turbo#456][] The changes made in [hotwired/turbo#464][] resolved an issue in how `GET` requests merged query parameters into their `URL` instance. Unfortunately, the changes were too loosely scoped, and they affected _all_ `GET` requests, including those initiated by `` elements with `[href]` attributes that declare search parameters directly (that *do not* need to be "merged" into anything). The [change][] made in [hotwired/turbo#464][] to conditionally include the `FetchRequest.body` based on the request's verb enables the changes made in this commit. In response to that change, this commit moves the `mergeFormDataEntries()` function out of the `FetchMethod` module and into the `FormSubmission` class. Since merging _from_ `FormData` instances _into_ a `URL` is a concern of `GET`-powered Form Submissions, moving the function out of the `FetchRequest` further simplifies that class. Move the `mergeFormDataEntries()` to the `FormSubmission` constructor to re-write its `location` property's [searchParams][] prior to constructing the instance's `FetchRequest`. [hotwired/turbo#456]: https://github.com/hotwired/turbo/issues/465 [hotwired/turbo#464]: https://github.com/hotwired/turbo/pull/464 [change]: https://github.com/hotwired/turbo/pull/461/commits/6906e57769a79fcf39955c4c0d1037434ea0c579#diff-68b647dc2963716dc27c070f261d0b942ee9c00be7c4149ecb3a5acd94842d40R119 [searchParams]: https://developer.mozilla.org/en-US/docs/Web/API/URL/searchParams --- src/core/drive/form_submission.ts | 23 +++++++++++++++++++---- src/core/frames/frame_controller.ts | 2 +- src/http/fetch_request.ts | 18 +----------------- src/tests/fixtures/frames.html | 3 ++- src/tests/fixtures/navigation.html | 1 + src/tests/fixtures/visit.html | 1 + src/tests/functional/frame_tests.ts | 18 ++++++++++++++++++ src/tests/functional/navigation_tests.ts | 9 +++++++++ src/tests/functional/visit_tests.ts | 7 +++++++ 9 files changed, 59 insertions(+), 23 deletions(-) diff --git a/src/core/drive/form_submission.ts b/src/core/drive/form_submission.ts index 39660e34e..e7f884644 100644 --- a/src/core/drive/form_submission.ts +++ b/src/core/drive/form_submission.ts @@ -44,6 +44,7 @@ export class FormSubmission { readonly formElement: HTMLFormElement readonly submitter?: HTMLElement readonly formData: FormData + readonly location: URL readonly fetchRequest: FetchRequest readonly mustRedirect: boolean state = FormSubmissionState.initialized @@ -58,6 +59,10 @@ export class FormSubmission { this.formElement = formElement this.submitter = submitter this.formData = buildFormData(formElement, submitter) + this.location = expandURL(this.action) + if (this.method == FetchMethod.get) { + mergeFormDataEntries(this.location, [ ...this.body.entries() ]) + } this.fetchRequest = new FetchRequest(this, this.method, this.location, this.body, this.formElement) this.mustRedirect = mustRedirect } @@ -72,10 +77,6 @@ export class FormSubmission { return this.submitter?.getAttribute("formaction") || this.formElement.getAttribute("action") || formElementAction || "" } - get location(): URL { - return expandURL(this.action) - } - get body() { if (this.enctype == FormEnctype.urlEncoded || this.method == FetchMethod.get) { return new URLSearchParams(this.stringFormData) @@ -220,3 +221,17 @@ function getMetaContent(name: string) { function responseSucceededWithoutRedirect(response: FetchResponse) { return response.statusCode == 200 && !response.redirected } + +function mergeFormDataEntries(url: URL, entries: [string, FormDataEntryValue][]): URL { + const searchParams = new URLSearchParams + + for (const [ name, value ] of entries) { + if (value instanceof File) continue + + searchParams.append(name, value) + } + + url.search = searchParams.toString() + + return url +} diff --git a/src/core/frames/frame_controller.ts b/src/core/frames/frame_controller.ts index d53ae75b2..ce98a5fae 100644 --- a/src/core/frames/frame_controller.ts +++ b/src/core/frames/frame_controller.ts @@ -236,7 +236,7 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest // Private private async visit(url: URL) { - const request = new FetchRequest(this, FetchMethod.get, url, url.searchParams, this.element) + const request = new FetchRequest(this, FetchMethod.get, url, new URLSearchParams, this.element) this.currentFetchRequest?.cancel() this.currentFetchRequest = request diff --git a/src/http/fetch_request.ts b/src/http/fetch_request.ts index 6c2627dfd..ea1fbdfef 100644 --- a/src/http/fetch_request.ts +++ b/src/http/fetch_request.ts @@ -57,9 +57,7 @@ export class FetchRequest { this.method = method this.headers = this.defaultHeaders this.body = body - this.url = this.isIdempotent ? - mergeFormDataEntries(new URL(location.href), this.entries) : - location + this.url = location this.target = target } @@ -150,17 +148,3 @@ export class FetchRequest { if (event.defaultPrevented) await requestInterception } } - -function mergeFormDataEntries(url: URL, entries: [string, FormDataEntryValue][]): URL { - const searchParams = new URLSearchParams - - for (const [ name, value ] of entries) { - if (value instanceof File) continue - - searchParams.append(name, value) - } - - url.search = searchParams.toString() - - return url -} diff --git a/src/tests/fixtures/frames.html b/src/tests/fixtures/frames.html index e475268eb..2a84ca2a4 100644 --- a/src/tests/fixtures/frames.html +++ b/src/tests/fixtures/frames.html @@ -29,6 +29,7 @@

Frames: #frame

Navigate #frame from within + Navigate #frame with ?key=value Navigate #frame from within with a[data-turbo-action="advance"] Navigate #frame to /frames/form.html @@ -84,7 +85,7 @@

Frames: #nested-child

- Visit one.html + Visit one.html?key=value Visit self diff --git a/src/tests/fixtures/navigation.html b/src/tests/fixtures/navigation.html index 2b97cb37c..e5d977aaf 100644 --- a/src/tests/fixtures/navigation.html +++ b/src/tests/fixtures/navigation.html @@ -33,6 +33,7 @@

Navigation

Same-origin unannotated link

+

Same-origin unannotated link ?key=value

Same-origin data-turbo-action=replace link

diff --git a/src/tests/fixtures/visit.html b/src/tests/fixtures/visit.html index ae09a8ec7..08e0831ba 100644 --- a/src/tests/fixtures/visit.html +++ b/src/tests/fixtures/visit.html @@ -10,6 +10,7 @@

Visit

Same-origin link

+

Same-origin link with ?key=value

Sample response

diff --git a/src/tests/functional/frame_tests.ts b/src/tests/functional/frame_tests.ts index 3316287d1..b2550e832 100644 --- a/src/tests/functional/frame_tests.ts +++ b/src/tests/functional/frame_tests.ts @@ -27,6 +27,22 @@ export class FrameTests extends TurboDriveTestCase { this.assert.equal(await frame.getAttribute("src"), await this.propertyForSelector("#hello a", "href")) } + async "test following a link sets the frame element's [src]"() { + await this.clickSelector("#link-frame-with-search-params") + + const { url } = await this.nextEventOnTarget("frame", "turbo:before-fetch-request") + const fetchRequestUrl = new URL(url) + + this.assert.equal(fetchRequestUrl.pathname, "/src/tests/fixtures/frames/frame.html") + this.assert.equal(fetchRequestUrl.searchParams.get("key"), "value", "fetch request encodes query parameters") + + await this.nextBeat + const src = new URL(await this.attributeForSelector("#frame", "src") || "") + + this.assert.equal(src.pathname, "/src/tests/fixtures/frames/frame.html") + this.assert.equal(src.searchParams.get("key"), "value", "[src] attribute encodes query parameters") + } + async "test a frame whose src references itself does not infinitely loop"() { await this.clickSelector("#frame-self") @@ -129,6 +145,8 @@ export class FrameTests extends TurboDriveTestCase { const frameText = await this.querySelector("body > h1") this.assert.equal(await frameText.getVisibleText(), "One") this.assert.notOk(await this.hasSelector("#navigate-top a")) + this.assert.equal(await this.pathname, "/src/tests/fixtures/one.html") + this.assert.equal(await this.getSearchParam("key"), "value") } async "test following a link that declares data-turbo-frame='_self' within a frame with target=_top navigates the frame itself"() { diff --git a/src/tests/functional/navigation_tests.ts b/src/tests/functional/navigation_tests.ts index 5e349f95d..8538f59e2 100644 --- a/src/tests/functional/navigation_tests.ts +++ b/src/tests/functional/navigation_tests.ts @@ -48,6 +48,15 @@ export class NavigationTests extends TurboDriveTestCase { }) await this.nextBody this.assert.equal(await this.pathname, "/src/tests/fixtures/one.html") + this.assert.equal(await this.search, "") + this.assert.equal(await this.visitAction, "advance") + } + + async "test following a same-origin unannotated link with search params"() { + this.clickSelector("#same-origin-unannotated-link-search-params") + await this.nextBody + this.assert.equal(await this.pathname, "/src/tests/fixtures/one.html") + this.assert.equal(await this.search, "?key=value") this.assert.equal(await this.visitAction, "advance") } diff --git a/src/tests/functional/visit_tests.ts b/src/tests/functional/visit_tests.ts index 563ba2625..c80cccb75 100644 --- a/src/tests/functional/visit_tests.ts +++ b/src/tests/functional/visit_tests.ts @@ -79,6 +79,13 @@ export class VisitTests extends TurboDriveTestCase { this.assert.ok(url.toString().includes("/src/tests/fixtures/one.html")) } + async "test turbo:before-fetch-request event.detail encodes searchParams"() { + await this.clickSelector("#same-origin-link-search-params") + const { url } = await this.nextEventNamed("turbo:before-fetch-request") + + this.assert.ok(url.includes("/src/tests/fixtures/one.html?key=value")) + } + async "test turbo:before-fetch-response open new site"() { this.remote.execute(() => addEventListener("turbo:before-fetch-response", async function eventListener(event: any) { removeEventListener("turbo:before-fetch-response", eventListener, false);