Introduce Webpack for browser bundling #33
Conversation
|
@pvorb is this PR ok? I don't think adding webpack is too much a problem, it's a dev-dep |
There was a problem hiding this comment.
Sorry, this went unnoticed for some reason.
I really like the idea of using Webpack in order to provide a minified version of this module. I know this is how you do NPM modules in 2016+ but this module hasn't changed much since the beginnings of Node and I am not willing to invest much of my time in advancing tooling as it's unlikely to change a lot in the future.
Your PR is valid, but needs some minor improvements before I will accept it.
| @@ -1,9 +1,8 @@ | |||
| language: node_js | |||
| node_js: | |||
| - 0.6 | |||
| - 0.8 | |||
There was a problem hiding this comment.
Why did you remove old Node.js versions?
There was a problem hiding this comment.
I think webpack fails to install with them, on travis
npm WARN engine mocha@2.3.4: wanted: {"node":">= 0.8.x"} (current: {"node":"0.6.21-pre","npm":"1.1.37"})
npm WARN engine webpack@2.4.1: wanted: {"node":">=4.3.0 <5.0.0 || >=5.10"} (current: {"node":"0.6.21-pre","npm":"1.1.37"})
0.12 works though
0.6 and 0.8 are probably old enough to be skipped
Thanks for the reviews, didn't know for webpack -p (5.9kB vs 5.7kB before, but that's fine).
Yes I think it'd be nice to package it for browser, easily accessible from things like https://unpkg.com/md5@latest/dist/md5.min.js
There was a problem hiding this comment.
The origin file is 5.86kB by the way, so this even got worse by -p. I start to wonder if this is even worth the hassle.
There was a problem hiding this comment.
After gzip the original file is 1837 bytes. md5.min.js is 2474 after gzip. Webpack really seems to make things worse.
There was a problem hiding this comment.
Before switching to -p, things weren't that much better:
$ ls -l md5*
-rw-r--r-- 1 Paul 197121 5996 Aug 31 2016 md5.js
-rw-r--r-- 1 Paul 197121 1837 Aug 31 2016 md5.js.gz
$ ls -l dist/md5*
-rw-r--r-- 1 Paul 197121 5878 Apr 25 00:36 dist/md5.min.js
-rw-r--r-- 1 Paul 197121 2378 Apr 25 00:36 dist/md5.min.js.gz
There was a problem hiding this comment.
yes but md5.js has some requires at the top, so it can't really be compared to dist/md5.min.js which is bundled.
Ah sorry for the confusion, I was comparing 5.9 vs 5.7 (that I had with UglifyJS plugin before, for dist/md5.min.js too)
There was a problem hiding this comment.
Sorry, you're right. It's easy to miss that point. :)
demo/index.html
Outdated
| <output id="output"></output> | ||
| <style> | ||
| output::before { | ||
| content: "output:"; |
There was a problem hiding this comment.
Please use two spaces for indentation throughout the PR.
demo/index.html
Outdated
| reader.onload = e => { | ||
| resolve(e.target.result) | ||
| } | ||
| }); |
There was a problem hiding this comment.
Don't use ES6 syntax as it's not supported by every browser around.
package.json
Outdated
| }, | ||
| "dependencies": { | ||
| "charenc": "~0.0.1", | ||
| "crypt": "~0.0.1", | ||
| "is-buffer": "~1.1.1" | ||
| }, | ||
| "devDependencies": { | ||
| "mocha": "~2.3.4" | ||
| "mocha": "~2.3.4", | ||
| "webpack": "^2.2.1" |
There was a problem hiding this comment.
This should be "webpack": "~2.2.1"
demo/index.html
Outdated
| } | ||
|
|
||
|
|
||
| </script> |
There was a problem hiding this comment.
All files need to end with a new line character.
webpack.config.js
Outdated
| comments: false | ||
| } | ||
| }) | ||
| ], |
There was a problem hiding this comment.
Isn't the UglifyJsPlugin obsolete? webpack -p should already compress the script.
No description provided.