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

Node crypto.scrypt support #10

Open
Cretezy opened this issue Oct 9, 2018 · 6 comments
Open

Node crypto.scrypt support #10

Cretezy opened this issue Oct 9, 2018 · 6 comments

Comments

@Cretezy
Copy link

Cretezy commented Oct 9, 2018

Currently this packages uses the scrypt package, but since Node v10.5, Node ships with it's own built-in crypto.scrypt implementation.

Is their plans to support this?

A possible integration plan would be:

  • Make the scrypt package optional
  • Use crypto.scrypt when possible, fallback to scrypt package
    • If both are not found (< v10.5 && package not install), throw an error
  • Bump major version (since this is a breaking change)
    • Add in the README to install scrypt if using Node < v10.5

Would that be a good plan? I would be willing to implement this if we have a consensus.

@simonepri
Copy link
Owner

Thank you for opening this issue!
I don't really like the optional dependency thing, but the usage of crypto.scrypt is definitely something I want to integrate here.

I have some plans for this, I'll try to elaborate more on this in the following days.

@Cretezy
Copy link
Author

Cretezy commented Nov 24, 2018

The scrypt package is now mostly broken, and abandoned. I think now would be the time to upgrade to crypto.scrypt.

Edit: Working on a PR for this.

@simonepri
Copy link
Owner

Hi @Cretezy I'm already in the process of rewriting all the phc-* packages, but I didn't finish yet.
I was just super busy in the last month and I didn't had time to release the new versions, but they will come soon.

@Cretezy
Copy link
Author

Cretezy commented Nov 25, 2018

Sounds good! Would be great to have a branch with the newest code so we can check it out in the meantime for fun!

Great work!

Also, I've been using my fork (which is the PR) and it works great so far.

@simonepri
Copy link
Owner

I'm not really happy with just merging the pr here.
The use of crypto.scrypt is a breaking change so we should handle it nicely.
I would rather prefer to wait until I finish the new structure for the packages.

@Cretezy
Copy link
Author

Cretezy commented Feb 21, 2019

Hey! Any updates on this?

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

No branches or pull requests

2 participants