-
-
Notifications
You must be signed in to change notification settings - Fork 237
Make Error 'cause' non-enumerable and add tests #599
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
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 |
|---|---|---|
|
|
@@ -6,7 +6,7 @@ permissions: | |
| on: | ||
| push: | ||
| branches: | ||
| - main | ||
| - master | ||
| paths: | ||
| - '.github/workflows/test.yml' | ||
| - 'src/**' | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,32 @@ describe('hooks/types', () => { | |
| expect(error.hint).toBe('Check the key.') | ||
| }) | ||
|
|
||
| it('keeps cause non-enumerable to match native ES2022 Error semantics', () => { | ||
| const cause = new Error('native failure') | ||
| const error = new HookError('Wrapper message', { cause }) | ||
|
|
||
| const descriptor = Object.getOwnPropertyDescriptor(error, 'cause') | ||
| expect(descriptor).toBeDefined() | ||
| expect(descriptor?.enumerable).toBe(false) | ||
| expect(Object.keys(error)).not.toContain('cause') | ||
| expect(JSON.parse(JSON.stringify(error))).not.toHaveProperty('cause') | ||
| }) | ||
|
|
||
| it('omits cause when not provided', () => { | ||
| const error = new HookError('Wrapper message') | ||
| expect(Object.hasOwn(error, 'cause')).toBe(false) | ||
| }) | ||
|
Comment on lines
+22
to
+36
|
||
|
|
||
| it('defines a non-enumerable own cause when explicitly passed as undefined', () => { | ||
| const error = new HookError('Wrapper message', { cause: undefined }) | ||
| const descriptor = Object.getOwnPropertyDescriptor(error, 'cause') | ||
| expect(Object.hasOwn(error, 'cause')).toBe(true) | ||
| expect(error.cause).toBeUndefined() | ||
| expect(descriptor).toBeDefined() | ||
| expect(descriptor?.enumerable).toBe(false) | ||
| expect(Object.keys(error)).not.toContain('cause') | ||
| }) | ||
|
|
||
| it('creates the initial async state', () => { | ||
| const state = createInitialAsyncState<string>() | ||
| expect(state).toEqual({ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,6 +62,16 @@ export class SensitiveInfoError extends Error { | |
| /** Stable discriminant identifying the failure mode. */ | ||
| readonly code: ErrorCodeValue | ||
|
|
||
| /** | ||
| * The underlying cause forwarded to {@link Error.cause}. | ||
| * | ||
| * Declared as a type-only member so the property type-checks under `tsconfig` | ||
| * `lib` targets that predate ES2022 (where {@link Error} did not yet expose | ||
| * `cause`). At runtime the value is installed by the constructor via | ||
| * {@link Object.defineProperty} so it stays non-enumerable. | ||
| */ | ||
| declare readonly cause?: unknown | ||
|
|
||
| /** | ||
| * @param code - Stable {@link ErrorCodeValue} for the failure. | ||
| * @param message - Human-readable description. | ||
|
|
@@ -75,11 +85,17 @@ export class SensitiveInfoError extends Error { | |
| super(message) | ||
| this.name = 'SensitiveInfoError' | ||
| this.code = code | ||
| // Assign `cause` directly instead of passing it to `super()` so this | ||
| // Define `cause` manually instead of passing it to `super()` so this | ||
| // compiles cleanly under TS configs whose `lib` predates ES2022 (where | ||
| // the second `Error` constructor argument was introduced). | ||
| // the second `Error` constructor argument was introduced), while keeping | ||
| // the property non-enumerable to match the native ES2022 `Error` constructor. | ||
| if (options && 'cause' in options) { | ||
| ;(this as { cause?: unknown }).cause = options.cause | ||
| Object.defineProperty(this, 'cause', { | ||
| value: options.cause, | ||
| writable: true, | ||
| configurable: true, | ||
| enumerable: false, | ||
| }) | ||
| } | ||
|
Comment on lines
85
to
99
|
||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,23 +30,40 @@ export class HookError extends Error { | |
| /** UI-facing remediation hint (e.g. `'Ask the user to retry biometrics.'`). */ | ||
| readonly hint?: string | undefined | ||
|
|
||
| /** | ||
| * The underlying cause forwarded to {@link Error.cause}. | ||
| * | ||
| * Declared as a type-only member so the property type-checks under `tsconfig` | ||
| * `lib` targets that predate ES2022 (where {@link Error} did not yet expose | ||
| * `cause`). At runtime the value is installed by the constructor via | ||
| * {@link Object.defineProperty} so it stays non-enumerable. | ||
| */ | ||
| declare readonly cause?: unknown | ||
|
|
||
| /** | ||
| * @param message - Human-readable description of the failure. | ||
| * @param options - Additional metadata; see {@link HookErrorOptions}. | ||
| */ | ||
| constructor( | ||
| message: string, | ||
| { cause, operation, hint }: HookErrorOptions = {} | ||
| ) { | ||
| constructor(message: string, options: HookErrorOptions = {}) { | ||
| super(message) | ||
| this.name = 'HookError' | ||
| this.operation = operation | ||
| this.hint = hint | ||
| // Assign `cause` directly instead of passing it to `super()` so this | ||
| this.operation = options.operation | ||
| this.hint = options.hint | ||
| // Define `cause` manually instead of passing it to `super()` so this | ||
| // compiles cleanly under TS configs whose `lib` predates ES2022 (where | ||
| // the second `Error` constructor argument was introduced). | ||
| if (cause !== undefined) { | ||
| ;(this as { cause?: unknown }).cause = cause | ||
| // the second `Error` constructor argument was introduced), while keeping | ||
| // the property non-enumerable to match the native ES2022 `Error` constructor. | ||
|
Comment on lines
49
to
+55
|
||
| // | ||
| // We deliberately use `'cause' in options` (rather than a value check) | ||
| // so that `new HookError(msg, { cause: undefined })` still installs the | ||
| // property — matching native `new Error(msg, { cause: undefined })`. | ||
| if ('cause' in options) { | ||
| Object.defineProperty(this, 'cause', { | ||
| value: options.cause, | ||
| writable: true, | ||
| configurable: true, | ||
| enumerable: false, | ||
| }) | ||
| } | ||
| } | ||
| } | ||
|
|
||
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.
Consider adding a test for the ES2022 edge case where options explicitly include
{ cause: undefined }. NativeErrorstill defines an own (non-enumerable)causeproperty in that case, and covering it here would make the intended semantics forSensitiveInfoErrorexplicit.