Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

GET Forms: fire submit-start and submit-end #424

Merged
merged 1 commit into from
Oct 18, 2021
Merged

GET Forms: fire submit-start and submit-end #424

merged 1 commit into from
Oct 18, 2021

Conversation

seanpdoyle
Copy link
Contributor

@seanpdoyle seanpdoyle commented Oct 14, 2021 •
edited
Loading

Closes #421
Closes #122

Fire the same sequence of events for <form method="get"> submissions
as for <form method="post"> submissions.

The FormSubmission.prepareHeadersForRequest already accounts for
[method="get"] forms by omitting the Accept: header with the custom
turbo-stream MIME type.

Visit actions

Prior to this change, a <form method="get"> would always result in
two Visits: the first with { action: "advance" }, then a second with
{ action: "replace" }.

Since the response to a <form method="get"> has the potential to be a
200 OK or a 201 Created, this commit also modifies the Visit
class to skip the followRedirect() call when the request is idempotent
and the response is not a redirect.

Test changes

The eventLogs mechanisms we have in place declared in
src/tests/fixutres/test.js cannot properly serialize Element
instances, so adding turbo:submit-start and turbo:submit-end
listeners to serialize events for our test suite isn't possible in the
current configuration. To that end, this commit adds assertions for
<form> submit event sequences for all events except those two. In
their place, the suite adds listeners to set [data-form-submit-start]
and [data-form-submit-end] to the <html> element when they fire.

@seanpdoyle seanpdoyle mentioned this pull request Oct 14, 2021
Closed
@seanpdoyle seanpdoyle force-pushed the form-method-get-submit-events branch 3 times, most recently from 89c85d6 to d21181d Compare October 14, 2021 14:37
@seanpdoyle
GET Forms: fire submit-start and submit-end
ac1efe0 
Closes hotwired#421
Closes hotwired#122

---

Fire the same sequence of events for  `<form method="get">` submissions
as for `<form method="post">` submissions.

The [FormSubmission.prepareHeadersForRequest][] already accounts for
`[method="get"]` forms by omitting the `Accept:` header with the custom
`turbo-stream` MIME type.

Visit actions
---

Prior to this change, a `<form method="get">` would _always_ result in
two Visits: the first with `{ action: "advance" }`, then a second with
`{ action: "replace" }`.

Since the response to a `<form method="get">` has the potential to be a
[200 OK][] or a [201 Created][], this commit also modifies the `Visit`
class to skip the `followRedirect()` call when the request is idempotent
and the response is not a redirect.

Test changes
---

The `eventLogs` mechanisms we have in place declared in
`src/tests/fixutres/test.js` cannot properly serialize `Element`
instances, so adding `turbo:submit-start` and `turbo:submit-end`
listeners to serialize events for our test suite isn't possible in the
current configuration. To that end, this commit adds assertions for
`<form>` submit event sequences for all events except those two. In
their place, the suite adds listeners to set `[data-form-submit-start]`
and `[data-form-submit-end]` to the `<html>` element when they fire.

[FormSubmission.prepareHeadersForRequest]: https://github.com/hotwired/turbo/blob/58d2261274533a80a2c5efda7da211b3f20efcbb/src/core/drive/form_submission.ts#L136-L144
[200 OK]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/200
[200 Created]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/201
@dhh
Copy link
Member

dhh commented Oct 18, 2021 

Since the response to a <form method="get"> has the potential to be a
200 OK or a 201 Created, this commit also modifies the Visit
class to skip the followRedirect() call when the request is idempotent
and the response is not a redirect

I don't know if this is a good idea. A GET action should never respond in 200 OK or 201 Created, since it should never be a writing action. Having it as such wrecks havoc with DB balancing and proxies.

@seanpdoyle
Copy link
Contributor Author

I agree, implementing a GET in a way that is not idempotent means trouble. Having said that, a GET response status code is something that applications can set arbitrarily. 200 OK status codes are what a GET typically responds with. A 201 Created is atypical, and most likely incorrect.

Regardless, so long as the status code isn't 3XX, the fetch response won't have its redirected property set to true, which is what this implementation change checks for. In that case, a 200 response should result in an "advance" action instead of an "advance" and "replace" action like the current behavior.

To avoid confusion, I could re-word the commit message to more strongly emphasize that fact instead of mentioning 201s explicitly.

@dhh
Copy link
Member

dhh commented Oct 18, 2021

Lol, yes, of course 200 OK is fine. And agree re: advance vs advance + replace.

@dhh dhh merged commit 9fcb68d into hotwired:main Oct 18, 2021
@seanpdoyle seanpdoyle mentioned this pull request Nov 15, 2021
Closed
seanpdoyle added a commit to seanpdoyle/turbo that referenced this pull request Nov 16, 2021
@seanpdoyle
Frames: handle GET form submissions
c31f4cd 
Closes [446][].

When handling a `<form method="get" action="..."
data-turbo-frame="...">` submission targeting a `<turbo-frame>` element,
responses that don't redirect do not change the element's `[src]`
attribute.

Following the changes made in [424][], `<form>` submissions that result
in `GET` requests are handled the same as other form submissions in that
they fire `turbo:submit-start` and `turbo:submit-end` events. What
[424][] did not account for is navigations initiated by `<form
method="get">` submissions that _do not_ redirect.

In response to those requests, this commit adds two additional criteria
for updating the `<turbo-frame src="...">` attribute:

* the response's status code is [200 OK][], which is idempotent and does
  not indicate a server-side state change (for example, like a [201
  Created][] with a `Location:` header might)
* the response's `Content-Type:` is HTML (not JSON or even Turbo Stream)

Under those circumstances, the frame's `[src]` is updated.

This commit adds test coverage for this new behavior along with
additional coverage to guard against regressions.

[446]: hotwired#446
[424]: hotwired#424
[200 OK]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/200
[201 Created]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/201
@seanpdoyle seanpdoyle mentioned this pull request Nov 16, 2021
Merged
seanpdoyle added a commit to seanpdoyle/turbo that referenced this pull request Nov 16, 2021
@seanpdoyle
Frames: handle GET form submissions
abb060a 
Closes [446][].

When handling a `<form method="get" action="..."
data-turbo-frame="...">` submission targeting a `<turbo-frame>` element,
responses that don't redirect do not change the element's `[src]`
attribute.

Following the changes made in [424][], `<form>` submissions that result
in `GET` requests are handled the same as other form submissions in that
they fire `turbo:submit-start` and `turbo:submit-end` events. What
[424][] did not account for is navigations initiated by `<form
method="get">` submissions that _do not_ redirect.

In response to those requests, this commit adds two additional criteria
for updating the `<turbo-frame src="...">` attribute:

* the response's status code is [200 OK][], which is idempotent and does
  not indicate a server-side state change (for example, like a [201
  Created][] with a `Location:` header might)
* the response's `Content-Type:` is HTML (not JSON or even Turbo Stream)

Under those circumstances, the frame's `[src]` is updated.

This commit adds test coverage for this new behavior along with
additional coverage to guard against regressions.

[446]: hotwired#446
[424]: hotwired#424
[200 OK]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/200
[201 Created]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/201
seanpdoyle added a commit to seanpdoyle/turbo that referenced this pull request Nov 16, 2021
@seanpdoyle
Frames: handle GET form submissions
823d574 
Closes [446][].

When handling a `<form method="get" action="..."
data-turbo-frame="...">` submission targeting a `<turbo-frame>` element,
responses that don't redirect do not change the element's `[src]`
attribute.

Following the changes made in [424][], `<form>` submissions that result
in `GET` requests are handled the same as other form submissions in that
they fire `turbo:submit-start` and `turbo:submit-end` events. What
[424][] did not account for is navigations initiated by `<form
method="get">` submissions that _do not_ redirect.

In response to those requests, this commit adds two additional criteria
for updating the `<turbo-frame src="...">` attribute:

* the response's status code is [200 OK][], which is idempotent and does
  not indicate a server-side state change (for example, like a [201
  Created][] with a `Location:` header might)
* the response's `Content-Type:` is HTML (not JSON or even Turbo Stream)

Under those circumstances, the frame's `[src]` is updated.

This commit adds test coverage for this new behavior along with
additional coverage to guard against regressions.

[446]: hotwired#446
[424]: hotwired#424
[200 OK]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/200
[201 Created]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/201
dhh pushed a commit that referenced this pull request Nov 19, 2021
@seanpdoyle
Frames: handle GET form submissions (#449)
1562a57 
* Frames: handle `GET` form submissions

Closes [446][].

When handling a `<form method="get" action="..."
data-turbo-frame="...">` submission targeting a `<turbo-frame>` element,
responses that don't redirect do not change the element's `[src]`
attribute.

Following the changes made in [424][], `<form>` submissions that result
in `GET` requests are handled the same as other form submissions in that
they fire `turbo:submit-start` and `turbo:submit-end` events. What
[424][] did not account for is navigations initiated by `<form
method="get">` submissions that _do not_ redirect.

In response to those requests, this commit adds two additional criteria
for updating the `<turbo-frame src="...">` attribute:

* the response's status code is [200 OK][], which is idempotent and does
  not indicate a server-side state change (for example, like a [201
  Created][] with a `Location:` header might)
* the response's `Content-Type:` is HTML (not JSON or even Turbo Stream)

Under those circumstances, the frame's `[src]` is updated.

This commit adds test coverage for this new behavior along with
additional coverage to guard against regressions.

[446]: #446
[424]: #424
[200 OK]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/200
[201 Created]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/201

* Fix flaky Progress Bar test

While testing the progress bar rendering, navigate the page to the
`/__turbo/delayed_response` to sleep for 1 second to give the progress
bar a chance to render. Similarly, wait on the `turbo:load` event to
ensure that the progress bar is hidden.
@seanpdoyle seanpdoyle mentioned this pull request Nov 21, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Forms with data-turbo-action?
2 participants
@seanpdoyle @dhh