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

Adds scrypt support in node 12 #739

Merged
merged 3 commits into from
Jul 8, 2020
Merged

Adds scrypt support in node 12 #739

merged 3 commits into from
Jul 8, 2020

Conversation

bojeil-google
Copy link
Contributor

Replaces scrypt npm module (no longer maintained and
doesn't work for node 12) for testing standard scrypt with
crypto.scryptSync which has been supported since node v10.5.

Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

This LGTM 👍

Let's hold off on merging until Node 8 support is officially terminated next month.

@bojeil-google
Copy link
Contributor Author

Node 8's end of life is December 31, 2019. I guess the earliest business day we can merge is 1/2/2020. Or do you prefer some time later that month?

@hiranya911
Copy link
Contributor

Gcloud (Firestore, Storage etc) has a plan to drop Node8 support sometime in mid January. Let's align with their timetable.

@sk-
Copy link
Contributor

sk- commented Jun 25, 2020

Firestore already dropped support for Node 8. See #921

bojeil-google and others added 3 commits July 8, 2020 10:57
Replaces scrypt npm module (no longer maintained and
doesn't work for node 12) for testing standard scrypt with
`crypto.scryptSync` which has been supported since node v10.5.
@hiranya911
Copy link
Contributor

It's taken a while, but we are finally ready to drop Node 8 support.

@bojeil-google I've rebased your branch against latest master, and updated the GitHub Actions configs to drop Node 8 support. Can I ask you to take a quick look?

@hiranya911
Copy link
Contributor

Related to #921

Copy link
Contributor Author

@bojeil-google bojeil-google left a comment

Choose a reason for hiding this comment

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

LGTM

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.

3 participants