Restrict Accept: turbo-stream to Form Submissions

[seanpdoyle]

Closes #139

Since Turbo Stream responses over HTTP are in response to user
interactions, and the conventional way to trigger destructive user
interactions is through form submissions, this commit limits the
StreamObserver only inject the Accept header containing the
text/html; turbo-stream content type during form submissions.

Replace the turbo:before-fetch-request event listener
with a turbo:submit-start event listener.

From a Rails perspective, this enables an interesting pattern. Consider
the following controller:

class MessagesController < ApplicationController
  def create
    @message = Message.new(params.require(:message).permit(:body))

    if @message.save
      redirect_to messages_url
    else
      render :new
    end
  end
end

By omitting a respond_to block in the controller's action, the
controller offloads the rendering responsibility to the view layer,
enabling the presence or absence of a template to be the deciding factor
on the type of response:

<%# app/views/messages/new.html.erb %>
<%= turbo_frame_tag @message do %>
  <%= render partial: "form", locals: { message: @message } %>
<% end %>

<%# app/views/messages/new.turbo_stream.erb %>
<%= turbo_stream.replace dom_id(@message, :form) do %>
  <%= render partial: "form", locals: { message: @message }, formats: :html %>
<% end %>

Prior to this change, GET requests initiated by Turbo Drive
navigations were submitting requests with the content type, resulting in
requests to the messages#new being handled by the
messages/new.turbo_stream.erb template, instead of the fully-formed
HTML responses provided by the messages/new.html.erb template.

[inopinatus]

There are plenty of reasons to send a turbo stream response to a GET. Replacing a <tbody> is already a case in point in these issue reports, and I can think of more besides, from menus to form errors to dashboard updates.

Observe also the request for programmatic access in #34. I suspect there will be more uses for turbo stream responses in the future, not fewer.

[seanpdoyle]

There are plenty of reasons to send a turbo stream response to a GET.

This change set proposes that navigational GET requests should omit the Turbo Steam content type from the headers. The turbo:before-fetch-request event listener does not apply to all fetch requests.

Replacing a <tbody> is already a case in point in these issue reports, and I can think of more besides, from menus to form errors to dashboard updates.

I'm not familiar with the tbody example. Could you share a link? To me, these use cases seem to by good matches for GET-powered turbo frames.

Observe also the request for programmatic access in #34. I suspect there will be more uses for turbo stream responses in the future, not fewer.

If there were a Turbo.stream programmatic interface introduced in the future, this changeset wouldn't prevent those requests from submitting with the Turbo Stream content type header.

[inopinatus]

This change set proposes that navigational GET requests should omit the Turbo Steam content type

A dry read through the diff seemed to indicate that turbo frame GETs would also be affected? since they only deliver turbo:before-fetch-request.

I'm not familiar with the tbody example. Could you share a link?

#48 here, but also repeated by others in private spaces, and it's a well-known issue besides when working with component-style custom elements.

these use cases seem to by good matches for GET-powered turbo frames

In general, the opposite is so; autonomous custom elements are phrasing content, so they're not permitted in tables, or other contexts where they'd have obvious potential otherwise e.g. <option> lists, and don't enclose multiple areas of the page, thus making them doubly inappropriate, unless being misused as an ersatz programmatic stream fetch trigger.

This would also be instructing other folks how to go about their business, which some may consider anathematic.

Another possibility could be merging the turbo-frame element, or its constituent parts, into the prototypes of custom extensions to existing table elements, which the DOM should hopefully then permit.

If there were a Turbo.stream programmatic interface introduced in the future, this changeset wouldn't prevent those requests from submitting with the Turbo Stream content type header.

I was rather expecting to simply fire a turbo:before-fetch-request from my own stuff to maintain a loose coupling, unless/until a Turbo.stream comes along, or possibly use a Hey.com-style controller<>frame whammy for programmatic visits. With this change, I'd be looking instead at using an inappropriate turbo:submit-start, talking directly to Turbo internal APIs, or using a POST for things that are not actually POSTs.

If the intention is to identify Turbo Drive GETs contextually to shave a line or two of format negotiation from controller methods, then I'd definitely suggest circling around on the signalling mechanisms and consider how to elegantly achieve just that, rather than carving it as negative space from other parts.

[kaspth]

Looks good! Already reviewed the enctype changes, so this is pretty simple really.

[Menduist]

Sorry to bump this, but if the goal is to remove the turbo-stream header from navigational GETs, shouldn't the forms with method="get" action="" include the turbo-stream accept, since they're not navigational per se?

Example use case:

<form method="get" action="">
  <input type="search" name="s">
  <button type="submit">Search</button>
</form>
<ul id="results">[Whatever]</ul>

Server side (pseudocode) :

if headers["Accept"].contains("text/vnd.turbo-stream.html"):
  sendTurboStream(action="replace", id="results", value=computeResults())
else:
  sendFullPage(computeFullPage())

The form could be a POST, but then you wouldn't have the ?s=xx in the URL, which is needed in some cases (eg, to "save the state")
And in the current version, with GET, the full page must always be sent, which is a bit wasteful

[feliperaul]

@seanpdoyle @dhh Given the number of issues and questions regarding Turbo stop handling GET requests (see kaminari/kaminari#1067 (comment)), I think it would be great if this could be reconsidered.

I too find it immensely useful to have a link trigger a GET request for a .turbo_stream.erb response capable of aggregating multiple turbo_stream responses. This is the biggest feature missing from the good .js.erb days IMHO.

What about if regular links included the turbo Accept header, but only if they were annotated with data: { turbo_method: :get }, now that turbo handles the data-method attribute?

[dhh]

I'd be happy to see a way where we can force a GET to process turbo streams. I think the data-turbo-method=get is an interesting approach. Could also go with a specific one like data-turbo-stream=true. Thoughts @seanpdoyle?

[feliperaul]

Happy that you agree that there should be a way of achieving this!

My vote would be for data: { turbo_method: :get }, because it keeps it consistent (it's also the option I'd intuitively try without looking at any documentation) and it avoids creating another data attribute that won't be present on the others data-turbo-methods that will always accept turbo-streams.

[cpanderson]

Further to using a specific one is there currently, or could there be, support for adding this to a link_to helper? Maybe with an easy option like "turbo_stream: true"? In this case setting data: {turbo_method: :get} on a request that is :get by default might make it less confusing. Or would this be outside the scope for a link_to helper?

[feliperaul]

@cpanderson Yes, I think the data: { turbo_method: :get } could be used with link_to, button_to, or forms.

@seanpdoyle Sean, first of all, thank you for the huge contribution on Turbo. I'm learning a lot from https://github.com/thoughtbot/hotwire-example-template (for the ones that don't know, there's a different branch for every tutorial there).

Sorry to bump this, but did you have any time to consider this proposal? By the number of votes on DHH's comment above, it seems this would be a greatly appreciated addition!

[seanpdoyle]

Could also go with a specific one like data-turbo-stream=true

Is writing to the request headers something that applications could opt-into themselves using events that are already available?

addEventListener("submit", ({ target: formElement, submitter }) => {
  const forcesTurboStream = submitter?.hasAttribute("data-turbo-stream") || target.hasAttribute("data-turbo-stream")

  if (forcesTurboStream) {
    formElement.addEventListener("turbo:submit-start", ({ detail: { formSubmission }) => {
     formSubmission.fetchRequest.headers["Accept"] = "text/vnd.turbo-stream.html"
    }, { once: true })
  }
})

[bilogic]

addEventListener("submit", ({ target: formElement, submitter }) => {
  const forcesTurboStream = submitter?.hasAttribute("data-turbo-stream") || target.hasAttribute("data-turbo-stream")

  if (forcesTurboStream) {
    formElement.addEventListener("turbo:submit-start", ({ detail: { formSubmission }) => {
     formSubmission.fetchRequest.headers["Accept"] = "text/vnd.turbo-stream.html"
    }, { once: true })
  }
})

Does this mean the above needs to be written for every form that needs it? I'm doing a GET filter=search_term