Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ETags don't take assets into account #42544

Open
georgeclaghorn opened this issue Jun 20, 2021 · 14 comments
Open

ETags don't take assets into account #42544

georgeclaghorn opened this issue Jun 20, 2021 · 14 comments
Labels
actionpack pinned

Comments

@georgeclaghorn
Copy link
Contributor

georgeclaghorn commented Jun 20, 2021 •
edited

Consider the below controller action, layout, and view:

class PostsController < ApplicationController
  def show
    @post = Post.find(params[:id])

    fresh_when @post
  end
end
<%# app/views/layouts/application.html.erb %>

<!DOCTYPE html>
<html>
  <head>
    <title>Sandbox</title>

    <%= csrf_meta_tags %>
    <%= csp_meta_tag %>

    <%= stylesheet_link_tag "application" %>
    <%= javascript_pack_tag "application" %>
  </head>

  <body>
    <%= yield %>
  </body>
</html>
<%# app/views/posts/show.html.erb %>

<h1><%= @post.title %></h1>

<%= @post.content %>

Note the use of fresh_when in PostsController#show to set an ETag response header based on @post.

  1. Visit /posts/1, assuming a Post exists with ID 1.
  2. Make and deploy a change to the application stylesheet.
  3. Return to the app. Navigate away from /posts/1 and back.

Your browser will perform a conditional GET using the ETag from the response to the request in step 1. It’ll receive a 304 Not Modified response from the app, even though the application stylesheet’s fingerprint has changed.

The browser will then request the old stylesheet. Worst case: the old stylesheet doesn’t exist anymore, and the page may be rendered unstyled. Best case: it does still exist, but it contains the old styles.

To fix, we could incorporate asset fingerprints into ETags by default. Low-touch: incorporate all asset fingerprints into ETags for all cached HTML requests. More sophisticated: only assets explicitly linked from the view.
@byroot
Copy link
Member

byroot commented Jun 24, 2021

Low-touch: incorporate all asset fingerprints into ETags for all cached HTML requests

I guess a digest of the manifest could be used as a seed. fresh_when already takes an option to digest the template that's supposed to be rendered, so it would fit.

More sophisticated: only assets explicitly linked from the view.

I don't think this one is worth it. You basically need to render the view to know what assets are linked, so you'd lose a huge part of the perf gain. You'd still save sending the body, but you would have rendered it.

@georgeclaghorn
Copy link
Contributor Author

You basically need to render the view to know what assets are linked, so you'd lose a huge part of the perf gain. You'd still save sending the body, but you would have rendered it.

Doh, of course. 🤦‍♂️

@rails-bot
Copy link

rails-bot bot commented Sep 22, 2021

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 6-1-stable branch or on main, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@rails-bot rails-bot bot added the stale label Sep 22, 2021
@intrip
Copy link
Contributor

intrip commented Sep 23, 2021

still relevant

@rails-bot rails-bot bot removed the stale label Sep 23, 2021
@rails-bot
Copy link

rails-bot bot commented Dec 22, 2021

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 6-1-stable branch or on main, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@rails-bot rails-bot bot added the stale label Dec 22, 2021
@intrip
Copy link
Contributor

intrip commented Dec 22, 2021

still relevant

@rails-bot rails-bot bot removed the stale label Dec 22, 2021
@rails-bot
Copy link

rails-bot bot commented Mar 22, 2022

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 7-0-stable branch or on main, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@rails-bot rails-bot bot added the stale label Mar 22, 2022
@intrip
Copy link
Contributor

intrip commented Mar 29, 2022

still relevant

@rails-bot rails-bot bot removed the stale label Mar 29, 2022
@rails-bot
Copy link

rails-bot bot commented Jun 27, 2022

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 7-0-stable branch or on main, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@rails-bot rails-bot bot added the stale label Jun 27, 2022
@intrip
Copy link
Contributor

intrip commented Jun 27, 2022

still relevant

@rails-bot rails-bot bot removed the stale label Jun 27, 2022
@rails-bot
Copy link

rails-bot bot commented Sep 25, 2022

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 7-0-stable branch or on main, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@rails-bot rails-bot bot added the stale label Sep 25, 2022
@intrip
Copy link
Contributor

intrip commented Sep 26, 2022

still relevant

@rails-bot rails-bot bot removed the stale label Sep 26, 2022
@rails-bot
Copy link

rails-bot bot commented Dec 25, 2022

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 7-0-stable branch or on main, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@rails-bot rails-bot bot added the stale label Dec 25, 2022
@byroot byroot added pinned and removed stale labels Dec 25, 2022
@intrip
Copy link
Contributor

intrip commented Dec 28, 2022

still relevant

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
actionpack pinned
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@byroot @georgeclaghorn @zzak @intrip