-
-
Notifications
You must be signed in to change notification settings - Fork 158
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
feat: use webpack hashFunction rather than hard-code MD4 #213
feat: use webpack hashFunction rather than hard-code MD4 #213
Conversation
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.
One note, tests need to be rewritten too
Anyway thanks for the PR, good job! |
Please accept CLA too |
Codecov Report
@@ Coverage Diff @@
## master #213 +/- ##
==========================================
+ Coverage 95.67% 95.96% +0.29%
==========================================
Files 7 7
Lines 393 397 +4
Branches 152 155 +3
==========================================
+ Hits 376 381 +5
+ Misses 17 16 -1
Continue to review full report at Codecov.
|
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.
Hm, i forgot what webpack has utils for creating hashes https://github.com/webpack/webpack/blob/master/lib/index.js#L154, i think will be great to use it instead own implementation
Okay, using that now instead. |
@@ -10,6 +9,7 @@ import { | |||
javascript, | |||
version as webpackVersion, | |||
} from 'webpack'; | |||
import createHash from 'webpack/lib/util/createHash'; |
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.
Please use import { utils: { createHash } } from 'webpack'
, no need hardcore path to exported utils
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.
Can't destructure in an import statement.
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.
Just use webpack.utils.createHash
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.
Doesn't work.
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.
Strange, I will look at this in near future, anyway thanks for the PR
Let's merge, I will refactor before release 👍 Thanks! |
This PR contains a:
Motivation / Use-Case
Previously, this plugin had hard-coded MD4 as the hash algorithm to use. But Webpack provides a
hashFunction
in its config which should be used instead. The reason for this is that MD4 is not supported on all systems. For instance, a FIPS-compliant system cannot use MD4 or MD5 because they are not secure algorithms. So any use of this plugin on such a system will fail the build.Now, if a users' webpack config has set a specific
hashFunction
to use, this plugin will respect that. If not, it will fall back to the MD4 algorithm by default.Breaking Changes
This PR is backwards compatible.
Additional Info
Note: I added a
compiler
constructor parameter to theWebpackXCache
classes so thehashFunction
can be extracted from the Webpack config. I was not sure if the existingcompilation
oroptions
parameters might also include the Webpack config. If either does, we can remove the new parameter.