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

esbuild fingerprint detection broken #728

Open
jarednorman opened this issue Dec 13, 2021 · 4 comments
Open

esbuild fingerprint detection broken #728

jarednorman opened this issue Dec 13, 2021 · 4 comments

Comments

@jarednorman
Copy link

Expected behavior

According to its description, #718 was meant to make it so that esbuild's chunk hashing could be detected by sprockets to avoid adding a second fingerprint. All that should be required by the user is to add .digested to the chunk name to make it more detectable and avoid false positives.

Actual behavior

The assets are refingerprinted.

The pattern does not match the current fingerprinting used by esbuild. It changed from SHA1 to xxHash in this PR. The hashes that esbuild now generates are base32, uppercase, exactly 8 chars.

The pattern than detects the fingerprinting is /-([0-9a-f]{7,128})\.digested/. (link)

Here's an example of a build with a line highlighting showing a hashed asset receiving a fingerprint from Sprockets:

CleanShot 2021-12-13 at 14 08 35

System configuration

  • Sprockets version: master
  • Ruby version: 2.7.4p191
  • Rails version: 6.1.4.1

Example App (Reproduction)

Sorry, no reproduction app, but I'm totally happy to make a PR to fix this.

@jarednorman
Copy link
Author

I can whip up a PR to change the fingerprint detection from /-([0-9a-f]{7,128})\.digested/ to something like /-([0-9a-zA-Z]{7,128})\.digested/ if that's not too permissive. I know the original intent was to avoid false positives, but with the \.digested that shouldn't be an issue.

@jhawthorn
Copy link
Member

jhawthorn commented Dec 13, 2021

👍 Given that some systems are case-insensitive treating A-Z the same as a-z seems like a good idea

@thanosbellos
Copy link

Hi, i think there is a pull request for this issue by @dhh, that will address this issue on the way you have both described.

The exact same change, has been recently merged into the main branch of the propshaft gem.

@jarednorman
Copy link
Author

Yep, that should resolve this!

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

No branches or pull requests

3 participants