-
Notifications
You must be signed in to change notification settings - Fork 322
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
"Break out" of a frame from the server #367
base: main
Are you sure you want to change the base?
Conversation
888b0a6
to
3f66023
Compare
3f66023
to
d8e85d6
Compare
The suite passes locally when executing |
6d69e39
to
5b3ed2d
Compare
I am not responsible for Devise, but wanted to give this a big thumbs up, as imho this PR would help Devise support Turbo a bit better. There is a pattern that I feel a lot of apps use which is to have some sort of If that link is currently inside a WIth this PR (and maybe an updated PR in responders/devise), forcing the turbo_frame: '_top' makes the user behavior closely match our pre-turbo world. |
6ab111c
to
29e1101
Compare
1d98a5d
to
d0596ce
Compare
@seanpdoyle this is very helpful, and much needed. Thank you! |
@tomasc thank you for raising that concern! I've opened hotwired/turbo#694 to make the Turbo Action available to the server as a request header. If that lands, this implementation could incorporate that value into the Stream's |
Thanks @seanpdoyle ! Hope it gets merged soon, I could use this on a project right now ;-). |
cbe2e96
to
8d8b3f2
Compare
I think hotwired/turbo#863 shouldn't have been merged without merging this PR first. People were relying on the existing behavior to break out of frames. Luckily there's an escape hatch. But it'd be great to have this so that we can remove it and control it ourselves more granularly. It is especially useful when you want to conditionally break out from a frame, depending on arbitrary server state. |
Having watched this PR and concept for a pretty long time, I wonder if this can actually be closed now. It seems like hotwired/turbo#863 and hotwired/turbo#867 together (with the escape hatch noted above and a couple other details to be aware of) should satisfy the needs for an app to 'break out' of a frame from either front-end controls or server responses. |
@jon-sully This seems to offer a lot more functionality than either hotwired/turbo#863 or hotwired/turbo#867. It is also a much cleaner and easier-to-use syntax, allowing "redirect_to" to actually perform a full-page redirect without having to add additional content on the target page (like a |
Closes hotwired/turbo#257 Closes hotwired/turbo#397 Follow-up to: * hotwired/turbo#257 (comment) * hotwired/turbo#257 (comment) Depends on hotwired/turbo#660 Introduces the `Turbo::Stream::Redirect` concern to override the [redirect_to][] routing helper. When called with a `turbo_frame:` option, the `redirect_to` helper with check whether the request was made with the Turbo Stream `Accept:` header. When it's absent, the response will redirect with a typical HTTP status code and location. When present, the controller will respond with a `<turbo-stream>` element that invokes [Turbo.visit($URL, { frame: $TURBO_FRAME })][hotwired/turbo#649] where `$URL` is set to the redirect's path or URL, and `$TURBO_FRAME` is set to the `turbo_frame:` argument. This enables server-side actions to navigate the entire page with a `turbo_frame: "_top"` option. Incidentally, it also enables a frame request to navigate _a different_ frame. Typically, an HTTP that would result in a redirect nets two requests: the first submission, then the subsequent GET request to follow the redirect. In the case of a "break out", the same number of requests are made: the first submission, then the subsequent GET made by the `Turbo.visit` call. Once the `Turbo.visit` call is made, the script removes its ancestor `<script>` by calling [document.currentScript.remove()][], and marking it with [data-turbo-cache="false"][] [redirect_to]: https://edgeapi.rubyonrails.org/classes/ActionController/Redirecting.html#method-i-redirect_to [hotwired/turbo#649]: hotwired/turbo#649 [document.currentScript.remove()]: https://developer.mozilla.org/en-US/docs/Web/API/Document/currentScript [data-turbo-cache="false"]: https://turbo.hotwired.dev/reference/attributes#data-attributes
a4d84ad
to
0cf26aa
Compare
@dhh Hoping to see this merged soon! 😃 |
current work around for people that waiting this to be merged this is how you could redirecting and rendering notice from a turbo frame
|
I think there are too many moving parts in this setup at the moment. I think the underlying desire to be able to break out from the server side is right, but let's see if we can't find a path that uses the new turbo-visit-control setup for frames: hotwired/turbo#867. |
I'm really looking forward to being able to do: |
This part is true out of the box, but FWIW we can hook into the JS event and traverse the response directly without another request. I outlined that here but the document.addEventListener("turbo:frame-missing", event => {
event.preventDefault()
event.detail.visit(event.detail.response)
}) And, while I think this is a common use-case (we needed this too), it's just one of many to consider when thinking through the back-end changes for supporting back-end-driven, explicit, frame navigations. In that sense, it's not like any other case of "turbo_frame: top". I'd be curious to see a solution that does provide some kind of helper to essentially automated the |
Please do explore both approaches. What I'm saying is that the implementation required in this PR is too heavy and cumbersome. I appreciate the effort, but let's try some alternate angles on the same problem. Feel free to CC me on additional attempts. We'll eventually crack this with a simple solution! |
I think @jon-sully's idea would make a lot of sense here, of using a helper to bring the information across from the The intention of hotwired/turbo#867 was to give an easy escape hatch for pages that should never be in a frame, but which should instead always be full-page loads. That's a common enough situation when dealing with things like login pages that it needs an easy solution, and we wanted a way to state that on the page itself rather than have to accomodate it in the controllers. It should feel like configuration. That, and the fact that we weren't respecting But if there is a need to conditionally turn a frame request into a full-page navigation, we could consider adding a frame header to the redirect response to indicate that. On the Turbo side, when a turbo-frame receives a redirect with that header, it can follow it using a I think that will be more straightforward than the method proposed here. And also would make it available to people using Turbo outside of Rails -- we can include a helper here to make it easy to include that header from the Also, as an aside -- for anything more involved than turning a frame request into navigation, I think it's worth leaning on the existing abilities of Turbo Streams rather than making Turbo Frames more flexible with which parts of the page it updates. Streams already provides a way to target arbitrary areas of the page, so if you just need to have one frame update another, you can already do that with Streams. Making Frames any more "server-directed" would mean more overlap between Frames and Streams, and I think it will start to introduce complexity that we don't need. (Which is not to say that Streams is perfect either, of course. But just that if we need more flexibility in triggering updates outside of the calling frame, I think it's the right place to start). |
After reading through the source of this PR and thinking more about the problem...I think the solution to this is simpler than I realized. I think the idea behind this PR is correct (respond with some javascript to perform a For those who need a short-term fix for this problem, here is a helper method you can add to your def turbo_visit(url, frame: nil, action: nil)
options = {frame: frame, action: action}.compact
turbo_stream.append_all("head") do
helpers.javascript_tag(<<~SCRIPT.strip, nonce: true, data: {turbo_cache: false})
window.Turbo.visit("#{helpers.escape_javascript(url)}", #{options.to_json})
document.currentScript.remove()
SCRIPT
end
end
# then you use it like this:
def create
@article = Article.new article_params
if @article.save
render turbo_stream: turbo_visit(articles_url)
else
render :new, status: :unprocessable_entity
end
end As far as overriding def redirect_to(url_options = {}, response_options = {})
turbo_frame = response_options.delete(:turbo_frame)
turbo_action = response_options.delete(:turbo_action)
return super unless request.format.turbo_stream? && turbo_frame.present?
location = url_for(url_options)
response_options.slice(:flash, :notice, :alert).each do |key, value|
(key == :flash) ? flash.update(value) : flash[key] = value
end
render turbo_stream: turbo_visit(location, frame: turbo_frame, action: turbo_action)
end But we would probably need something more robust, like this (which copies some stuff from this PR and the current implementation of def redirect_to(options = {}, response_options = {})
turbo_frame = response_options.delete(:turbo_frame)
turbo_action = response_options.delete(:turbo_action)
return super unless request.format.turbo_stream? && turbo_frame.present?
allow_other_host = response_options.delete(:allow_other_host) { _allow_other_host }
location = _enforce_open_redirect_protection(_compute_redirect_to_location(request, options), allow_other_host: allow_other_host)
self.class._flash_types.each do |flash_type|
if type = response_options.delete(flash_type)
flash[flash_type] = type
end
end
if other_flashes = response_options.delete(:flash)
flash.update(other_flashes)
end
render turbo_stream: turbo_visit(location, frame: turbo_frame, action: turbo_action)
end |
This is code pulled from PR hotwired/turbo-rails#367
Wondering what the status is on this one? I tried it in a test project and it worked wonderfully and I haven't seen anything else as elegant as adding |
looks great, looking forward to it being merged! My current solution: // app/javascript/application.js
Turbo.StreamActions.redirect = function () {
Turbo.visit(this.target);
}; # my_controller.rb
render turbo_stream: turbo_stream.action(:redirect, posts_path) |
@yshmarov's solution looks super clean! |
@yshmarov Have you thought of a way to combine your solution to also have an alert/notice? |
@yshmarov That's pretty nice. |
@yshmarov If you want to keep scroll, add Turbo.visit(url, { action: "replace" }) |
@kevinmcconnell in response to #367 (comment), I've tried my best to distill a common use case I've encountered into a self-contained application. Boiled down, the scenario entails the following:
I've created a single-file application to make this scenario more concrete. You can copy-paste the following into a It utilizes the It "works", in that it meets the acceptance criteria outlined above without introducing new abstractions or Turbo mechanisms. However, the resulting There are three System Tests covering the behavior. If you'd like to experiment with it locally, change the
I've explored this, and haven't found a way to make a server-set HTTP header available to Turbo when the resulting redirect occurs. I believe Turbo's use of
Is there an architectural change to be made to Turbo to improve support for this style of scenario? If there isn't a way to support this directly, are there any examples of concrete changes to the example application below that you'd make to flesh out Turbo Stream-powered solutions? require "bundler/inline"
gemfile(true) do
source "https://rubygems.org"
git_source(:github) { |repo| "https://github.com/#{repo}.git" }
gem "rails"
gem "propshaft"
gem "puma"
gem "sqlite3"
gem "turbo-rails"
gem "capybara"
gem "cuprite", "~> 0.9", require: "capybara/cuprite"
end
ENV["DATABASE_URL"] = "sqlite3::memory:"
ENV["RAILS_ENV"] = "test"
require "active_record/railtie"
require "action_controller/railtie"
require "action_view/railtie"
require "rails/test_unit/railtie"
class App < Rails::Application
config.load_defaults Rails::VERSION::STRING.to_f
config.root = __dir__
config.eager_load = false
config.secret_key_base = "secret_key_base"
config.consider_all_requests_local = true
config.turbo.draw_routes = false
routes.append do
resources :todos, only: [:new, :create]
root to: "todos#index"
end
end
Rails.application.initialize!
ActiveRecord::Schema.define do
create_table :todos, force: true do |t|
t.text :body, null: false
end
end
class Todo < ActiveRecord::Base
validates :body, presence: true
end
class TodosController < ActionController::Base
include Rails.application.routes.url_helpers
class_attribute :template, default: DATA.read
def index
@todos = Todo.all
render inline: template, formats: :html
end
def new
@todo = Todo.new
render_new_template
end
def create
@todo = Todo.new(params.require(:todo).permit(:body))
if @todo.save
flash.notice = "Todo created"
redirect_to root_url(turbo_visit_control: "reload")
else
render_new_template status: :unprocessable_entity
end
end
helper_method def breaking_out_of_turbo_frame?
turbo_frame_request? && params[:turbo_visit_control] == "reload"
end
before_action -> { flash.keep }, if: :breaking_out_of_turbo_frame?
private
def render_new_template(**)
render formats: :html, inline: <<~HTML, **
<turbo-frame id="new_todo">
<%= "Failed to create Todo" if @todo.errors.any? %>
<%= form_with model: @todo do |form| %>
<%= form.label :body %>
<%= form.text_field :body %>
<%= form.button %>
<% end %>
<%= link_to "Cancel", root_path %>
</turbo-frame>
HTML
end
end
class ApplicationSystemTestCase < ActionDispatch::SystemTestCase
driven_by :cuprite, using: :chrome, screen_size: [1400, 1400], options: {js_errors: true, headless: true}
end
require "rails/test_help"
class TurboSystemTest < ApplicationSystemTestCase
test "navigates frame to form and back" do
visit root_path
fill_in "Search", with: "a search term"
click_link "New Todo"
assert_field "Search", with: "a search term"
assert_field "Body"
click_link "Cancel"
assert_field "Search", with: "a search term"
assert_no_field "Body"
assert_no_text "Todo created"
end
test "re-renders the form with validation messages within the frame" do
visit root_path
fill_in "Search", with: "a search term"
click_link "New Todo"
fill_in "Body", with: " "
click_button "Create Todo"
assert_field "Body", with: " "
assert_text "Failed to create Todo"
assert_no_text "Todo created"
assert_no_css "p"
end
test "breaks out of frame on successful creation" do
visit root_path
fill_in "Search", with: "a search term"
click_link "New Todo"
fill_in "Body", with: "Hello, world"
click_button "Create Todo"
assert_css "p", text: "Hello, world"
assert_text "Todo created"
assert_field "Search", with: ""
assert_no_field "Body"
end
end
__END__
<!DOCTYPE html>
<html>
<head>
<%= csrf_meta_tags %>
<script type="importmap">
{
"imports": {
"@hotwired/turbo-rails": "<%= asset_path("turbo.js") %>"
}
}
</script>
<script type="module">
import "@hotwired/turbo-rails"
</script>
<%= turbo_page_requires_reload_tag if breaking_out_of_turbo_frame? %>
</head>
<body>
<%= flash.notice if flash.notice.present? %>
<label>Search to demonstrate page state being preserved <input></label>
<turbo-frame id="new_todo">
<%= link_to "New Todo", new_todo_path %>
</turbo-frame>
<% @todos.each do |todo| %>
<p><%= todo.body %></p>
<% end %>
</body>
</html> |
Incorporating @yshmarov suggestion from (#367 (comment)) into the requires the following changes shared below. diff --git a/app.rb b/app.rb
index aad947f..6dbadfb 100644
--- a/app.rb
+++ b/app.rb
@@ -71,17 +71,17 @@ class TodosController < ActionController::Base
@todo = Todo.new(params.require(:todo).permit(:body))
if @todo.save
flash.notice = "Todo created"
- redirect_to root_url(turbo_visit_control: "reload")
+ respond_to do |format|
+ format.html { redirect_to root_url }
+ format.turbo_stream { render turbo_stream: turbo_stream.action(:visit, root_url) }
+ end
else
render_new_template status: :unprocessable_entity
end
end
- helper_method def breaking_out_of_turbo_frame?
- turbo_frame_request? && params[:turbo_visit_control] == "reload"
- end
- before_action -> { flash.keep }, if: :breaking_out_of_turbo_frame?
-
private
def render_new_template(**)
@@ -166,10 +166,11 @@ __END__
</script>
<script type="module">
- import "@hotwired/turbo-rails"
+ import { Turbo } from "@hotwired/turbo-rails"
+ Turbo.StreamActions.visit = function () {
+ Turbo.visit(this.target)
+ }
</script>
-
- <%= turbo_page_requires_reload_tag if breaking_out_of_turbo_frame? %>
</head>
<body> While it's a suitable workaround given the constraints, and behaves the way it needs to, I dislike that it mixes HTML and Turbo Stream content types. Through that lens, I'm similarly dissatisfied with the original approach proposed by this PR's changeset. What I've come to appreciate about the Copy-paste the following into a app.rb file, then execute it with ruby app.rb.require "bundler/inline"
gemfile(true) do
source "https://rubygems.org"
git_source(:github) { |repo| "https://github.com/#{repo}.git" }
gem "rails"
gem "propshaft"
gem "puma"
gem "sqlite3"
gem "turbo-rails"
gem "capybara"
gem "cuprite", "~> 0.9", require: "capybara/cuprite"
end
ENV["DATABASE_URL"] = "sqlite3::memory:"
ENV["RAILS_ENV"] = "test"
require "active_record/railtie"
require "action_controller/railtie"
require "action_view/railtie"
require "rails/test_unit/railtie"
class App < Rails::Application
config.load_defaults Rails::VERSION::STRING.to_f
config.root = __dir__
config.eager_load = false
config.secret_key_base = "secret_key_base"
config.consider_all_requests_local = true
config.turbo.draw_routes = false
routes.append do
resources :todos, only: [:new, :create]
root to: "todos#index"
end
end
Rails.application.initialize!
ActiveRecord::Schema.define do
create_table :todos, force: true do |t|
t.text :body, null: false
end
end
class Todo < ActiveRecord::Base
validates :body, presence: true
end
class TodosController < ActionController::Base
include Rails.application.routes.url_helpers
class_attribute :template, default: DATA.read
def index
@todos = Todo.all
render inline: template, formats: :html
end
def new
@todo = Todo.new
render_new_template
end
def create
@todo = Todo.new(params.require(:todo).permit(:body))
if @todo.save
flash.notice = "Todo created"
respond_to do |format|
format.html { redirect_to root_url }
format.turbo_stream { render turbo_stream: turbo_stream.action(:visit, root_url) }
end
else
render_new_template status: :unprocessable_entity
end
end
private
def render_new_template(**)
render formats: :html, inline: <<~HTML, **
<turbo-frame id="new_todo">
<%= "Failed to create Todo" if @todo.errors.any? %>
<%= form_with model: @todo do |form| %>
<%= form.label :body %>
<%= form.text_field :body %>
<%= form.button %>
<% end %>
<%= link_to "Cancel", root_path %>
</turbo-frame>
HTML
end
end
class ApplicationSystemTestCase < ActionDispatch::SystemTestCase
driven_by :cuprite, using: :chrome, screen_size: [1400, 1400], options: {js_errors: true, headless: true}
end
require "rails/test_help"
class TurboSystemTest < ApplicationSystemTestCase
test "navigates frame to form and back" do
visit root_path
fill_in "Search", with: "a search term"
click_link "New Todo"
assert_field "Search", with: "a search term"
assert_field "Body"
click_link "Cancel"
assert_field "Search", with: "a search term"
assert_no_field "Body"
assert_no_text "Todo created"
end
test "re-renders the form with validation messages within the frame" do
visit root_path
fill_in "Search", with: "a search term"
click_link "New Todo"
fill_in "Body", with: " "
click_button "Create Todo"
assert_field "Body", with: " "
assert_text "Failed to create Todo"
assert_no_text "Todo created"
assert_no_css "p"
end
test "breaks out of frame on successful creation" do
visit root_path
fill_in "Search", with: "a search term"
click_link "New Todo"
fill_in "Body", with: "Hello, world"
click_button "Create Todo"
assert_css "p", text: "Hello, world"
assert_text "Todo created"
assert_field "Search", with: ""
assert_no_field "Body"
end
end
__END__
<!DOCTYPE html>
<html>
<head>
<%= csrf_meta_tags %>
<script type="importmap">
{
"imports": {
"@hotwired/turbo-rails": "<%= asset_path("turbo.js") %>"
}
}
</script>
<script type="module">
import { Turbo } from "@hotwired/turbo-rails"
Turbo.StreamActions.visit = function () {
Turbo.visit(this.target)
}
</script>
</head>
<body>
<%= flash.notice if flash.notice.present? %>
<label>Search to demonstrate page state being preserved <input></label>
<turbo-frame id="new_todo">
<%= link_to "New Todo", new_todo_path %>
</turbo-frame>
<% @todos.each do |todo| %>
<p><%= todo.body %></p>
<% end %>
</body>
</html> |
There are two semantically meaningful HTTP status codes that might be worth considering as special-case escape hatches to "break out" of Turbo Frame requests from the server. There is 201 Created:
This means that a server like Rails could control a frame with a status code through something like head: def create
@todo = Todo.new(todo_params)
if @todo.save
if turbo_frame_request?
head :created, location: @todo
else
redirect_to @todo
end
else
render :new, status: :unprocessable_entity
end
end Then the Turbo Frame Controller could special case responses with There is also 205 Reset Content:
This could communicate to Turbo that it should "reset" the view, similar to a Page Refresh. The downside here is that it doesn't have the |
@yshmarov Perfect, thanks. |
Now UX is to click a "New post" button that will render the post form dynamically in a target turbo frame. The Stimulus refresh controller is renamed as it now needs to handle a special redirect use case; on successful submission of the POST from the "examples_post_form" we want to render an updated list in the "examples_posts" turbo frame. The framework does not yet provide an intuitive mechanism for breaking out of a frame on redirect, so we use a Stimulus Turbo.visit to the "examples_post" form here. This does result in a "double GET" request, the first results in re-rendering the requesting "examples_post_form", but the second is needed to re-render the "examples_posts" frame. There are other approaches, described in the resources below, which also lay out the problem in more detail: - https://www.ducktypelabs.com/turbo-break-out-and-redirect/ - hotwired/turbo-rails#367 (comment) - https://discuss.hotwired.dev/t/break-out-of-a-frame-during-form-redirect/1562/26
Closes hotwired/turbo#257
Closes hotwired/turbo#397
Follow-up to hotwired/turbo#257 (comment), hotwired/turbo#257 (comment)
Depends on hotwired/turbo#660
Introduces the
Turbo::Stream::Redirect
concern to override theredirect_to routing helper.
When called with a
turbo_frame:
option, theredirect_to
helper withcheck whether the request was made with the Turbo Stream
Accept:
header. When it's absent, the response will redirect with a typical HTTP
status code and location. When present, the controller will respond with
a
<turbo-stream>
element that invokes Turbo.visit($URL, { frame:$TURBO_FRAME }) where
$URL
is set to theredirect's path or URL, and
$TURBO_FRAME
is set to theturbo_frame:
argument.
This enables server-side actions to navigate the entire page with a
turbo_frame: "_top"
option. Incidentally, it also enables a framerequest to navigate a different frame.
Typically, an HTTP that would result in a redirect nets two requests:
the first submission, then the subsequent GET request to follow the
redirect.
In the case of a "break out", the same number of requests are made: the
first submission, then the subsequent GET made by the
Turbo.visit
call.
Once the
Turbo.visit
call is made, the script removes its ancestor<script>
by calling document.currentScript.remove().