-
Notifications
You must be signed in to change notification settings - Fork 319
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
Include minimal layout in frame responses #428
Conversation
While I think this is a helpful change to support ongoing Conceptually, a new and separate minimized page layout provides an additional source of deviation and drift when weighed against rendering frame responses in the exact same way as other responses. In the past, |
I think the advantage to using a minimal layout wrapper, as opposed to a full layout, is that we'd still get the benefits of the optimisation, including the reduced rendering effort. And the small responses are easy to read when debugging, which is minor, but quite handy. The extra html/head elements here don't really impact that. If there's a case for sending full-layout responses to frame requests, maybe we should provide people with an easy & obvious way to turn the optimisation off? (You can already do that by specifying an explicit layout, but we could make it more explicit). |
What do you think about implementing the reverse. Full responses by default, then a turn-key configuration to render with the optimized minimal layout? Regardless of defaults, I do agree that a well-documented configuration makes a lot of sense here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kevinmcconnell looks great 🙌
What do you think about implementing the reverse. Full responses by default, then a turn-key configuration to render with the optimized minimal layout?
@seanpdoyle I think minimal layout by default makes more sense for two reasons:
- It's closer to the original behavior, so less likely to introduce surprises when people upgrade.
- Rendering a
turbo_rails/frame
also adds an extension point without any need for extra configuration. If you'd rather render the application layout you can create anapp/views/layouts/turbo_rails/frame.html.erb
and renderapplication.html.erb
(or whatever) from there. This would not work the other way araound. Changing the minimal response is a fairly edge case, so I think it's ok for the option to be available but not very prominent. Most people won't need any changes and they'll get optimized version out of the box.
# | ||
# This is merely a rendering optimization. Everything would still work just fine if we rendered everything including the layout. | ||
# Turbo Frames knows how to fish out the relevant frame regardless. | ||
# When that header is detected by the controller, we substitute our own minimal layout in place of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about mentioning the minimal layout is turbo_rails/frame.html.erb
? People can still override the default if the want creating their own app/views/layouts/turbo_rails/frame.html.erb
file. Probably not a very common case, but still doable if someone wants to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about mentioning the minimal layout is
turbo_rails/frame.html.erb
? People can still override the default if the want creating their ownapp/views/layouts/turbo_rails/frame.html.erb
file.
I like that idea. Action Text's documentation makes a similar suggestion for folks needing to customize the default layout in https://guides.rubyonrails.org/action_text_overview.html#rendering-rich-text-content
@afcapel that's a great point!
Is the idea that you'd declare something like: <%# app/views/layouts/turbo_rails/frame.html.erb %>
<%= render layout: "application" do %>
<%= yield %>
<% end %> If that's the case, could we also introduce test coverage of some kind that exercises that behavior to make sure it's possible? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @kevinmcconnell. I think this strikes a good balance. It ensures that frame responses contain a full HTML document, but it avoids any performance penalty from rendering a potentially-expensive application layout.
I wanted to add that I agree with @seanpdoyle's arguments in #232 and elsewhere about the benefits of returning full documents instead of fragments. I think the problem of "breaking out" of frames is a perfect illustration: as soon as frame responses are documents, things become simpler. Earlier breakout proposals are often working around the fact that there was no document to configure, and the suitability of turbo-visit-control
was never apparent.
Hotwire's premise is that shipping HTML over the wire is fine, and that dropping down to fragments is an unnecessary optimization that in the worst case can lead to the invention of an ad-hoc, informally-specified, bug-ridden, slow implementation of half of a document format. Best to steer clear 😅
Thanks for the feedback @seanpdoyle, @afcapel & @packagethief! I agree that we should document the way to extend that default template. I don't think it's be something that people should need often, but we should make it clear for any cases where they do. I've added an explanation for that, and a corresponding test. |
c975ede
to
a86a4f1
Compare
Previously, responses to frame requests were rendered without any layout. Since Turbo does not need the content outside of the frame, reducing the amount that is rendered can be a useful optimisation. However, this optimisation prevents responses from specifying `head` content as well. There are cases where it would be useful for Turbo to have access to items specified in head, like the meta tags used for specifying visit and cache control. To get around this, we change the optimisation to use a minimal layout for frame responses, rather than no layout. With this, applications can render content into the `head` if they want, and the Turbo Rails helpers like `turbo_page_requires_reload` will also work.
a86a4f1
to
5614797
Compare
@kevinmcconnell Maybe I'm missing something, but is this not going to cause some scripts to be loaded twice? |
@davidalejandroaguilar I'm not sure what you mean. Can you elaborate a bit? |
@kevinmcconnell So I'm thinking that if you're showing |
@davidalejandroaguilar for frame responses, Turbo already knows how to extract the frame content from the response, and only include the relevant portion on the page. It doesn’t depend on that having been stripped out by the server. In the same way, you can use your full layout on your frame responses if you want to. The fact that turbo rails renders with less/no layout is an optimisation to avoid rendering and shipping content that won’t be used. It’s not a necessary part of frames working correctly. |
@kevinmcconnell Thanks, I do understand that. Maybe I'm not understanding this PR. I thought this PR would cause Turbo to start including But I think I see now that the overall "ignore-everything-outside-the-turbo-frame" behavior is done on Turbo Javascript and will still keep this behavior. This PR will just add the head tag so that Turbo Javascript can pick specific information encoded there to alter some behavior, if needed. Did I get that right? |
Exactly @davidalejandroaguilar 👍 |
After the changes made in [@hotwired/turbohotwired#867][] and changes made in [@hotwired/turbo-railshotwired#428][] (the canonical server-side implementation), Turbo expects full HTML documents in response to requests with `Turbo-Frame:` headers. Prior to this commit, the `FrameController` compensated for missing pieces of an HTML document by taking an HTML "snapshot" of the current page through the `<html>` element's [outerHTML][]. This commit changes the `fetchResponseLoaded` callback to read the `responseHTML` directly from the `FetchResponse`, since that will be a fully formed HTML document in Turbo v7.3.0 and later. To support that change, this commit also updates various `src/test/fixtures` files to render fully-formed HTML documents. [@hotwired/turbohotwired#867]: hotwired#867 [@hotwired/turbo-railshotwired#428]: hotwired/turbo-rails#428 [outerHTML]: https://developer.mozilla.org/en-US/docs/Web/API/Element/outerHTML
After the changes made in [@hotwired/turbohotwired#867][] and changes made in [@hotwired/turbo-railshotwired#428][] (the canonical server-side implementation), Turbo expects full HTML documents in response to requests with `Turbo-Frame:` headers. Prior to this commit, the `FrameController` compensated for missing pieces of an HTML document by taking an HTML "snapshot" of the current page through the `<html>` element's [outerHTML][]. This commit changes the `fetchResponseLoaded` callback to read the `responseHTML` directly from the `FetchResponse`, since that will be a fully formed HTML document in Turbo v7.3.0 and later. To support that change, this commit also updates various `src/test/fixtures` files to render fully-formed HTML documents. [@hotwired/turbohotwired#867]: hotwired#867 [@hotwired/turbo-railshotwired#428]: hotwired/turbo-rails#428 [outerHTML]: https://developer.mozilla.org/en-US/docs/Web/API/Element/outerHTML
After the changes made in [@hotwired/turbohotwired#867][] and changes made in [@hotwired/turbo-railshotwired#428][] (the canonical server-side implementation), Turbo expects full HTML documents in response to requests with `Turbo-Frame:` headers. Prior to this commit, the `FrameController` compensated for missing pieces of an HTML document by taking an HTML "snapshot" of the current page through the `<html>` element's [outerHTML][]. This commit changes the `fetchResponseLoaded` callback to read the `responseHTML` directly from the `FetchResponse`, since that will be a fully formed HTML document in Turbo v7.3.0 and later. To support that change, this commit also updates various `src/test/fixtures` files to render fully-formed HTML documents. [@hotwired/turbohotwired#867]: hotwired#867 [@hotwired/turbo-railshotwired#428]: hotwired/turbo-rails#428 [outerHTML]: https://developer.mozilla.org/en-US/docs/Web/API/Element/outerHTML
Hi, Does something else need to change on the Turbo side to take advantage of this behavior? For example with Turbo Rails 1.4.0 and Turbo 7.3.0, the following doesn't update:
<head>
<title><%= yield :title %></title>
</head>
<%
content_for(:title) { 'Something cool' }
%> If you hard reload There is an open issue from last year on Turbo at hotwired/turbo#600, I was hoping this PR may have fixed it but I tested it in an app I'm building and the title along with other |
Hi @nickjj this change doesn't affect how Turbo processes frame responses. Turbo still excludes everything outside of the frame itself, except for specific items like Turbo-specific meta tags that it checks for. So although this change sounds a bit like what you're describing, it's not really the same thing, unfortunately. |
There's something anti-pattern i must be doing, but this release broke every regular GET link I have For example GET /users to GET /users/:id
I guess because of this frame in the layout? Don't really understand how now rendering the layout would still cause that error message <%# app/views/layouts/application.html.erb" %>
<body>
<%= yield %>
<%# render "application/current_user" %>
<%= turbo_frame_tag :current_user do %>
<% end %>
</body> |
This suggests the request was made from within a |
Thanks for your work on this with commits and merging it in @seanpdoyle @kevinmcconnell I really wish I could update the Seems like a few others were expecting this too: |
Hi everyone, I want to express my gratitude for the hard work you've put into improving the turbo-rails gem. I'd like to share some feedback regarding this pull request and @turgs' comment: We integrated turbo-rails into our SaaS project to enhance page loading speed and ensure a faster interface, leveraging turbo frames. Example: In our "companies" controller, specifically in the transition between companies#show and companies#edit, we added a turbo_frame with the "advance" action, as suggested by @turgs. While implementing this, we encountered the same issue as @turgs. The tag was not being updated, preventing us from modifying certain elements like the <title> tag. While this wasn't a critical problem for us, as our clients would not pay attention to the <title> tag. Everything worked flawlessly until we deployed the changes to the staging environment. 🦸 In companies#edit, we have a form that includes a "file_field" input for file uploads. Under normal circumstances, there were no issues—except when we utilized the clever solution (On a humorous note, I primarily use Chrome, which shielded me from this reality. 😂) Chrome employs caching to store user sessions, so when we had the CSRF token within the tag using However, this led to a problem for users who prefer Safari. When simulating the same scenario with Safari, the file_field would block the file upload process, and the form displayed the error message: "can't verify CSRF token authenticity." After an entire day of patience and countless cups of coffee, I believe I've grasped the problem and stumbled upon this pull request. Despite testing the minimal layout and failing to achieve the desired outcome, I reached the same conclusion as @turgs and others. It would be fantastic if there was a way to update a section of the tag using the turbo frame and the "advance" action. I hope my experience helps others avoid similar mistakes. Currently, there are three solutions to address the Safari issue:
Thank you all once again for your efforts in making turbo-rails better. Best regards, |
After the changes made in [@hotwired/turbohotwired#867][] and changes made in [@hotwired/turbo-railshotwired#428][] (the canonical server-side implementation), Turbo expects full HTML documents in response to requests with `Turbo-Frame:` headers. Prior to this commit, the `FrameController` compensated for missing pieces of an HTML document by taking an HTML "snapshot" of the current page through the `<html>` element's [outerHTML][]. This commit changes the `fetchResponseLoaded` callback to read the `responseHTML` directly from the `FetchResponse`, since that will be a fully formed HTML document in Turbo v7.3.0 and later. To support that change, this commit also updates various `src/test/fixtures` files to render fully-formed HTML documents. [@hotwired/turbohotwired#867]: hotwired#867 [@hotwired/turbo-railshotwired#428]: hotwired/turbo-rails#428 [outerHTML]: https://developer.mozilla.org/en-US/docs/Web/API/Element/outerHTML
After the changes made in [@hotwired/turbohotwired#867][] and changes made in [@hotwired/turbo-railshotwired#428][] (the canonical server-side implementation), Turbo expects full HTML documents in response to requests with `Turbo-Frame:` headers. Prior to this commit, the `FrameController` compensated for missing pieces of an HTML document by taking an HTML "snapshot" of the current page through the `<html>` element's [outerHTML][]. This commit changes the `fetchResponseLoaded` callback to read the `responseHTML` directly from the `FetchResponse`, since that will be a fully formed HTML document in Turbo v7.3.0 and later. To support that change, this commit also updates various `src/test/fixtures` files to render fully-formed HTML documents. [@hotwired/turbohotwired#867]: hotwired#867 [@hotwired/turbo-railshotwired#428]: hotwired/turbo-rails#428 [outerHTML]: https://developer.mozilla.org/en-US/docs/Web/API/Element/outerHTML
After the changes made in [@hotwired/turbohotwired#867][] and changes made in [@hotwired/turbo-railshotwired#428][] (the canonical server-side implementation), Turbo expects full HTML documents in response to requests with `Turbo-Frame:` headers. Prior to this commit, the `FrameController` compensated for missing pieces of an HTML document by taking an HTML "snapshot" of the current page through the `<html>` element's [outerHTML][]. This commit changes the `fetchResponseLoaded` callback to read the `responseHTML` directly from the `FetchResponse`, since that will be a fully formed HTML document in Turbo v7.3.0 and later. To support that change, this commit also updates various `src/test/fixtures` files to render fully-formed HTML documents. [@hotwired/turbohotwired#867]: hotwired#867 [@hotwired/turbo-railshotwired#428]: hotwired/turbo-rails#428 [outerHTML]: https://developer.mozilla.org/en-US/docs/Web/API/Element/outerHTML
@nickjj Yea it looks like you need to use Turbo Drive for this. We had disabled it by default and were hoping the same |
@sc0ttman I am using drive for most things but I have a very heavy sidebar with unique user content that's not really cache'able that I wanted to avoid re-rendering by loading the main content area in with a frame when sidebar links were clicked. I hope one day they allow this behavior with frames, it's a really common feature in my opinion to be able to update the page's title or meta tags while still using frames without having to resort to a custom hand rolled Javascript approach. |
Previously, responses to frame requests were rendered without any layout. Since Turbo does not need the content outside of the frame, reducing the amount that is rendered can be a useful optimisation.
However, this optimisation prevents responses from specifying
head
content as well. There are cases where it would be useful for Turbo to have access to items specified in head, like the meta tags used for specifying visit and cache control.To get around this, we change the optimisation to use a minimal layout for frame responses, rather than no layout. With this, applications can render content into the
head
if they want, and the Turbo Rails helpers liketurbo_page_requires_reload
will also work.