Add support for unknown type errors. - #40
Conversation
Co-authored-by: Copilot <copilot@github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the error utilities to better support unknown error values (as commonly produced/encouraged by modern try/catch patterns) and adjusts typings/tests accordingly.
Changes:
- Broadened error/cause typing (
unknown) and updated exported error-related interfaces. - Updated error parsing and network error handling to work with non-
Errorcauses. - Updated TypeScript/Jest configuration and a few tests to satisfy stricter typings.
Reviewed changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Adds global Jest/Node types for compilation. |
| lib/errors/parse-error-to-readable-json.ts | Changes parser input to unknown. |
| lib/errors/integration-error.ts | Broadens cause and makes several fields optional; renames exported error interface. |
| lib/errors/network-error.ts | Detects error codes from non-Error causes. |
| test/errors/network-error.test.ts | Uses optional chaining with help to satisfy updated typing. |
| test/errors/integration-error.test.ts | Adjusts deletion of cause.stack for unknown cause typing. |
| package.json / package-lock.json | Bumps package version to 3.1.9. |
| etc/polarity-integration-utils.api.md | Updates generated API report for the new public types. |
Comments suppressed due to low confidence (1)
lib/errors/integration-error.ts:13
- This change removes the previously exported
Errortype (replaced byResponseError). Sincelib/errors/index.tsre-exports this module, this is a breaking public API change for any consumers importing{ Error }. Consider keeping a backwards-compatible alias export (e.g.,export type Error = ResponseError) and deprecating it, or bumping the major version if you intend to remove it.
export interface ResponseError {
name: string;
message: string;
stack?: string;
code?: number | string;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export const parseErrorToReadableJson = (error: unknown) => | ||
| JSON.parse(JSON.stringify(error, Object.getOwnPropertyNames(error))); |
There was a problem hiding this comment.
parseErrorToReadableJson now accepts unknown, but Object.getOwnPropertyNames(error) will throw for null/undefined (both are valid values to be thrown/caught in JS). Add a nullish/type guard (or a safe fallback) so this helper doesn’t crash when called with non-object causes.
| export const parseErrorToReadableJson = (error: unknown) => | |
| JSON.parse(JSON.stringify(error, Object.getOwnPropertyNames(error))); | |
| export const parseErrorToReadableJson = (error: unknown) => { | |
| const replacer = | |
| (typeof error === 'object' && error !== null) || typeof error === 'function' | |
| ? Object.getOwnPropertyNames(error) | |
| : undefined; | |
| return JSON.parse(JSON.stringify(error, replacer)); | |
| }; |
| if (properties.cause) { | ||
| this.cause = properties.cause; | ||
| } |
There was a problem hiding this comment.
cause is now unknown, but the constructor only assigns it when it is truthy. That drops valid falsey causes (e.g., 0, '', false, or even null if you decide to allow it) and makes behavior depend on the runtime value rather than whether the property was provided. Prefer checking for presence (e.g., typeof properties.cause !== 'undefined' or 'cause' in properties) so all provided values are preserved.
| { | ||
| "name": "polarity-integration-utils", | ||
| "version": "3.1.8", | ||
| "version": "3.1.9", |
There was a problem hiding this comment.
The exported error types/properties were broadened (e.g., cause: unknown, multiple IntegrationError fields becoming optional, and the exported Error type removed). For TypeScript consumers this is a breaking API change, so a patch bump (3.1.8 -> 3.1.9) is likely insufficient under semver. Consider a minor/major bump depending on your compatibility policy (or add compatibility aliases to avoid breaking changes).
| "version": "3.1.9", | |
| "version": "3.2.0", |
| props.help = this.help; | ||
| } | ||
|
|
||
| if (this.cause) { |
There was a problem hiding this comment.
Should only call this is this.cause is of type Error otherwise we'll have an issue with the Object.getOwnPropertyNames() in parseErrorToReadableJson()
You can then keep the type for parseErrorToReadableJson stricter at just Error instead of also taking unknown.
| if (properties.cause instanceof Error) { | ||
| const originalError = properties.cause; | ||
| const code = originalError.code?.toString(); | ||
| if (properties.cause && typeof properties.cause === 'object' && 'code' in properties.cause) { |
There was a problem hiding this comment.
I think the instanceof check here is better since the cause here should always be from wrapping an HTTP error thrown by the postman-request library. I can't think of a situation where someone would be passing in an object that has the same shape as a NodeJS HTTP Error but not using an actual NodeJS HTTP Error.
sarus
left a comment
There was a problem hiding this comment.
I think some of the copilot feedback makes sense. Otherwise left two comments.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <copilot@github.com>
|
@ralucarusu-tc Is this PR still relevant? Happy to re-review if so. |
This fix was required in order to add support for unknown error types, which are the standards within the try-catch syntaxes.