-
Notifications
You must be signed in to change notification settings - Fork 158
chore!: migrate to Node 18 #2399
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
base: main
Are you sure you want to change the base?
Conversation
Warning: This pull request is touching the following templated files:
|
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.
Just want to make sure you're following the script here, b/397525375 (might be worth mentioning it in your PR description)
Update to latest image
* I will fix the floating promise errors. This PR fixes all the floating promise errors in the test files. It also converts all the `then` calls to `async/await` in the test files. * remove jules txt output files * Fix hanging promise errors * Attempting CI fix of tests * Change solution to floating promises where adding await previously changed the meaning of the test. * revert void assert.rejects changes in gapic_firestore_v1beta1.ts tests * Address PR comments --------- Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
Approved! I think your tests are failing because you don't have a Node 18 kokoro file (this might be because it's excluded in the owlbot.py file). It should mostly just comprise of renaming the kokoro 14 files to 18 (and making sure they're using 18 as the image, node version, etc.). |
// This list is based on https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/src/core/transaction_runner.ts#L112 | ||
switch (error.code as number) { | ||
case StatusCode.ABORTED: | ||
case 409: // GAXIOS may now return HTTP 409 instead of Aborted |
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.
So weird to see a 409 in the middle of the StatusCodes :)
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.
At least I expected the formatter to move it before (or after) all other case
statements.
options?: gax.CallOptions | ||
): AsyncIterable<protos.google.longrunning.ListOperationsResponse> { | ||
options?: gax.CallOptions, | ||
): AsyncIterable<protos.google.longrunning.IOperation> { |
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.
Not sure why this changed. The proto still returns a ListOperationsResponse
@@ -1,1714 +0,0 @@ | |||
## API Report File for "@google-cloud/firestore" |
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.
what was the reason for removing firestore.api.md?
Fixes b/397525375. Uses steps defined in b/397525375.