Skip to content
This repository has been archived by the owner on Nov 16, 2023. It is now read-only.

Remove bcrypt-nodejs in favor of bcrypt. Closes #244 #245

Closed
wants to merge 1 commit into from

Conversation

davl3232
Copy link

@davl3232 davl3232 commented Oct 28, 2019

This PR replaces bcrypt-nodejs to bcrypt, since, as discussed in #244, the later is a better maintained version and uses the same API.

  • Updated an import and one call to bcrypt.hash() at models/User.ts.
  • Updated README.md references to bcrypt-nodejs.
  • Aligned a table in README.md.

Align table in README.md.
@msftclas
Copy link

msftclas commented Oct 28, 2019

CLA assistant check
All CLA requirements met.

@peterblazejewicz peterblazejewicz changed the title Remove bcrypt-nodejs in favor of bcrypt. Remove bcrypt-nodejs in favor of bcrypt. Closes #244 Oct 28, 2019
Copy link
Collaborator

@peterblazejewicz peterblazejewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Reviewed and run locally (mac OS)

@peterblazejewicz
Copy link
Collaborator

Hey @davl3232
Thanks for the change. Could you redact the PR message, for future reference? Appreciated!

@davl3232
Copy link
Author

I just did. Does that work?

@peterblazejewicz
Copy link
Collaborator

Yup!

@davl3232
Copy link
Author

I found an issue with this new library. It has native dependencies, so it fails on some platforms and NodeJS versions. Sorry for the bother, I'll test one that works with pure Javascript first.

@davl3232 davl3232 closed this Oct 29, 2019
@peterblazejewicz
Copy link
Collaborator

peterblazejewicz commented Oct 29, 2019

so it fails on some platforms and NodeJS versions

Can you post the scenario?
Thanks!

@davl3232
Copy link
Author

It failed for me building on a docker instance derived from https://hub.docker.com/_/node/ node:12-alpine

Here's my Dockerfile

FROM node:12-alpine

WORKDIR /usr/app

COPY package.json .

RUN npm i --quiet

COPY . .

ENV NODE_ENV=production

RUN npm run build

RUN npm i -g pm2 --quiet

EXPOSE 8082

CMD ["pm2-runtime", "dist/server.js"]

@davl3232
Copy link
Author

There's a bunch of issues on the repo like this one. kelektiv/node.bcrypt.js#759

It fails during npm install:

> [email protected] install /usr/app/node_modules/bcrypt
> node-pre-gyp install --fallback-to-build

node-pre-gyp WARN Using request for node-pre-gyp https download 
node-pre-gyp WARN Tried to download(404): https://github.com/kelektiv/node.bcrypt.js/releases/download/v3.0.6/bcrypt_lib-v3.0.6-node-v72-linux-x64-musl.tar.gz 
node-pre-gyp WARN Pre-built binaries not found for [email protected] and [email protected] (node-v72 ABI, musl) (falling back to source compile with node-gyp) 
gyp ERR! find Python 

It attempts a download a prebuilt binary for each platform + node version, if the platform is not known, then it tries to build the binary. In my docker instance it fails due alpine linux not having a python interpreter.

@peterblazejewicz
Copy link
Collaborator

Thanks!

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

Successfully merging this pull request may close these issues.

3 participants