Keep flash in Turbo Frame requests #699
Conversation
A POST request can set the flash and return a redirect response. If a Turbo-Frame request that gets made before the redirect location gets called, the flash gets discared by the Turbo-Frame request. By making sure a Turbo-Frame request keeps the flash, we can avoid this problem.
| message = Message.create! | ||
|
|
||
| post messages_path | ||
| assert_equal @request.flash[:notice], 'Message was successfully created.' | ||
|
|
||
| get messages_path, headers: { "Turbo-Frame" => "true" } | ||
| assert_equal @request.flash[:notice], 'Message was successfully created.' | ||
|
|
||
| get messages_path | ||
| assert_equal @request.flash[:notice], 'Message was successfully created.' | ||
|
|
||
| get messages_path | ||
| assert_nil @request.flash[:notice] |
There was a problem hiding this comment.
Is it necessary to call Message.create!? Is the @request instance variable accessible as request?
| message = Message.create! | |
| post messages_path | |
| assert_equal @request.flash[:notice], 'Message was successfully created.' | |
| get messages_path, headers: { "Turbo-Frame" => "true" } | |
| assert_equal @request.flash[:notice], 'Message was successfully created.' | |
| get messages_path | |
| assert_equal @request.flash[:notice], 'Message was successfully created.' | |
| get messages_path | |
| assert_nil @request.flash[:notice] | |
| post messages_path | |
| assert_equal request.flash[:notice], "Message was successfully created." | |
| get messages_path, headers: { "Turbo-Frame" => "true" } | |
| assert_equal request.flash.notice, "Message was successfully created." | |
| get messages_path | |
| assert_equal request.flash.notice, "Message was successfully created." | |
| get messages_path | |
| assert_nil request.flash.notice |
There was a problem hiding this comment.
Hmm, the message was required in my initial change as I was following the redirect to message_url(id: 1).
In the current implementation it isn't required. I think this could use a little clean up.
|
@jorgemanrubia @brunoprietog are either of you able to enable CI to execute for this PR? |
|
Thanks @jorgemanrubia and @seanpdoyle ! |
|
Just wondering, but what if a turbo frame response renders the flash messages? Would this not then keep the messages and display them again (potentially) on another subsequent unrelated request? |
|
@jorgemanrubia I have the same question as Brendon above. I now have to audit my entire application to see if this behavior has a negative impact on our notices. It was just released as a patch release, but as far as I can tell this is a potentially breaking change with a pretty good question asked not too long after it was merged. Is there a chance that this can cause breaking issues in existing applications? If so, should it be rolled back? Thanks. |
|
I think you are right, this looks like a breaking change. Sorry I missed the comment above. I think we should release a patch with the revert, and look for a non-breaking way of achieving what we want here. What do you think @p8 @seanpdoyle? |
|
I'm ok with reverting it. |
|
@jorgemanrubia Thank you for the quick response. A patch release with a revert sounds good to me. Is removing gem versions from rubygems ever done for this library? I don't know if this has a significant enough impact to warrant that, but just want to mention it in case you think it is. For what it's worth, a note in the README about flashes may be something that can be done after the reversion to let people know about the impact of turbo frame requests and flash. We have our own way of handling notices that involves rendering a turbo stream along with any response, so there are likely many ways to address this in user-space. |
This reverts commit 0e42702.
|
Thank you. |
|
Thanks @jorgemanrubia. Can't remember why I locked on to this issue well before release :D Must have seen it in the upcoming changelog. |
A POST request can set the flash and return a redirect response. If a Turbo-Frame request that gets made before the redirect location gets called, the flash gets discared by the Turbo-Frame request.
By making sure a Turbo-Frame request keeps the flash, we can avoid this problem.