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 |
pvorb
left a comment
pvorb
left a comment
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.
| 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 [email protected]: wanted: {"node":">= 0.8.x"} (current: {"node":"0.6.21-pre","npm":"1.1.37"})
npm WARN engine [email protected]: 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. :)
| <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.
| 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.
| "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"
| } | ||
|
|
||
|
|
||
| </script> No newline at end of file |
There was a problem hiding this comment.
All files need to end with a new line character.
| comments: false | ||
| } | ||
| }) | ||
| ], |
There was a problem hiding this comment.
Isn't the UglifyJsPlugin obsolete? webpack -p should already compress the script.
2 participants
No description provided.