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.

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

Asset digest is computed before compilation #123

Closed
grncdr opened this issue Dec 20, 2022 · 7 comments
Closed

Asset digest is computed before compilation #123

grncdr opened this issue Dec 20, 2022 · 7 comments

Comments

@grncdr
Copy link

grncdr commented Dec 20, 2022

This is the underlying cause of #90 but the description and comments there conflate a number of issues.

Steps to reproduce

Create two CSS files:

/* application.css */
import('other.css');
/* other.css */
body {
  background-color: pink;
}

Load application.css:

<%= stylesheet_link_tag 'application' %>

Load the page (with browser caching enabled) and observe a pink background.

Change the body background in other.css:

-  background-color: pink;
+  background-color: red;

Reload the page.

Expected result

The background is now red.

Actual result

The background is still pink.

Why this happens

The digest for application.css is computed from the unprocessed file content. This causes Propshaft::Asset to produce the same digest for the asset even though the actual contents at that URL will be different (due to the changing digest of other.css).

Because the asset is served with a very long expiry, browsers will hang on to the outdated version of the asset for a very long time.

Why that's a problem

In a production scenario, browsers would never get the new version of other.css. This makes using @import a pretty big foot gun.

@grncdr
Copy link
Author

grncdr commented Feb 23, 2023

Just wanted to come back to this issue and confirm that it definitely affects production deployments, because the browser will use a cached version of the stylesheet with older content hashes for the imported files.

We are currently working around the issue by adding a cache-busting comment to the "root" stylesheet before running assets:precompile in CI, but a more robust solution from propshaft would be very appreciated.

@dhh
Copy link
Member

dhh commented Mar 4, 2023

@brenogazzola We'd need to introduce two digests to deal with this, yeah? Basically a raw source digest and a processed source digest. And then keep those straight for when to use.

@brenogazzola
Copy link
Collaborator

brenogazzola commented Mar 6, 2023

@dhh I think it's a bit more complicated than that? What about multiple levels of import?

/* application.css */
import('elements.css');
/* elements.css */
import('avatar');
import('button');
import('form');
/* buttons.css */
.button {
  background-color: red;
}

If I understand your comment, in this applementation of raw/processed digests, a change in red would cause the raw of buttons to change, but not of elements and application. Then the processed of buttons would change (due to the change in its file) and the processed of elements would change (due to the adjusted import url), but the processed of application would not (since none of its imports had changes in the raw digest)?

I can think of two options:
1 - Keep digesting/redigesting until no digests change
2 - Teach propshaft digest/compile things in order

@grncdr
Copy link
Author

grncdr commented Mar 7, 2023

but the processed of application would not (since none of its imports had changes in the raw digest)?

The processed digest of application should depend on the processed digests of its imports, not the raw digests. I guess this is essentially your second option "teach propshaft to compile things in order". (As a bonus, propshaft needs to error on circular dependencies in order for this to be reliable).

@tekin
Copy link

tekin commented Aug 27, 2023

Just to chime in here with additional context/colour on this issue, we migrated to using propshaft on our app for a period but made the decision to revert back to sprockets because of this lack of dependency-resolution in the digest calculation: it caught us out more than once that changes in an imported CSS file was not resulting in a change in the digest.

@brenogazzola
Copy link
Collaborator

I wasn’t aware sprockets could handle that. I’ll take a look at its source code to see it I can do something similar here. Thanks

@dhh
Copy link
Member

dhh commented May 18, 2024

Fixed by #188

@dhh dhh closed this as completed May 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants