ember-cli / broccoli-asset-rev

Broccoli plugin to add fingerprint checksums and CDN URLs to your assets
MIT License
87 stars 84 forks source link

Generate CSS/JS file hash after replacing fingerprinted asset paths #118

Closed mirague closed 6 years ago

mirague commented 6 years ago

The Problem

In between builds the exact same file-hash (bazaar-31277e46c9d21028df0dfe7beb607b4e.css) is generated even though the content of the CSS file have changed due to linked assets being fingerprinted:

image

The first file is cached for a long period on our CDN causing the newly generated file to not be picked up and never reach our users. The integrity hash however is updated in between builds which is problematic as the browser will expect to receive the latest generated file but instead receives the old version:

image

This puts us in a situation where the whole website will be loaded unstyled and we have to manually go into our CDN to force purge the cache.

My theory is that the MD5 checksums for CSS files are generated -before- asset links are replaced, causing any changes to the assets to not be picked up. It would make more sense to generate the MD5 hash based on the final CSS. If the latter is intended, there is a bug.

Summary

Reproduce

cbrevdev commented 6 years ago

Just encountered the same issue after including additional extensions for fingerprinting. css contents were updated to reflect the new fingerprinted names, but the built css kept its existing fingerprinted filename. This triggered SRI failures on clients who had cached version of the css.

mirague is correct that the md5 is of the file before the links have been rewritten.

Using a customHash function we can generate the same md5 as broccoli-asset-rev: 127333fa9e6639907145f95195cabc5c tmp/fingerprint-input_base_path-GHOuRbBl.tmp/assets/vendor.css

Which does not match the final css file: MD5 (dist/assets/vendor-127333fa9e6639907145f95195cabc5c.css) = 8399b5615f8f1f4f905e6d1e929447ee

At time of customHash we see the pre-updated references: background:url(../img/sprite-skin-nice.png)

which are updated post hashing: background:url(../img/sprite-skin-nice-41732f58be91fcdc79381f239685c0e1.png)

Resulting in a file whose md5sum is different than its fingerprint.

k3n commented 6 years ago

Funny enough, I'm battling this issue currently and came here to search for an issue... turns out it's at the top of the list!

I'm really surprised this hasn't been reported before.

makepanic commented 6 years ago

We have the same issue with ember-intl translation files that get updated without changing the js file fingerprint.

20170804-110821 Left is from cached response, right is from clean response. Both have the same fingerprint.

mirague commented 6 years ago

Does anyone know a workaround solution until this issue is addressed?

I've looked at the source code to see if I can do a PR but felt overwhelmed as I'm new to Ember/Broccoli and don't have the time necessary to invest.

Update: I just see #121 was created, hopefully it gets reviewed and merged soon!

cbrevdev commented 6 years ago

Yep, there's PR #121. You can help test it via:

npm uninstall brocolli-asset-rev
npm install RevolutionDisplay/broccoli-asset-rev.git#two-stage

Recommend against letting that slip into production, as it hasn't been reviewed. All feedback welcome :)

I also updated that PR late last week to address makepanic's issue of json files not updating the fingerprint. Some of the extension checks where based on string searches, so were incorrectly treating .json as .js.

makepanic commented 6 years ago

Just want to confirm that the #two-stage branch works for us.

@cbrevdev thanks for the great work.

mirague commented 6 years ago

@makepanic Are you using this in production?

makepanic commented 6 years ago

We just integrated it into our staging builds. Looks fine so far except https://github.com/rickharrison/broccoli-asset-rev/pull/121#issuecomment-321217135

cbrevdev commented 6 years ago

@makepanic 433ced7 hopefully addresses your comment in #121. Thanks for testing!!

k3n commented 6 years ago

It will be a week or more before I can test this, but I wanted to thank you @cbrevdev for the PR!

makepanic commented 6 years ago

The fix in 433ced7 works fine without globbing. Thanks @cbrevdev

rwjblue commented 6 years ago

I believe this is fixed by #121 and released in v2.6.0.