Make Error 'cause' non-enumerable and add tests#599
Conversation
Define the `cause` property with Object.defineProperty in SensitiveInfoError and HookError so it remains non-enumerable (matching native ES2022 Error semantics) while keeping compatibility with TS libs predating ES2022. Add tests to verify cause chaining, non-enumerability, and omission when not provided. Also update CI workflow triggers to use the 'master' branch for android, ios and test workflows.
There was a problem hiding this comment.
Pull request overview
Updates custom error classes and CI workflows to align more closely with ES2022 Error cause behavior and to ensure CI runs on the repository’s primary branch.
Changes:
- Define
causeviaObject.definePropertyinSensitiveInfoErrorandHookErrorto keep it non-enumerable and only defined when intended. - Add Jest coverage verifying
causeretention, non-enumerability, omission when absent, and subclass propagation. - Update GitHub Actions workflow push-branch filters from
maintomaster.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/hooks/types.ts |
Adjusts HookError cause definition to be non-enumerable. |
src/errors.ts |
Adjusts SensitiveInfoError cause definition to be non-enumerable and conditionally defined. |
src/__tests__/hooks.types.test.ts |
Adds tests for HookError cause enumerability/omission. |
src/__tests__/errors.test.ts |
Adds tests for SensitiveInfoError cause chaining and enumerability/omission. |
.github/workflows/test.yml |
Switches push trigger branch from main to master. |
.github/workflows/ios-build.yml |
Switches push trigger branch from main to master. |
.github/workflows/android-build.yml |
Switches push trigger branch from main to master. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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) | ||
| }) |
There was a problem hiding this comment.
These tests assert non-enumerability and omission when no options are passed, but they don’t cover the ES2022 edge case of explicitly providing { cause: undefined }. Native Error still defines an own non-enumerable cause property in that case; adding a test for it would prevent regressions and clarify intended semantics for HookError.
| 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, | ||
| }) | ||
| } |
There was a problem hiding this comment.
If “TS libs predating ES2022” compatibility is a goal, consumers compiling with older lib targets won’t see cause on Error, and SensitiveInfoError doesn’t declare it either. Consider adding a type-only member declare readonly cause?: unknown to SensitiveInfoError so err.cause type-checks without emitting a real class field (which would otherwise create an enumerable own property and defeat the non-enumerability change).
| const err = new SensitiveInfoError(ErrorCode.NotFound, 'wrapped') | ||
| expect(Object.hasOwn(err, 'cause')).toBe(false) | ||
| }) | ||
|
|
There was a problem hiding this comment.
Consider adding a test for the ES2022 edge case where options explicitly include { cause: undefined }. Native Error still defines an own (non-enumerable) cause property in that case, and covering it here would make the intended semantics for SensitiveInfoError explicit.
| it('defines a non-enumerable own cause when explicitly passed as undefined', () => { | |
| const err = new SensitiveInfoError(ErrorCode.NotFound, 'wrapped', { | |
| cause: undefined, | |
| }) | |
| const descriptor = Object.getOwnPropertyDescriptor(err, 'cause') | |
| expect(err.cause).toBeUndefined() | |
| expect(Object.hasOwn(err, 'cause')).toBe(true) | |
| expect(descriptor).toBeDefined() | |
| expect(descriptor?.enumerable).toBe(false) | |
| expect(Object.keys(err)).not.toContain('cause') | |
| }) |
| writable: true, | ||
| configurable: true, | ||
| enumerable: false, | ||
| }) |
There was a problem hiding this comment.
HookError claims to match native ES2022 Error cause semantics, but the current destructuring + cause !== undefined check cannot distinguish between “no cause provided” and { cause: undefined }. Native Error(message, { cause: undefined }) defines an own cause property (value undefined) that is still non-enumerable. Consider accepting an options object (no destructuring in the parameter list) and checking options && 'cause' in options before calling Object.defineProperty to align with ES2022 behavior.
| 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. |
There was a problem hiding this comment.
If the goal is compatibility with TS libs predating ES2022, consumers on older lib targets won’t have Error['cause'] in their type definitions, and HookError doesn’t declare it either. To preserve typing without changing runtime enumerability, add a type-only class member like declare readonly cause?: unknown (avoid a real class field, which would emit JS and create an enumerable own property).
Declare a type-only readonly `cause?: unknown` on HookError and SensitiveInfoError so TS configs that predate ES2022 type-check. Update HookError to accept an options object, install the `cause` property via Object.defineProperty when `'cause' in options` (so passing { cause: undefined } still creates a non-enumerable own property), and wire up operation/hint from the options. Add tests for HookError and SensitiveInfoError that assert a non-enumerable own `cause` is defined when explicitly passed as undefined. Also remove react-native-specific exports from package.json.
This pull request updates workflow branch targets and improves the handling of error causes in custom error classes to better align with ES2022 Error semantics. The most significant changes include switching workflow triggers from the
mainto themasterbranch and ensuring that thecauseproperty on custom errors is non-enumerable and only defined when provided, matching native JavaScript behavior.Workflow configuration updates:
.github/workflows/android-build.yml,.github/workflows/ios-build.yml, and.github/workflows/test.ymlto use themasterbranch instead ofmain. [1] [2] [3]Error handling improvements:
SensitiveInfoErrorinsrc/errors.tsandHookErrorinsrc/hooks/types.tsto define thecauseproperty usingObject.defineProperty, making it non-enumerable and only present when actually provided, to match native ES2022Errorbehavior. [1] [2]Testing enhancements:
src/__tests__/errors.test.tsandsrc/__tests__/hooks.types.test.tsto verify that thecauseproperty is correctly handled—ensuring it is retained, non-enumerable, omitted when not given, and propagated through subclasses. [1] [2]Define thecauseproperty with Object.defineProperty in SensitiveInfoError and HookError so it remains non-enumerable (matching native ES2022 Error semantics) while keeping compatibility with TS libs predating ES2022. Add tests to verify cause chaining, non-enumerability, and omission when not provided. Also update CI workflow triggers to use the 'master' branch for android, ios and test workflows.