-
Notifications
You must be signed in to change notification settings - Fork 84
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
base: master
Are you sure you want to change the base?
Conversation
1 similar comment
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. |
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. |
@s-devenay That's exactly the point that I try to pointed out. I don't know about credential |
I think this is fine, especially since bcrypt updating to latest nan is so slow. rebase and squash, and I'll merge it. |
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. |
Sorry , didn't really have time to handle I'll rebase this and wait for merge |
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™ :)