GET Submissions: Do not merge into [action]
#464
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Follow-up to [hotwired#461][] While the changes made in hotwired#461 added coverage for `<form method="post" action="...">` and `<form>` submissions, it didn't cover `<form action="/?encoded=value">` submissions with search parameters encoded into the `[action]` attribute. Without Turbo or any other JavaScript, submitting a form with an `[action]` value that contains query parameters requests that `[action]` path without any key-value pairs (that is, with `?` as its the value of `URL.search`): ```html <form action="/src/tests/fixtures/form.html?sort=asc"> <!-- clicking the button navigates to `/src/tests/fixtures/form?` --> <button>Submit</button> </form> ``` This commit modifies the `FetchRequest` to always invoke the module-private `mergeFormDataEntries()` function when constructing `GET` requests, and modifies the `mergeFormDataEntries()` to completely discard the `[action]` URL's search parameters before merging-in the `FormData` key-value pairs. [hotwired#461]: hotwired#461
seanpdoyle
added a commit
to seanpdoyle/turbo
that referenced
this pull request
Nov 23, 2021
Closes [hotwired#456][] The changes made in [hotwired#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 `<a>` elements with `[href]` attributes that declare search parameters directly (that *do not* need to be "merged" into anything). The [change][] made in [hotwired#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`. In addition to that change, this commit removes the `FetchRequestBody` union type that combined `URLSearchParams` and `FormData`. In its place, change the `FetchRequest` to only accept `FormData` instances, since any `URLSearchParams` management will occur outside that class. [hotwired#456]: hotwired#465 [hotwired#464]: hotwired#464 [change]: hotwired@6906e57#diff-68b647dc2963716dc27c070f261d0b942ee9c00be7c4149ecb3a5acd94842d40R119 [searchParams]: https://developer.mozilla.org/en-US/docs/Web/API/URL/searchParams
seanpdoyle
added a commit
to seanpdoyle/turbo
that referenced
this pull request
Nov 23, 2021
Closes [hotwired#456][] The changes made in [hotwired#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 `<a>` elements with `[href]` attributes that declare search parameters directly (that *do not* need to be "merged" into anything). The [change][] made in [hotwired#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`. In addition to that change, this commit removes the `FetchRequestBody` union type that combined `URLSearchParams` and `FormData`. In its place, change the `FetchRequest` to only accept `FormData` instances, since any `URLSearchParams` management will occur outside that class. [hotwired#456]: hotwired#465 [hotwired#464]: hotwired#464 [change]: hotwired@6906e57#diff-68b647dc2963716dc27c070f261d0b942ee9c00be7c4149ecb3a5acd94842d40R119 [searchParams]: https://developer.mozilla.org/en-US/docs/Web/API/URL/searchParams
seanpdoyle
added a commit
to seanpdoyle/turbo
that referenced
this pull request
Nov 23, 2021
Closes [hotwired#456][] The changes made in [hotwired#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 `<a>` elements with `[href]` attributes that declare search parameters directly (that *do not* need to be "merged" into anything). The [change][] made in [hotwired#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#456]: hotwired#465 [hotwired#464]: hotwired#464 [change]: hotwired@6906e57#diff-68b647dc2963716dc27c070f261d0b942ee9c00be7c4149ecb3a5acd94842d40R119 [searchParams]: https://developer.mozilla.org/en-US/docs/Web/API/URL/searchParams
dhh
pushed a commit
that referenced
this pull request
Nov 23, 2021
* Restore searchParams to Visit and Frame Navigation Closes [#456][] The changes made in [#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 `<a>` elements with `[href]` attributes that declare search parameters directly (that *do not* need to be "merged" into anything). The [change][] made in [#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`. [#456]: #465 [#464]: #464 [change]: 6906e57#diff-68b647dc2963716dc27c070f261d0b942ee9c00be7c4149ecb3a5acd94842d40R119 [searchParams]: https://developer.mozilla.org/en-US/docs/Web/API/URL/searchParams * Attempt to fix flaky Scroll Restoration test There is a common test failure in CI: ``` × firefox on linux 5.11.0-1020-azure - ScrollRestorationTests - landing on an anchor (0.02s) 444 TypeError: Cannot read property 'slice' of null 445 at RemoteChannel.read @ src/tests/helpers/remote_channel.ts:14:44 ``` This commit addresses it in two ways. First, ensures that the `scroll_restoration.html` page loads the `src/tests/fixtures/test.js` script to wire-up the remote channels (like `window.eventLogs` and `window.mutationLogs`). Next, it modifies the test itself to await the `this.nextBody` hook, to ensure that the page load completes before checking the scroll.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow-up to hotwired/turbo#461
While the changes made in #461 added coverage for
<form method="post" action="...">and<form>submissions, it didn't cover<form action="/?encoded=value">submissions with search parameters encodedinto the
[action]attribute.Without Turbo or any other JavaScript, submitting a form with an
[action]value that contains query parameters requests that[action]path without any key-value pairs (that is, with
?as its the value ofURL.search):This commit modifies the
FetchRequestto always invoke themodule-private
mergeFormDataEntries()function when constructingGETrequests, and modifies the
mergeFormDataEntries()to completelydiscard the
[action]URL's search parameters before merging-in theFormDatakey-value pairs.