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

Do not fingerprint if filename contains a valid digest #714

Merged
merged 1 commit into from
Sep 12, 2021

Conversation

brenogazzola
Copy link
Contributor

As discussed in rails/jsbundling-rails#15, this PR prevents Sprockets from fingerprinting an asset that already contains a valid digest in its name.

It does that by changing digest_path in Asset to return logical_path if @name contains a sequence of characters whose unpacked bytesize matches that of one of the valid digest classes

@brenogazzola brenogazzola force-pushed the skip-digested-files branch 3 times, most recently from 1283b98 to 05e893d Compare September 11, 2021 00:47
@brenogazzola brenogazzola changed the title Do not generate digests if filename contains a digest Do not fingerprint if filename contains a valid digest Sep 11, 2021
@dhh
Copy link
Member

dhh commented Sep 11, 2021

This is great. How convenient that we already had all the machinery to detect a valid hash!

@brenogazzola
Copy link
Contributor Author

Changelog added. I also had to change the regex because it was not recognizing .chunk.js files which the bundlers like to generate

@brenogazzola
Copy link
Contributor Author

brenogazzola commented Sep 11, 2021

In the end it was not necessary to consider .chunk since we can control it from jsbundling side and keep the code cleaner.

@dhh
Copy link
Member

dhh commented Sep 27, 2021

@rafaelfranca Think we could cut a point release of sprockets for this?

@tsrivishnu
Copy link

We are migrating from Webpacker to jsbundling-rails. Currently, this is the only thing that is keeping us from going to production with jsbundling-rails.
@rafaelfranca consider making a release of sprockets with this.

@Vannsl
Copy link

Vannsl commented Dec 13, 2021

Great work, thank you! Is there an update on the release? We need this package for migration. @rafaelfranca @brenogazzola @dhh

@dhh
Copy link
Member

dhh commented Dec 13, 2021

You don't need anyone's release or permission to use this code! Just bundle directly: gem "sprockets", github: "rails/sprockets", ref: "cddf9fb841eece80276a1ccaee1e018a356547a0".

@tsrivishnu
Copy link

I suppose the goal of the comments was to also show that the community is looking forward to having these changes released.

@rafaelfranca
Copy link
Member

Yeah. I think we can release a minor release for this change. I'll do it

@Vannsl
Copy link

Vannsl commented Dec 15, 2021

Thank you @rafaelfranca, that's a lovely christmas present!

@joker-777
Copy link

@rafaelfranca It would amazing if you could release this change officially.

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 this pull request may close these issues.

6 participants