Turbo new install - forms do not redirect

kiddrew commented 3 years ago

I just installed Turbo in an existing Rails 6 app and all my existing forms are broken. They submit as expected but the redirect after doesn't happen. I'm able to interact with Turbo as expected - ie, I have a frame loading correctly, so it appears I loaded Turbo correctly in Webpack.

import "@hotwired/turbo-rails"

And with just a very simple form (using slim and simple_form):

= simple_form_for @comment do |f|
  = f.input :text, label: "Comment"
  = f.button :submit

My controller performs the redirect (using responders):

class CommentsController < ApplicationController
  load_and_authorize_resource

  def index
    @comment = Comment.new
  end

  def create
    @comment.save
    respond_with @comment, location: comments_path
  end

  private

  def resource_params
    params.require(:comment).permit(
      :text
    )
  end
end

The comment gets created and the request for the redirect URL happens but the redirect does not. I have to refresh to see the changes.

SleeplessByte commented 2 years ago

The solutions are in this issue.

In short: use redirect_to x, status: :see_other whenever you are redirecting from a form submission, use status: :unprocessible_entity to render errors in-line (so without redirecting), or disable turbo on the form.

vikdotdev commented 2 years ago

I had troubles with status: :unprocessible_entity not rendering any changes onto the screen (even though the HTML was returned from the server). It turns out turbo-rails require .html in the view template name. I had a bunch of new.slim or edit.slim without the .html part, and that was what was causing trouble.

Is this documented anywhere? Couldn't find anything about the correct template naming.

SleeplessByte commented 2 years ago

I think that if you don't have .html in the template, then the content_type is going to be something else. Basically rails will try to infer the content type from the rendered content.

kiddrew commented 2 years ago

If what @vikdotdev says is true (I haven't attempted to reproduce), seems like that should be logged as a bug. A .html.slim file extension isn't technically correct. The file isn't HTML, it's Slim. Slim converts to HTML but it is not HTML. Like coffee is to JS.

archonic commented 2 years ago

This would explain the trouble I had with Turbo. In an existing app which uses haml, upgrading to Turbo broke all forms which had a redirect response. Using status: :see_other never got the response to render. I still just have Turbo disabled on all forms which made me wonder what the point of upgrading from Turbolinks was. That should absolutely be a bug.

SleeplessByte commented 2 years ago

@kiddrew

It's been like this since Rails 1 as far as I recall, so I think a non-discussion for this issue. ERB files are generally written as .html.erb, because you can also have .js.erb which is "run this JavaScript file through the ERB handler first. IRRC, you can also do things like: .json.jbuilder.erb.

I use media type negotiation a lot and this isn't inherent to Turbo or Devise, or any other library. What's new in turbo is that it "forces" content negotiation (the responds_to block), which is why you may not have ran into this before.

I don't necessarily agree with the implementation but that is the explanation.

kiddrew commented 2 years ago

@SleeplessByte Interesting. I admittedly have very little experience with Turbo so far. Do you know why the media type determination is different (non-existent?) in Turbo vs normal renders? Is there a technical reason or is this just a nuance of the implementation? I don't know what happens behind the scenes, but I name all my slim templates with a .slim extension and normally don't have any trouble.

archonic commented 2 years ago

FYI the haml / slim issue has already been confirmed here: https://github.com/hotwired/turbo-rails/issues/287.

It was exceedingly difficult to troubleshoot that issue and I only noticed because @SleeplessByte mentioned it on a thread I've been following for years.

SleeplessByte commented 2 years ago

The confirmed post also shows that this is the convention rails expects.

TL;DR is at the bototm of this post.

@kiddrew yes! Happy to explain. @archonic I agree. This stuff is super hard to debug too because of the massive amounts of indirection in turbo and rails.

Media type registration

So in the new turbo you will find this piece of code:

https://github.com/hotwired/turbo-rails/blob/102e919318ec528b9f5908e10d6427847d7a197b/lib/turbo/engine.rb#L50-L52

This ensures you can call https://example.org/your/endpoint.turbo_stream to forgo content-negotiation, in the same way :html is registered to facilitate https://example.org/your/endpoint.html. It also allows you to write code like this:

respond_to do |format|
  format.turbo_stream do
    # respond to a turbo stream request
  end
  format.html do 
    # respond to an html request
  end
  format.any do
    # anything else
  end
end

When is the turbo_stream block executed? It's executed when rails understands that it "should".

Because the format is explicitly set in the URL (.turbo_stream). This is not content negotiation.
Following content negotiation, meaning the Accept header contains the text/vnd.turbo-stream.html media type and it is more important than anything else acceptable. In the example case, because of .any any valid media type is acceptable.

When a block is executed, it will by default set the content-type to whatever format was accepted. So, if the turbo_stream is executed, the Content-Type is set to text/vnd.turbo-stream.html if some code requests it.

Renderer creation

Often, you will not have any respond_to block in your code, but rather you'll do something like:

render json: my_json

# or

render plain: my_plain

This internally uses a Rails Renderer instead. So for example, if you want to be able to write render csv: my_csv_obj and that object be turned into csv content by calling to_csv on it, you could do something like this:

ActionController::Renderers.add :csv do |obj, options|
  filename = options[:filename] || 'data'
  str = obj.respond_to?(:to_csv) ? obj.to_csv : obj.to_s
  send_data str, type: Mime[:csv],
    disposition: "attachment; filename=#{filename}.csv"
end

For turbo, the code is:

https://github.com/hotwired/turbo-rails/blob/102e919318ec528b9f5908e10d6427847d7a197b/lib/turbo/engine.rb#L55-L60

What this shows is: return the content to render verbatim, which means you need to do render turbo_stream: valid_html. It will, however, set the Content-Type to text/vnd.turbo-stream.html unless it's already set.

Default render

These two things together may help you understand what happens when you do not have respond_to blocks and do not call render turbo_stream: .... but rather have the following:

def show
 # render
end

Either explicit render call without a renderer option, or no render call at all (implicit render).

What happens under the hood is a chain of calls that effectively call render_to_body, usually on ActionView:

https://github.com/rails/rails/blob/04972d9b9ef60796dc8f0917817b5392d61fcf09/actionview/lib/action_view/rendering.rb#L108-L124

def _render_template(options)
  variant = options.delete(:variant)
  assigns = options.delete(:assigns)
  context = view_context

  context.assign assigns if assigns
  lookup_context.variants = variant if variant

  rendered_template = context.in_rendering_context(options) do |renderer|
    renderer.render_to_object(context, options)
  end

  rendered_format = rendered_template.format || lookup_context.formats.first
  @rendered_format = Template::Types[rendered_format]

  rendered_template.body
end

This in turn finds its template via context.in_rendering_context which comes from ActionView as well: https://github.com/rails/rails/blob/04972d9b9ef60796dc8f0917817b5392d61fcf09/actionview/lib/action_view/base.rb#L257-L274

def in_rendering_context(options)
  old_view_renderer  = @view_renderer
  old_lookup_context = @lookup_context

  if !lookup_context.html_fallback_for_js && options[:formats]
    formats = Array(options[:formats])
    if formats == [:js]
      formats << :html
    end
    @lookup_context = lookup_context.with_prepended_formats(formats)
    @view_renderer = ActionView::Renderer.new @lookup_context
  end

  yield @view_renderer
ensure
  @view_renderer = old_view_renderer
  @lookup_context = old_lookup_context
end

...which finally uses the lookup_context to see which templates are really available. It will use the provided views_paths (usually a list of all the view paths in your project) to find files and tries to match these things:

https://github.com/rails/rails/blob/04972d9b9ef60796dc8f0917817b5392d61fcf09/actionview/lib/action_view/lookup_context.rb#L43-L52

locale (like en)
formats (like html)
variants (like mobile)
handlers (like haml)

We can go deeper, but the important thing to understand is that a file index.html+mobile.haml will be _processed (handled) by haml, only be matchable if the variant requested is mobile, and will output html (because that is the format).

The matched template will actually set the format (see @rendered_format above).

Turbo, and why `index.haml` fails

Okay. Final step. Turbo doesn't use native browser navigation but rather a fetch(y) request. It will create a special request. This will happen first when a form is being submitted, first via requestStarted which dispatches the turbo:submit-start event, and then via start, which, if allowed (confirm) will call the fetch request perform function.

(We're almost at the point where everything lines up).

This function calls prepareHeadersForRequest on the form submission which basically checks if the form will understand a Turbo Streams response and if so adds text/vnd.turbo-stream.html to the Accept header. If it doesn't, then it only accepts text/html, application/xhtml+xml.

WHY does it accept turbo streams? Because of this line: https://github.com/hotwired/turbo/blob/256418fee0178ee483d82cd9bb579bd5df5a151f/src/core/drive/form_submission.ts#L224

The form submission is not idempotent (aka a POST) <-- this.
Or some data attribute is present

Your controller does it's action and then redirects to a new location by passing the location header. After it has redirected, it renders the index action and the response is sent back to the client, right?

The code that determines what gets called when it returns can be found here: https://github.com/hotwired/turbo/blob/256418fee0178ee483d82cd9bb579bd5df5a151f/src/core/drive/form_submission.ts#L180-L196

Either it succeeds or it fails, the delegated functions are here: https://github.com/hotwired/turbo/blob/aeeaae8edb5f6ec6ef6b19eaf93dc060a1e67b10/src/core/drive/navigator.ts#L92-L126.

The particular lines that we care about are:

const responseHTML = await fetchResponse.responseHTML
if (responseHTML) {
  // do stuff
}

That responseHTML property is defined here: https://github.com/hotwired/turbo/blob/aeeaae8edb5f6ec6ef6b19eaf93dc060a1e67b10/src/http/fetch_response.ts#L50-L56

get isHTML() {
  return this.contentType && this.contentType.match(/^(?:text\/([^\s;,]+\b)?html|application\/xhtml\+xml)\b/)
}

get responseHTML(): Promise<string | undefined> {
  if (this.isHTML) {
    return this.response.clone().text()
  } else {
    return Promise.resolve(undefined)
  }
}

This basically says: if the content type is text/html or the xhtml equiv, then I know it's HTML and I can interpret it. In that case only process the response and let turbo visit the new content. It does match text/vnd.turbo-stream.html as well, (and anything that ends with html).

Okay. So. Why doesn't it then?

You use implicit render in your controller
Implicit render will try to match the current request which has Accept: text/vnd.turbo-stream.html, text/html, application/xhtml+xml. The order is significant.
It cannot find any file that has a format of .turbo_stream or .html. So then it tries to look for a less specific thing. It finds a file without format called index.haml.
When the template is picked it will use the file's format OR THE FIRST FORMAT FROM THE ACCEPT HEADER (refresher: rendered_format = rendered_template.format || lookup_context.formats.first)
When you're using implicit render, it will USE THE FORMAT THAT THE RENDERER RENDERED.
The haml is rendered with Content-Type: text/vnd.turbo-stream.html.
Turbo receives the HTMl and starts interpreting it as a stream...
...a stream requires something to stream into, but you're rendering a document.
It doesn't render.

Hope it helps.

TL;DR

Turbo adds support for turbo streams because it wants to be able to respond to a stream response which is allowed because the form is a POST.
It does this by adding a media type to the Accept header.
Your files always assumed you're rendering a single media type (called action.handler instead of action.format.handler).
Rails will always use the first format if none was given in a file name.
Because turbo stream value is prepended, your rendered view will be rendered as (Content-Type) a turbo stream.
Turbo will not allow you to render the response because it thinks it's a stream responses, without a place to stream to and because it can only render a document HTML.

kiddrew commented 2 years ago

@SleeplessByte thanks for the thorough explanation!

SleeplessByte commented 2 years ago

There was a slight error in the final section which I've since fixed. TL;DR stays the same :)

johnykov commented 2 years ago

I've stumbled upon this issue yesterday. My App relies on 2 format renderers in update method in controller: turbo_stream and html. In one case I do post form with rails-ujs from haml template (submit button on top of the page, above form) and CTRL instead of html renderer it always used turbo_stream. By some kind of intuition I've re-ordered renderers and it worked. And today in the morning I've discovered great explanation of why it worked - default renderer. Great job @SleeplessByte ! Thx a ton

danielricecodes commented 2 years ago

I had troubles with status: :unprocessible_entity not rendering any changes onto the screen (even though the HTML was returned from the server). It turns out turbo-rails require .html in the view template name. I had a bunch of new.slim or edit.slim without the .html part, and that was what was causing trouble.

Is this documented anywhere? Couldn't find anything about the correct template naming.

FYI - its unprocessable_entity. an a not an i.

danielricecodes commented 2 years ago

@SleeplessByte - I really appreciate the thorough response but this issue is maddening. On a brand new Rails 7.0.3.1 app I used rails generate scaffold quote name:string which generates the following:

class QuotesController < ApplicationController
  before_action :set_quote, only: %i[ show edit update destroy ]

  # GET /quotes or /quotes.json
  def index
    @quotes = Quote.all
  end

  # GET /quotes/1 or /quotes/1.json
  def show
  end

  # GET /quotes/new
  def new
    @quote = Quote.new
  end

  # GET /quotes/1/edit
  def edit
  end

  # POST /quotes or /quotes.json
  def create
    @quote = Quote.new(quote_params)

    respond_to do |format|
      if @quote.save
        format.html { redirect_to quote_url(@quote), notice: "Quote was successfully created." }
        format.json { render :show, status: :created, location: @quote }
      else
        format.html { render :new, status: :unprocessable_entity }
        format.json { render json: @quote.errors, status: :unprocessable_entity }
      end
    end
  end

  # PATCH/PUT /quotes/1 or /quotes/1.json
  def update
    respond_to do |format|
      if @quote.update(quote_params)
        format.html { redirect_to quote_url(@quote), notice: "Quote was successfully updated." }
        format.json { render :show, status: :ok, location: @quote }
      else
        format.html { render :edit, status: :unprocessable_entity }
        format.json { render json: @quote.errors, status: :unprocessable_entity }
      end
    end
  end

  # DELETE /quotes/1 or /quotes/1.json
  def destroy
    @quote.destroy

    respond_to do |format|
      format.html { redirect_to quotes_url, notice: "Quote was successfully destroyed." }
      format.json { head :no_content }
    end
  end

  private
    # Use callbacks to share common setup or constraints between actions.
    def set_quote
      @quote = Quote.find(params[:id])
    end

    # Only allow a list of trusted parameters through.
    def quote_params
      params.require(:quote).permit(:name)
    end
end

NONE OF THE SUGGESTIONS ABOVE ARE PRESENT IN THIS FILE, YET THE FORMS AND EVERYTHING WORKS OUT OF THE BOX. I do not need to copy the view template code into this reply because it's all unedited scaffold. In a Rails 7 scaffold controller, there is no :see_other, there are no respond_to blocks, and it is using implicit rendering....and yet....scaffold works out of the box while upgraded apps are horked. 😡

That said, it is extremely challenging to tell what the problem is with my recently upgraded Rails 7 app when such a basic scaffold works perfectly and without any of the above suggestions. My Rails 7 app has @rails/ujs removed and there are no remote forms. Nothing with Rails UJS is interfering here. For all intents and purposes, my Rails 7 app's forms should be working but they are not.

I will keep working on it and post my solution when I find it but good gravy this issue needs to be fixed or the upgrade docs need to be a lot more clearer than they are.

laptopmutia commented 2 years ago

The solutions are in this issue.

In short: use redirect_to x, status: :see_other whenever you are redirecting from a form submission, use status: :unprocessible_entity to render errors in-line (so without redirecting), or disable turbo on the form.

could u help me sir? I have already add status: :see_other but still the page is not redirected

  def lock
    if @enrollment.update(wlkp_params.merge(lock: true))
      respond_to do |format|
        format.html { redirect_to ranking_path, notice: "Pendaftaran berhasil disimpan", status: :see_other}
        # format.turbo_stream { flash.now[:notice] = "Pendaftaran berhasil di submit." }
      end
    else
      render "enrollments/current", notice: "Error harap hubungi admin", status: :unprocessable_entity
    end
  end

I could see the response of 303/302 in inspect element networks and the 200 ok of the redirected page but my browser window still show the game page

================================

I have found the root problem and solution for this

this is because my response is not enough have html element so turbo not behave accordingly

when I add more html code to the response its redirected normaly

SleeplessByte commented 2 years ago

@danielricecodes what's the file names of the views? They are probably .html.erb.

What are your file names?

glaszig commented 1 year ago

@kiddrew is your simple_form_for inside any turbo-frame? if so, turbo will load the redirect via ajax and not do any actual redirect or Turbo.visit().

@danielricecodes i just tried your scaffold example in a fresh rails 7 app. everything works. like with turbolinks. but there also is no turbo-frame at play.

i was scimming through turbo's code quickly and from what i've seen the following happens:

everything inside a turbo-frame tag is handled through a FrameController. it'll submit forms, detect redirects, load the resource redirected to, scan the response for turbo frames and replace those on the page. that's why you won't see any other change on the page.

now, for a form outside of a turbo-frame, the Navigation class will handle the submission and actually do a redirect (or execute a Turbo.visit()).

so, there you have it. you cannot break out of a turbo-frame. which is unfortunate i think.

my use case:

load modal content into a turbo-frame
do everything inside that frame (have a form, submit form, handle validation error)
do an actual redirect/Turbo.visit() upon successful submit

danielricecodes commented 1 year ago

@danielricecodes what's the file names of the views? They are probably .html.erb.

What are your file names?

This was actually the problem. Wow 🤯

My editor was saving files with only the .slim extension. Explicitly using .html.slim fixed my app.

awolfson commented 1 year ago

A million thanks to @SleeplessByte for thoroughly demystifying this issue that folks have been battling against for—checks date when this issue was created—two years and one day! I wish there was a way to pin your answer to the top, especially since it feels like there's unlikely to be any remediation (unless I've missed something).

(If you haven't read it, read it.)

It seems like maybe not prepending the Turbo Streams content type would fix it, though, right? Assuming that would not break Turbo Streams, I would like to see that happen, and here's why.

The reason that so many are reporting that form redirection is broken out of the box is that we are following two longstanding Rails conventions in our new apps:

Redirection with implicit render
Format-less template filenames

But because we now want to allow requests for Turbo Streams to work seamlessly, we've disrupted this pattern, so that you have to change one of those two conventions in your app just to get form submissions to work.

Wouldn't it be better to shift the burden onto the newer feature, Turbo Streams, by appending it to the Accept header instead of prepending it?

SleeplessByte commented 1 year ago

Yes and no.

Prepending signals "I prefer a turbo stream response" which you do. If you'd append it and the controller can return a html response, it will, despite a turbo stream response being available.

The real issue is that rails is defaulting to turbo stream implicitly. That's the real bug!

Edit: rails is returning a full document as a stream which it should do imo. Unless you explicitly opt in.

supaplexer commented 1 year ago

I've just gone through every comment and I think my problem differs in a way that my create action was correctly redirecting with turbo-rails 1.3.3 but stopped doing so after upgrading it. After upgrading the gem to 1.4.0 I can still see the 302 response but it's actually staying in the same view and updating the turbo-frame with "Content missing". Here's the code:

def create
    @msg = current_user.msgs.new(msg_params)

    respond_to do |format|
      if @msg.save
        format.html { redirect_to msgs_path, notice: 'Message was created.' } # <-- this was fine in 1.3.3
      else
        format.turbo_stream do
          flash.now[:alert] = "An error occurred"
          render turbo_stream: render_error
        end
        format.html do
          redirect_to msgs_path, alert: "An error occurred"
        end
      end
    end
  end

As you can see it's pretty basic and I just have an html response when a message is saved. Of course the form is submitting a POST.

koenhandekyn commented 1 year ago

    format.html { redirect_to msgs_path, notice: 'Message was created.' } # <-- this was fine in 1.3.3

fighting the same issue here

as a fix, if we explicitly add format :html to the path, then the behaviour is 'ok' again, but it's not really nice to have ".html" appearing in the path suddenly.

redirect_to client_sheets_path(format: :html), flash: { notice: I18n.t("controller.orders.create.order_created") }, status: :see_other

awolfson commented 1 year ago

    format.html { redirect_to msgs_path, notice: 'Message was created.' } # <-- this was fine in 1.3.3
fighting the same issue here

as a fix, if we explicitly add format :html to the path, then the behaviour is 'ok' again, but it's not really nice to have ".html" appearing in the path suddenly.
redirect_to client_sheets_path(format: :html), flash: { notice: I18n.t("controller.orders.create.order_created") }, status: :see_other

@koenhandekyn What is the name of your template file that renders at client_sheets_path? If it doesn't have .html in the filename, you should be able to add it, and remove format: :html from the redirect. For example, renaming index.haml to index.html.haml.

koenhandekyn commented 1 year ago

@awolfson that worked indeed. i'm a bit puzzled still as i thought it did the 'same' by make the respond_to :html explicit on that controller end point (and that didn't seem to help). - and also i remember reading at some point advice/documentation against adding the double extension :) -

awolfson commented 1 year ago

@awolfson that worked indeed. i'm a bit puzzled still as i thought it did the 'same' by make the respond_to :html explicit on that controller end point (and that didn't seem to help). - and also i remember reading at some point advice/documentation against adding the double extension :) -

@koenhandekyn Yeah, you're not supposed to need the double extension, that's part of the bug. You'll want to read this post for the full explanation of what's happening: https://github.com/hotwired/turbo-rails/issues/122#issuecomment-1217280952

QueeniePeng commented 1 year ago

Is anyone else experiencing an issue where they want to use turbo-confirm to add a confirmation dialog before archiving a record, while also keeping turbo: false so they can redirect to a new page if the record is saved? The form is within a turbo_frame_tag.

hotwired / turbo-rails