-
-
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 improved webpack cache #1916
Conversation
This uses crypto (the good one) to create a hash from options, for strong and proper caching.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
👆 my thoughts exactly |
* @param {Options} options | ||
*/ | ||
function getOptionsHash(options) { | ||
const hash = createHash('sha256') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here speed would be of value, it may be worth considering a blake2 hash
const hash = createHash('sha256') | |
const hash = createHash('blake2s256') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m a bit uneasy with a digest format that is not frequently used.
Searching Google or markdown on GitHub seems to almost exclusively include it in lists of all possible algorithms, not in code like us here or in tutorials or so.
I was also uneasy about a bundled/external dependency. It does seem like blake2s256
is supported in all maintained versions of Node.js though, as it seems to have landed in OpenSSL 1.1.1. So it seems okay, but I know that there are smaller Node.js builds out there as well?
I’m also assuming that the time factor is fine when using sha256, options objects should be rather small (especially as functions omitted) and performance bottlenecks of webpack and MDX are in other areas.
I do see room for improvement around hashing the options. In the area of preventing crashes on cyclical references in options, adding support for functions, and using a package to hash an options object which likely already exists,
Given that you’re 👍 on this PR without this change, I’ll merge it as is, but I’m open to discussing improvements to this PRs functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the 2b variant is also an option, and is more common https://github.com/search?q=%22blake2b%22&type=Code
The reason I suggested 2s is it matches the hash size, and it optimized for 16/32 bit processors.
While 2b has a larger hash size and is optimized for 64 bit.
This uses crypto (the good one) to create a hash from options, for strong and proper caching.
This is somewhat based on the conversation in #1912 (comment). Note though that for us, options could be anything, including non-JSON values. So caching with JSON isn’t perfect. But it’s likely better than nothing.
/cc @remcohaszing