-
-
Notifications
You must be signed in to change notification settings - Fork 236
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 1 commit
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,22 @@ 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('creates the initial async state', () => { | ||
| const state = createInitialAsyncState<string>() | ||
| expect(state).toEqual({ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,11 +75,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 |
|---|---|---|
|
|
@@ -42,11 +42,17 @@ export class HookError extends Error { | |
| this.name = 'HookError' | ||
| this.operation = operation | ||
| this.hint = hint | ||
| // 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. | ||
|
Comment on lines
49
to
+55
|
||
| if (cause !== undefined) { | ||
| ;(this as { cause?: unknown }).cause = cause | ||
| Object.defineProperty(this, 'cause', { | ||
| value: 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.