-
Notifications
You must be signed in to change notification settings - Fork 330
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
Turbo new install - forms do not redirect #122
Comments
Thank you for reporting this. Does your application run RailsUJS? Are your forms created with data-remote="true"? |
I'm not using data-remote. I did have UJS enabled, but I disabled it to test and that didn't help either. |
same issue here |
I had to explicitly use Perhaps that helps? |
@SleeplessByte That is correct. It's not well documented yet, but Rails is switching to returning a 422 on fail: rails/rails#41026 Devise will be ready for this change soon: heartcombo/devise#5340 Really excited to start playing with these new toys 🎉 |
@seanpdoyle there is a small issue with redirects. respond_to do |format|
format.html { redirect ...} <---- This will not redirect. Request is a js.erb, worked on turbolinks
end |
@dixpac If RailsUJS is present and Am I understanding your question properly? |
I tried modifying the response to use |
@seanpdoyle the problem is when migrating larger apps there is a lot of complex rails UJS code, which is impossible to migrate on So to make it work some forms need to be remote with This is maybe a different issue from the one @kiddrew posted. |
@dixpac thsnks for clarifying. Let's open a different issue then. As part of the new issue, could you please help us understand why we'd want to set |
@seanpdoyle sure, I will open issue or a PR but I need more time to inspect what is happening 😄
It is |
@dixpac right, is |
Yes. This works on turblinks and doesn't work with turbo( # Form is remote: true
def create
respond_to do |format|
if person.save
format.html { redirect .... } # redirect
else
format.js # Do some DOM manipulations with js
end
end
end |
Thanks @dixpac. Could you open a separate issue? |
@seanpdoyle same issue here. Turbo always handles the form even the form has
|
@Petercopter --- I see heartcombo/devise#5340 seems to fix the problem I have steps to reproduce for Rails 6.1.3 and Devise 4.7.3 here: https://stackoverflow.com/questions/66615478/turbo-rails-with-devise-does-not-redirect-consistently-rails-6-1-3-devise-4-7-3 I'd love to be able to upgrade my Rails 6.1 app for Turbo-Rails today, but my upgrade is riddled with these Devise bugs-- mostly redirects that don't redirect in the browser but the action has happened on the backend. I see this pull appears to fix everything (🎉 !), true? false? truthy? falsy? in master soon? Should I wait a bit? I guess if it were in master I could point my gem to the master branch of devise? Any tips appreciated, even just a monkey patch to get me going until the fix is released. I got all the other parts of my TR upgrade working so as you can imagine I am eager to fix this last bit. Would be happy to help if there's any way I can. |
@jasonfb thank you for creating https://github.com/jasonfb/TR001 to help reproduce the issue. Would it be possible for you to alter the git history so that the changes that are tied directly to reproducing the bug behavior are their own commit? It's very difficult to read through a commit that has a majority of its changes generated through Devise installation tasks. After quickly scanning through, I have some high level questions:
|
@seanpdoyle -- the whole app is to reproduce the issue. Yes I did it quickly and just made 1 commit. Oh bootstrap is in there for no reason -- I can take that out if you want.
So sorry… let me re-do, that is irrelevant now. (it was me debugging in some other way). The bug reproduces on devise directly, on the devise controller (which is not this one-- this HelloController#anything action is irrelevant, sorry). FYI..... I think may be a devise issue related to devise because it seems like they said over there this was fixed in a PR. I will try the other things you suggest. I removed the unneeded code, the |
You are a genius (although, this is still a bug). First: the devise login partials come from the devise gem itself, so without overriding it, I cannot change the links inside of the devise partials. Focusing on Symptom # 2 only (the Logout)--- your suggestion does indeed instantly restore the functionality. which is interesting, and suggests to me there is a bug related to links on the Turbo side. That is, since simply changing from As far as the rest of this, it seems like it should be addressed in devise (for one thing, if mods are to be made to the login logout form). it looks like they were discussing root cause of these issues back in January here heartcombo/devise#5325 (TBH I did not debug that part of the stack.-- so I can only speak to the part of the stack that I debugged of course but I see the thing appears to all be related to 422 status codes in Rails responses or something around this area. ) it makes sense in the sense that links are supposed to be non-destructive and buttons should do destructive things but UJS has spoiled us with method: :delete on our links. So...... at the very least maybe Turbo could give a console error ? |
Here you go ... new example app here: https://github.com/jasonfb/TR002 this is now without boostrap and without haml, so just enough to reproduce the Turbo-rails + Devise issue I have cross-poseted this to heartcombo/devise#5358 |
you can reproduce fully on your own machine using these steps https://gist.github.com/jasonfb/eb9cf8e90514dad1af0b98e01e9bce3d |
Hello everyone, I opened #152 earlier, but I think this issue is the same. It is old good HTML 1.0 and it gets broken unless you wrap it into This is not correct. Forms must work the same way as links. We don't need to opt-out from turbo on every link, so we shouldn't do it for every form. Links are smart enough to use whole page as response when not wrapped in I suggest the following logic:
We will get everything working for all existing and all non-Rails code out of the box. It will respect old good HTML basics. And it will be progressive enhancement, not a mandatory Turbo lock-in. It will be effortless magic! I think it is a more high-level problem of current implementation which will solve original redirect issue, too. It looks more server-side. Sorry if I'm missing some big idea or technical restriction, but this part is confusing. |
To add to what @dmitry-rychkov said, this would also improve the Turbolinks-to-Turbo upgrade path quite a lot. Most of it was straightforward, but having form submissions suddenly doing nothing (even without |
|
@jasonfb, my observation is that when server response doesn’t contain any When there is a I couldn’t find a line in Rails code which prevents normal HTML response when there is no frame inside. Still don’t see any reason for Turbo forms not to allow same logic as links (without any UJS) |
I ended up adding |
more than a year later and redirection still doesn't work with form when using turbo by default in rails 7, anyone got a solution? |
The solutions are in this issue. In short: use |
I had troubles with Is this documented anywhere? Couldn't find anything about the correct template naming. |
I think that if you don't have |
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. |
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 |
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 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 I don't necessarily agree with the implementation but that is the explanation. |
@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. |
FYI the haml / slim issue has already been confirmed here: #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. |
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. Media type registrationSo in the new turbo you will find this piece of code: turbo-rails/lib/turbo/engine.rb Lines 50 to 52 in 102e919
This ensures you can call 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
When a block is executed, it will by default set the content-type to whatever format was accepted. So, if the Renderer creationOften, you will not have any 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 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: turbo-rails/lib/turbo/engine.rb Lines 55 to 60 in 102e919
What this shows is: return the content to render verbatim, which means you need to do Default renderThese two things together may help you understand what happens when you do not have def show
# render
end Either explicit What happens under the hood is a chain of calls that effectively call 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 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
We can go deeper, but the important thing to understand is that a file The matched template will actually set the format (see Turbo, and why
|
@SleeplessByte thanks for the thorough explanation! |
There was a slight error in the final section which I've since fixed. TL;DR stays the same :) |
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 |
FYI - its |
@SleeplessByte - I really appreciate the thorough response but this issue is maddening. On a brand new Rails 7.0.3.1 app I used 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 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 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. |
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 |
@danielricecodes what's the file names of the views? They are probably .html.erb. What are your file names? |
@kiddrew is your @danielricecodes i just tried your scaffold example in a fresh rails 7 app. everything works. like with turbolinks. but there also is no 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 now, for a form outside of a turbo-frame, the so, there you have it. you cannot break out of a turbo-frame. which is unfortunate i think. my use case:
|
This was actually the problem. Wow 🤯 My editor was saving files with only the |
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:
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? |
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. |
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:
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. |
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.
|
@koenhandekyn What is the name of your template file that renders at |
@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: #122 (comment) |
Is anyone else experiencing an issue where they want to use |
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.
And with just a very simple form (using slim and simple_form):
My controller performs the redirect (using responders):
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.
The text was updated successfully, but these errors were encountered: