-
Notifications
You must be signed in to change notification settings - Fork 373
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: Dropped Node 8 support and upgraded Firestore/Storage dependencies #934
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Why is the engines field already at Node 10? Did you just merge this into master?
package.json
Outdated
@@ -61,15 +61,15 @@ | |||
"node-forge": "^0.7.6" | |||
}, | |||
"optionalDependencies": { | |||
"@google-cloud/firestore": "^3.0.0", | |||
"@google-cloud/firestore": "^4.0.0", | |||
"@google-cloud/storage": "^4.1.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update to 5.x as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Engines field was updated in #739 |
@@ -61,15 +61,15 @@ | |||
"node-forge": "^0.7.6" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hiranya911 Given that you are already updating the dependencies, could you update this one to 0.9.1. Given that it is a 0.x version, npm considers it to be a breaking change, and hence not compatible with the current requirement.
node-forge
is also transitively being used by firestore (direct dependency is google-p12-pem), and requiring version ^0.9.0, which means two copies of the library are currently needed. Switching to just one dependency would save 1.7Mib.
Good point @sk-. I'll send a separate PR for that. Feel free to beat me to it :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Hiranya! LGTM!
As commnented on firebase#934 (comment) Given that node-forge is a 0.x version, npm considers it to be a breaking change, and hence not compatible with the current requirement. node-forge is also transitively being used by firestore (direct dependency is google-p12-pem), and requiring version ^0.9.0, which means two copies of the library are currently needed. Switching to just one dependency would save 1.7Mib.
As commnented on #934 (comment) Given that node-forge is a 0.x version, npm considers it to be a breaking change, and hence not compatible with the current requirement. node-forge is also transitively being used by firestore (direct dependency is google-p12-pem), and requiring version ^0.9.0, which means two copies of the library are currently needed. Switching to just one dependency would save 1.7Mib.
Building on top of #739 this PR:
@google-cloud/firestore
to latest major version.API CHANGE: Admin SDK now requires Node.js 10 or higher. Node.js 8 support has been terminated.
API CHANGE: Upgraded dependency on the
@google-cloud/firestore
package to v4.API CHANGE: Upgraded dependency on the
@google-cloud/storage
package to v5.Resolves #934