-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix(error): add toJSON to APIError so response headers serialize correctly #1848
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: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,6 +36,26 @@ export class APIError< | |
| this.type = data?.['type']; | ||
| } | ||
|
|
||
| /** | ||
| * Returns a JSON-serializable representation of this error. | ||
| * | ||
| * The native `Headers` class does not serialize its entries with | ||
| * `JSON.stringify`, so this method converts them to a plain object so that | ||
| * rate-limit headers (e.g. `x-ratelimit-reset-requests`) and other response | ||
| * headers are visible when the error is logged or serialized. | ||
| */ | ||
| toJSON() { | ||
| return { | ||
| status: this.status, | ||
| headers: this.headers ? Object.fromEntries(this.headers.entries()) : undefined, | ||
| error: this.error, | ||
| code: this.code, | ||
| param: this.param, | ||
| type: this.type, | ||
| request_id: this.requestID, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The serializer emits Useful? React with 👍 / 👎.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, I'll rename the property to
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, I'll rename the property to |
||
| }; | ||
| } | ||
|
|
||
| private static makeMessage(status: number | undefined, error: any, message: string | undefined) { | ||
| const msg = | ||
| error?.message ? | ||
|
|
||
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.
toJSON()now returns a fixed object literal, soJSON.stringify()no longer includes enumerable fields added by subclasses (for exampleOAuthError.error_code) or by runtime assignment (for exampleAPIConnectionError.cause). Before this change those properties were serialized automatically, so this is a regression for log pipelines or cross-process error transport that depend on those fields.Useful? React with 👍 / 👎.
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.
Yeah good point, I'll switch it to use
Object.assignto pull in the subclass properties instead of hardcoding the fields.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.
Yeah good point, I'll switch it to use
Object.assignto pull in the subclass properties instead of hardcoding the fields.