Skip to content
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

Rename zxcvbn.js to zxcvbn.min.js and leave zxcvbn.js unminified #83

Closed
binarykitchen opened this issue Jul 15, 2015 · 4 comments
Closed

Comments

@binarykitchen
Copy link

Because I want to minify it myself through other minifiers.

And it's a resource we can build, not source code itself. Doesn't belong to source control either.

See
http://stackoverflow.com/questions/10854845/should-i-version-control-the-minified-versions-of-my-jquery-plugins and http://blog.andrewray.me/dealing-with-compiled-files-in-git/

@lowe
Copy link
Collaborator

lowe commented Jul 15, 2015

Hey @binarykitchen -- Thanks for the suggestion but I'm going to keep it as-is for now, here's the rationale:

  1. minification -- {matching,scoring,adjacency_graphs,frequency_lists,init}.js are all unminified and together constitute zxcvbn.js. If you want unminified files, whether for debugging purposes, for using your own bundler, etc, I would argue it's better to work with this set vs an unminified bundle.
  2. resource vs source -- I agree with your point, however because the zxcvbn build step currently depends on python and java, I don't want to force every client to have to build zxcvbn themselves. I realize it's inelegant to see zxcvbn.js diffs in nearly every commit. It's on my roadmap to remove python and java dependencies and build entirely within the node ecosystem, at which point I'll revisit removing zxcvbn.js from source control.

@lowe lowe closed this as completed Jul 15, 2015
@binarykitchen
Copy link
Author

  1. No. Problem is that module.exports = require('./zxcvbn').zxcvbn; in your index.js is requiring the minified version. If you expect me to do a require('./zxcvbn'). {matching,scoring,adjacency_graphs,frequency_lists,init}; i.E. in gulp before minification, then that's very bad practice.
  2. Great. Please do that. For now, tell me why it depends on python and hava?

Furthermore you should include {matching,scoring,adjacency_graphs,frequency_lists} into init.js through require() instead of manually bundling them together.

@lowe
Copy link
Collaborator

lowe commented Jul 15, 2015

  1. index.js is no longer used anywhere and scheduled for removal in the next release. See the end of init.coffee to see how export works (it doesn't reference zxcvbn.js). I don't see a problem here.
  2. I initially wrote this in 2012 and closure (java) provided the best size reduction. I'm considering switching to uglifyjs to remove the java dependency even though it adds about 100kb vs closure. re: python, it was easier for me to write those scripts in python at the time, but should be easy to port to js.
  3. I likely won't ever use require anywhere internally in the lib because I want it to work outside of node / other CommonJS environments. Build dependencies on node are fine, but not runtime dependencies.

@binarykitchen
Copy link
Author

I see ... okay then, see what happens in your next versions. Thanks for your hard work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants