-
Notifications
You must be signed in to change notification settings - Fork 93
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
chore!: migrate gax to Node 18 #1699
base: main
Are you sure you want to change the base?
Conversation
Warning: This pull request is touching the following templated files:
|
…to migrateToNode18
@@ -10,61 +10,62 @@ | |||
"!build/src/**/*.map" | |||
], | |||
"dependencies": { | |||
"@grpc/grpc-js": "^1.10.9", | |||
"@grpc/grpc-js": "^1.12.6", | |||
"@grpc/proto-loader": "^0.7.13", | |||
"@types/long": "^5.0.0", | |||
"abort-controller": "^3.0.0", |
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.
This is global in Node 18+; we can drop this dependency in this or separate PR.
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.
Looking through the code, I believe this is only used in browser-contexts.
gax/package.json
Outdated
"duplexify": "^4.0.0", | ||
"google-auth-library": "^9.3.0", | ||
"duplexify": "^4.1.3", | ||
"google-auth-library": "^9.15.1", | ||
"node-fetch": "^2.7.0", |
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.
We will need this bumped to v3 in order to avoid punycode
warnings in Node v22. Here are a few examples:
- DeprecationWarning: The
punycode
module is deprecated. Please use a userland alternative instead google-auth-library-nodejs#1829 - punycode deprecation warning gaxios#640
Here’s an example migration:
gax/package.json
Outdated
"protobufjs": "^7.3.2" | ||
"protobufjs": "^7.4.0", | ||
"retry-request": "^7.0.2", | ||
"uuid": "^11.0.5" |
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.
UUID is globally available as of Node 18+. Here’s how we can drop this dependency:
}, | ||
"devDependencies": { | ||
"@types/uuid": "^9.0.7", | ||
"@babel/plugin-proposal-private-methods": "^7.18.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.
We should be good to remove this now in Node 18+
gax/src/iamService.ts
Outdated
@@ -15,6 +15,7 @@ | |||
// ** This file is automatically generated by gapic-generator-typescript. ** | |||
// ** https://github.com/googleapis/gapic-generator-typescript ** | |||
// ** All changes to this file may be overwritten. ** | |||
/* eslint-disable @typescript-eslint/no-floating-promises */ |
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.
I think these should be handled rather than ignored - even if its adding .catch(console.error)
.
@@ -4,10 +4,11 @@ | |||
"rootDir": ".", | |||
"outDir": "build", | |||
"resolveJsonModule": true, | |||
"lib": ["ES2018", "DOM"] | |||
"lib": ["DOM"] |
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.
We may want ES2023
for language support, in case upstream dependencies use newer syntax or features.
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.
We get this for free since it inherits from the top-level config!
@@ -15,7 +15,7 @@ | |||
"url": "https://github.com/googleapis/gax-nodejs/issues" | |||
}, |
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.
[Optional] We can use npm workspaces now:
Which has the benefit of enabling top-level tests for all workspaces/packages among other features.
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.
hm good call! Maybe a good process request?
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!
For gax and tools directory: