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.

Sign up for GitHub

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

Jump to bottom

feat!: Use Error#cause in GaxiosError #661

Closed
wants to merge 12 commits into from
Closed

feat!: Use Error#cause in GaxiosError #661

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
f9a2d69
feat: Support `timeout` for `fetch` and `node-fetch` v3
d-goog Oct 25, 2024
59a4655
test: fix
d-goog Oct 25, 2024
d5bda53
test: fix
d-goog Oct 25, 2024
0190a1d
feat: `timeout`s should be retryable
d-goog Oct 25, 2024
9b76bba
Merge branch 'main' of github.com:googleapis/gaxios into update-timeout
d-goog Oct 25, 2024
85a81c6
chore: lint fixes
d-goog Oct 25, 2024
86c29db
feat!: Use `Error#cause` in `GaxiosError`
d-goog Oct 25, 2024
a45d9e1
Merge branch 'update-timeout' of github.com:googleapis/gaxios into ga…
d-goog Oct 25, 2024
99ba060
Merge branch 'main' into gaxios-error-native-cause
danielbankhead Oct 29, 2024
8cd6e2a
Merge branch 'main' of github.com:googleapis/gaxios into gaxios-error…
d-goog Oct 30, 2024
f5a4cae
Merge branch 'main' of github.com:googleapis/gaxios into gaxios-error…
d-goog Nov 1, 2024
ad63f64
fix: merge tsconfig
d-goog Nov 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions src/common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,9 @@ export class GaxiosError<T = any> extends Error {
message: string,
public config: GaxiosOptionsPrepared,
public response?: GaxiosResponse<T>,
public error?: Error | NodeJS.ErrnoException,
cause?: unknown,
Copy link
Contributor

Choose a reason for hiding this comment

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

From the documentation, it seems like cause is in addition to, not instead of the error. Is it possible to set both in GaxiosError for it to not be a breaking change?

Also, seems like any is better than unknown here, WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As GaxiosError is an error itself (with a few additional custom params) it would enable a chain of Errors to be appropriately logged along with each individual call stacks - providing a complete context for customers. The error param was a stop-gap, but often it would not be used properly. I think cleanly migrating would be a more optimal UX instead of both.

unknown is the appropriate type - cause could be anything, but should be verified through type checks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps a less destructive way then would be to add a deprecated tag to error, and simply continue to have it there? I'm concerned that it's a needlessly breaking change, especially since we're bundling a few other breaking changes in this release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A quick code search in googleapis will show virtually none of our libraries are using this property and it will be far more intuitive in the future to simply clean it up now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right but perhaps it's being used by external users?

) {
super(message);
super(message, {cause});

// deep-copy config as we do not want to mutate
// the existing config for future retries/use
Expand All @@ -126,8 +126,16 @@ export class GaxiosError<T = any> extends Error {
this.status = this.response.status;
}

if (error && 'code' in error && error.code) {
this.code = error.code;
if (this.cause instanceof Error) {
if (this.cause instanceof DOMException) {
// 'code' is a legacy `number` for DOMExceptions, use the `name` instead:
// - https://developer.mozilla.org/en-US/docs/Web/API/DOMException#error_names
// Notably, useful for `AbortError`:
// - https://developer.mozilla.org/en-US/docs/Web/API/DOMException#aborterror
this.code = this.cause.name;
} else if ('code' in this.cause && typeof this.cause.code === 'string') {
this.code = this.cause.code;
}
}

if (config.errorRedactor) {
Expand Down
13 changes: 9 additions & 4 deletions src/gaxios.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,10 +144,15 @@ export class Gaxios {
}
return translatedResponse;
} catch (e) {
const err =
e instanceof GaxiosError
? e
: new GaxiosError((e as Error).message, opts, undefined, e as Error);
let err: GaxiosError;

if (e instanceof GaxiosError) {
err = e;
} else if (e instanceof Error) {
err = new GaxiosError(e.message, opts, undefined, e);
Copy link
Contributor

Choose a reason for hiding this comment

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

(just for my curiosity) - why wouldn't we contstruct the error with e.cause vs. e.message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It allows GaxiosError to optionally have a root cause (say, the error is simple or purely on the gaxios level).

} else {
err = new GaxiosError(new Error(e as '').message, opts, undefined, e);
}

const {shouldRetry, config} = await getRetryConfig(err);
if (shouldRetry && config) {
Expand Down
5 changes: 2 additions & 3 deletions src/retry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,8 @@ function shouldRetryRequest(err: GaxiosError) {
const config = getConfig(err);

if (
(err.config.signal?.aborted && err.error?.name !== 'TimeoutError') ||
err.name === 'AbortError' ||
err.error?.name === 'AbortError'
(err.config.signal?.aborted && err.code !== 'TimeoutError') ||
err.code === 'AbortError'
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we getting rid of err.name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That line could never be true as err.name is always GaxiosError

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, this doesn't seem entirely relevant to this PR, I'm concerned it could have unintended consequences (even if it is a no-op). Would it be alright to leave it as a follow up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It’s not relevant, but is a one-line change. Not at all a big deal. Not worthy of a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, those are often the most tricky! We've had this situation occur before. Until we have very robust integration testing, I think it would be wise to make small incremental changes. If we had good integration tests I'd feel better about it!

) {
return false;
}
Expand Down
2 changes: 1 addition & 1 deletion tsconfig.base.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"extends": "gts/tsconfig-google.json",
"compilerOptions": {
"lib": ["es2020", "dom"],
"lib": ["ES2023", "dom"],
"esModuleInterop": true,
"rootDir": "."
},
Expand Down