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.

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

Allow Turbo Streams with GET requests #621

Closed
wants to merge 1 commit into from
Closed

Allow Turbo Streams with GET requests #621

wants to merge 1 commit into from

Conversation

kevinmcconnell
Copy link
Collaborator

Note: this is an alternative to #612

This PR makes Turbo Streams work with all Turbo HTTP requests, including GET requests.

Motivation

Currently, Turbo restricts the HTTP use of Turbo Streams to non-GET form submissions. This makes sense when those stream responses correspond to server-side state changes. However there are also cases where it's useful to send targeted DOM updates when not updating server state.

We already allow targeted DOM updates with any request method when using Turbo Frames. And in cases where a request affects a single DOM element, Turbo Frames will often be the most straightforward option. But when you have a change that doesn't fit within a frame, we should make it easy for people to reach for Turbo Streams instead.

Some situations where you might want a stream response to a GET include:

  • Updating multiple DOM areas at once, such as to change a header when switching in the editable view of a component.
  • Any time the inclusion of a <turbo-frame> element would disrupt layout.
  • When you're appending content, rather than replacing it, for example adding repeated sections to a form.

Alternatives

  • Allow Turbo Streams with GET via data-turbo-stream #612 provides a way to opt-in to this behaviour, via a new data-turbo-stream attribute. That has the advantage of not changing default behaviour. The downside is that needing to use an attribute to enable streams over GET seems non-obvious (it's not needed for Turbo Frames, for example), so we would rely on documentation to help users find it.

  • It's possible to achieve this behaviour in application code by intercepting turbo:before-fetch-request to modify the Accept header. However, putting the onus on the user to do this goes against Turbo's spirit of reducing the need for client-side JavaScript, and again, it's not obvious why it should be needed.

Previously, Turbo Streams could only be used with non-GET form
submissions. However there are also cases where it's useful to have a
Turbo Stream response to a GET action.

With this change, all Turbo-initiated requests will include the Turbo
Stream type in the `Accept` header, so that Turbo Streams can be used
with any of them.
@seanpdoyle
Copy link
Contributor

As the author of #52, I'd like to chime in here.

The scenario outlined in #52 involves a hypothetical messages/new template:

<%# app/views/messages/new.html.erb %>

<h1>New message</h1>

<%= render partial: "form", locals: { message: @message } %>

that submits to a messages#create controller action:

# app/controllers/messages_controller.rb

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

    if @message.save
      redirect_to @message
    else
      render :new, status: :unprocessable_entity
    end
  end
end

There is more than a decade's worth of documentation and search results for Rails Controller samples that resemble this block of code.

To editorialize a bit, most #create actions should resemble this one. It traffics in Standards-based HTTP and HTML concepts: redirect with a 30x status code with a Location: header on success, or render a full HTML document with a 422 status code on failure. It won't break in the absence of a JavaScript-capable client. It's "pure" Rails, and isn't aware of the presence or absence of Turbo support on the client.

It's sturdy, and serves as a foundation for client-side code to progressively enhance.

Globally including the Turbo Stream Accept: header into every Turbo-initiated GET request's could add some surprising pitfalls to conventional Rails patterns.

For example, suppose a teammate introduces the messages/new.turbo_stream.erb template to progressively enhance the messages#create action's failure branch:

<%# app/views/messages/new.turbo_stream.erb %>

<%= turbo_stream.replace dom_id(@message, :form) do %>
  <%= render partial: "form", locals: { message: @message }, formats: :html %>
<% end %>

Invalid POST requests to /messages made with the Accept: header are routed to the messages#create action, which renders the messages/new.turbo_stream.erb template.

In response to invalid POST requests to /messages made without the Accept: header, the controller would render the messages/new.html.erb template.

GET requests made to /messages/new **without the Accept: header** are routed to the messages#new action, which renders the messages/new.html.erb template.

However, if the Accept: header were present in all GET requests, the controller would respond to Turbo Drive-initiated navigations by rendering the messages/new.turbo_stream.erb template instead of the messages/new.html.erb template. That would break HTML full-page navigations.

Similarly, suppose a teammate were to introduce the messages/show.turbo_stream.erb template:

<%= turbo_stream.replace dom_id(@message) do %>
  <%= render partial: "message", locals: { message: @message }, formats: :html %>
<% end %>

The redirect_to @message line in the success branch would result in a 30x redirection response to a route handled by the messages#show action. The subsequent GET request made to follow the redirect would be made with the same headers as the initial request, which would include the Turbo Stream Accept: header. As a result, the response would be the messages/show.turbo_stream.erb template instead of the messages/show.html.erb template. Direct navigations to the messages#show action would also render the .turbo_stream.erb template instead of the .html.erb template, which would break HTML full-page navigations.

The presence of the *.turbo_stream.erb templates has the potential to break the conventions that underpin how Rails controllers respond to HTML GET requests without making any changes to the controller code.

Personally, I don't have a lot of experience with transmitting .turbo_stream.erb templates in response to GET requests, so I don't have a good sense of what the pain points are. If it's a popular pattern, I think built-in support is totally reasonable. Supporting a [data-turbo-stream] attribute (like what's proposed in #612), or a [turbo:submit-start] event listener or another pattern entirely).

With that being said, I believe that requiring applications to opt-in will lead to fewer surprises than including the header by default and requiring applications to opt-out.

The downside is that needing to use an attribute to enable streams over GET seems non-obvious (it's not needed for Turbo Frames, for example), so we would rely on documentation to help users find it.

My gut feeling is that we should improve the documentation around this and expand upon what's explained in hotwired/turbo-site#40.

@seanpdoyle
Copy link
Contributor

seanpdoyle commented Jul 3, 2022

My gut feeling is that we should improve the documentation around this and expand upon what's explained in hotwired/turbo-site#40.

For example, it might not be obvious that a <turbo-frame> (or any other part of the document, for that matter) can contain a <turbo-stream> element, and that <turbo-stream> element would become active immediately on connection to the document.

But when you have a change that doesn't fit within a frame, we should make it easy for people to reach for Turbo Streams instead.

In your use-case, could the singularly navigated <turbo-stream> embed a set of <turbo-stream> elements to target parts of the document outside of the <turbo-frame>?

<a href="/my-pages/1" data-turbo-frame="my-frame">Drive the frame</a>

<turbo-frame id="my-frame">
  <h1><%= @page.name %></h1>

  <turbo-stream action="replace" target="navigation">
    <template><%= render "navigation", current_page: @page %></template>
  </turbo-stream>
</turbo-frame>

@pinzonjulian
Copy link

pinzonjulian commented Jul 3, 2022

I see this conversation a lot online; of people needing/wanting to target multiple dom changes in response to GET requests.

I would encourage gathering detailed use cases for this (hopefully with diagrams or even videos) and understanding why there's such a need.

I think users of StimulusReflex and Cable Ready would have great insights since those tools encourage an RPC style that does not have these constraints. They tend to make very complex dom operations in response to user "GET" interactions

Edit: I've shared this in the StimulusReflex server to hopefully get some insights

@kevinmcconnell
Copy link
Collaborator Author

kevinmcconnell commented Jul 3, 2022

@seanpdoyle thanks for chiming in! You make a good point about that existing pattern being widely used. This would be a breaking change for that example. I think there are some clear ways to solve that - like having the create action render it’s own stream response - and I think it’s conceptually clearer to separate stream responses from the request type, which is why I wanted to bring this option up for discussion. But Turbo has been working this way for quite a while so any breaking changes around this would be disruptive.

I believe that requiring applications to opt-in will lead to fewer surprises than including the header by default and requiring applications to opt-out.

I agree, that’s definitely the safer way, and possibly the only way to do it that doesn’t involve some breaking change.

What do you think about moving ahead with #612? That would make it easy to opt in without changing any default behaviour. From browsing past discussions, it seems like that specific behaviour has been requested quite a few times.

@kevinmcconnell
Copy link
Collaborator Author

In your use-case, could the singularly navigated <turbo-stream> embed a set of <turbo-stream> elements to target parts of the document outside of the <turbo-frame>?

That's definitely a possible workaround, although I think it pushes some extra complexity and inconsistency into app code, for something that seems like it should be simple to do. It requires adding a <turbo-frame> when we're not really using frames; we'd just be using one to get the side effect of preventing navigation so the <turbo-stream> elements can be processed. It also means that the stream responses in that case will be text/html documents (.html.erb in Rails) whereas the other streamed responses are text/vnd.turbo-stream.html (.turbo_stream.erb in Rails), which is inconsistent. To me, at least, this approach looks more like a side effect than an intentional design decision.

If this is an intended way to use frames & streams together then I agree that the documentation should be updated to make it clear that this can be relied on. Currently the documentation says:

Turbo knows to automatically attach <turbo-stream> elements when they arrive in response to <form> submissions that declare a MIME type of text/vnd.turbo-stream.html

..which isn't what's happening that that case.

In my case, I added a little turbo:before-fetch-request shim to enable the GET behaviour instead. But I still think the ability to request a stream response over GET should be built-in, and as simple/obvious as possible. It certainly makes sense for that to be opt-in at this point, since there may be cases where folks are depending on the existing behaviour. But solutions that involve restructuring the markup, introducing extra frames, or writing client-side JavaScript are just not as appealing as something like an annotation (which still require documenting, but at least are declarative and less intrusive).

@kevinmcconnell
Copy link
Collaborator Author

I'm going to close this so we can focus on solutions that don't introduce any breaking changes. Probably better to have the discussion on #612 instead, for that reason.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants