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

change Bcrypt for bcryptjs #32

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

mdepecker
Copy link

bcryptjs is API compatible to bcrypt but 100% pure JS, which avoids issues with the node-gyp compiling, for example having the wrong python version and similar issues.

Basically, it is the same thing but just JS, so no issues caused by the native stuff, which makes for better forward and backwards compatibility and an easier time when deploying. Always bet on JS™ :)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.44%) to 39.33% when pulling ff786b3 on mdepecker:master into 2897e12 on waterlock:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.44%) to 39.33% when pulling ff786b3 on mdepecker:master into 2897e12 on waterlock:master.

@wayne-o
Copy link
Contributor

wayne-o commented Sep 29, 2015

Is this the preferred way to do this? I've read bad things perf wise but have zero numbers to back this up!

Are we wanting to go down the JS compat route to eliminate native code?

If we do then i can clean up and merge in.

@s-devaney
Copy link

Yes. In order to get waterlock-local-auth working on Windows with current native bcrypt implementation I had to install Python (~200mb?) and Windows Visual Studio (~4GB) which is just ridiculous.

If bcryptjs is a performance concern my recommendation would be to use npm credential, but that is outside of the scope of this PR.

@mdepecker
Copy link
Author

@s-devenay

That's exactly the point that I try to pointed out. I don't know about credential
But just for the sake of our sanity and the easy to use stuff this could be great

@kriswill
Copy link
Contributor

kriswill commented Oct 9, 2015

I think this is fine, especially since bcrypt updating to latest nan is so slow. rebase and squash, and I'll merge it.

@yura415
Copy link

yura415 commented Oct 21, 2015

So any chances that this will be accepted? Can't lift my app on production server 'cause can't install newer version of GCC on FreeBSD.

@mdepecker
Copy link
Author

Sorry , didn't really have time to handle I'll rebase this and wait for merge
@yura415
For a quick fix you can just check my own version and see what change then do the same, if this will be merge just do a npm update

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.

6 participants