Closed ryanwarsaw closed 7 years ago
Don't merge this, it looks like something is still going on with Travis CI but it isn't failing the build.
Edit: It looks like it's not an error caused by this PR, but lets wait until Monday just in-case.
@cadecairos What are your thoughts on merging this? I've looked into this further and the effects of this change are nullified by the fact that other dependencies within this project (sqlite3) and others in the Thimble chain would still require on-site compilation with gcc/clang if a remote binary isn't present.
That and the fact that this will ultimately make things slower and less performant for better or worse, I'm having a hard time justifying this change but want to leave it to you for final opinions.
repinging @cadecairos to make sure this isn't lost - it's currently blocking some dependency upgrade work in Thimble.
@ryanwarsaw can I get an update from you on this? I'd like to move the LTS stuff in thimble forward. Thanks.
@humphd Yeah, i'm just going to go cold turkey on this one 🤞
I reviewed the method structure for our current usage of bcrypt and cross-checked it with the method structures for bcrypt.js and it's basically a drop and replace solution. If someone could review this and ensure everything is functioning properly, that'd be great.
I'm filing a separate PR to remove the redundant New Relic references, I wanted to keep this PR as simple as possible and only include the changes relevant for it to function properly.