-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add support for sha256 hashes #1613
Conversation
6d66286
to
f382aeb
Compare
f382aeb
to
de53a5a
Compare
@evanw Is there anything blocking from getting this PR merged? I'm happy to make any required changes. Integration with Rails 7 would benefit greatly from having a more predictable hash signature than base32. |
I'd like to weigh in and voice my support for the proposed PR. Aside from Rails, having a flexible interface and a path forward for easily adding different fingerprinting methods for fragments and files is a great addition. |
It is not really clear from the PR. But why can't Rails change to support how esbuild does the hashing, rather than having esbuild change to support how Rails does it? |
I think that's a fair question @jtarchie and ultimately boils down to who owns the responsibility for the calculation of an asset or chunk fingerprint/digest, and what that calculation looks like. For other bundlers like These options commonly include:
For ESBuild, the fingerprinting/digest does currently to the best of my knowledge not allow specifying the Unfortunately, there is no standard or agreement between asset build tools, or other web standards like Thus I consider this PR valuable, as it opens up ESBuild for allowing users to specify their desired hash/digest algorithm to be in line with the rest of the front-end build and delivery pipeline: ultimately the web server, or CDN for an application may serve assets that could come from many different sources, languages, frameworks, and build tools. As a user I'd like to have some way to say "I want to standardize on [hash-algorithm]" for my asset digests, regardless of what build tool they come from. |
Thanks for the explanation. I understand the purpose, but not the need. There could be other tools that also don’t have the hashing capability. Your framework could easily adapt to those rather than waiting on them to have specific basing algorithms. Or instead of the extensibility in core esbuild binary, could this be done through plugins |
de53a5a
to
094c51d
Compare
@jtarchie Base32 is not in the Ruby standard library, so it would require another dependency somewhere in the Rails stack to support esbuild. The current approach also relies on regex to detect hashes, which is simpler to do for hex characters. My motivation is to make the onboarding experience with esbuild in Rails as smooth as possible while making it possible to use modern bundling techniques like bundle splitting and asynchronous loading. I know there has been development on the Rails side to make detecting hashes more flexible, I'll pursue an alternative approach that doesn't require this PR. |
Esbuild will be one of the recommended bundlers to use with Rails 7 (through the jsbundling-rails gem), but due to how hashes are computed differently in esbuild and sprockets using code splitting is hard to configure right out of the box.
A patch has already been made to sprockets via rails/jsbundling-rails#15 and rails/sprockets#714. These changes make it possible for the asset pipeline in Rails to not re-digest files that already end with a SHA256 digests, and as Webpack already supports SHA256 it will make code splitting work in new Rails projects. This PR should add support for SHA256 in Esbuild as well (without breaking any backwards-compatibility). With a couple more changes to
jsbundling-rails
, it should make code splitting work for most users.The new CLI parameter
--hash-function
and API/config optionHashFunction
, will let users determine whether to use the current Base32 hashes or SHA256.A lot of the code changes are relevant for passing the options around internally in esbuild - so please let me know if it can be refactored or changed to fit the project guidelines.
The most important change is here:
https://github.com/onelittle/esbuild/blob/6d66286349245a3fbe2d6d2783a6d0bf4cb520d6/internal/bundler/linker.go#L377-L383
If anyone wants to use other hash functions like
md5
orsha1
, these can now be added by changing the if to a switch statement.