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

Windows doesn't support bcrypt, swap in bcrypt-nodejs #55

Closed
wants to merge 2 commits into from
Closed

Conversation

sullivanpt
Copy link

There are two changes in this request; both are intended to make it easier to get started with generator-angular-fullstack.

First is explicitly including grunt-karma in the npm dev dependencies.

Second is swapping out bcrypt with bcrypt-json. The function signature is identical. Most of us (unfortunate) Windows users can't compile native bcrypt.cc easily.

Thanks much.

@DaftMonk
Copy link
Member

I was worried about using bcrypt for this very reason. However, because grunt-contrib-imagemin also required node-gyp, it seemed that most people should already have it.

If we were to replace the bcrypt dependency, I'd seriously consider just using nodes Crypto PBKDF2 encryption, because I'm not sure how reliable/fast the pure javascript implementation of bcrypt is.

I'd be happy to hear more people chime in on this issue though.

The karma dependencies should be handled by generator-karma. Are you having issues with them being installed?

@kjellski
Copy link

I've used bcrypt for my contributions to passport mainly because it should be stronger for short passphrases(source: http://codahale.com/how-to-safely-store-a-password/). Since this is only a starter project and you could really fast switch out the encryption provider back to bcryt, I would also suggest going for "the normal crypt" like @DaftMonk mentioned.

@sullivanpt
Copy link
Author

Re generator-karma: I started with a clean slate: no global npm modules on my system. Then I used these commands:

npm install -g yo generator-angular-fullstack; mkdir myApp; cd myApp; yo angular-fullstack

I selected No compass/sass, and yes to everything else.

I interrupted the 'auto' npm install, swapped bcrypt-nodejs, and then ran:

npm install; bower install; grunt

And I encountered the build error "task 'karma' is not defined".

A quick look at generator-angular shows an explicit dependency to grunt-karma in the generated package.json. Adding this to the broken myApp and rerunning 'npm install' makes it build and run fine.

My opinion: This plus the bcrypt issue make a bad first time experience, especially for naive, new users, which I assume is who these generators target. It should 'just work' out of the box.

Sent from my Windows Phone


From: Tyler Henkelmailto:[email protected]
Sent: ‎1/‎16/‎2014 4:23 AM
To: DaftMonk/generator-angular-fullstackmailto:[email protected]
Cc: Patrick Sullivanmailto:[email protected]
Subject: Re: [generator-angular-fullstack] Windows doesn't support bcrypt, swap in bcrypt-nodejs (#55)

I was worried about using bcrypt for this very reason. However, because grunt-contrib-imagemin also required node-gyp, it seemed that most people should already have it.

If we were to replace the bcrypt dependency, I'd seriously consider just using nodes Crypto PBKDF2 encryption, because I'm not sure how reliable/fast the pure javascript implementation of bcrypt is.

I'd be happy to hear more people chime in on this issue though.

The karma dependencies should be handled by generator-karma. Are you having issues with them being installed?


Reply to this email directly or view it on GitHub:
#55 (comment)

@DaftMonk
Copy link
Member

@sullivanpt You're right, I'll release a small update to replace bcrypt with the native crypto library to make it easier for new users.

On the issue of karma dependencies, generator-karma is supposed to add those, but it has been unreliable in the past. If something goes wrong during the npm install, those packages aren't added to the package.json. So I suppose we could add them as dependencies directly to the package.json template.

@DaftMonk DaftMonk closed this in 13ea60e Jan 16, 2014
DaftMonk added a commit that referenced this pull request Jan 16, 2014
@DaftMonk
Copy link
Member

Published the new patch over npm. Let me know if it works for you.

@sullivanpt
Copy link
Author

Thanks, (almost) works. I started from scratch (cleared my global installs and cache) with "npm install -g yo [email protected]" and then did not experience any of the previous issues with bcrypt or generator-karma.

One trivial new issue, there's a jshint error out of the box when running grunt :-(

Running "jshint:server" (jshint) task

lib/models/user.js
line 145 col 14 'salt' is already defined.

@DaftMonk
Copy link
Member

Oh shoot, I'll fix that.

@DaftMonk
Copy link
Member

Fixed and published,

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

Successfully merging this pull request may close these issues.

3 participants