Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe AppError constructor now accepts an optional ErrorOptions parameter and forwards it to the native Error via super(message, options). Documentation (README) was updated to list the new options payload field and tests were adjusted to assert an Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant AppError
participant NativeError as Error
Caller->>AppError: new AppError(message, options?)
Note right of AppError: normalize options (null -> undefined)
AppError->>NativeError: super(message, optionsOrUndefined)
NativeError-->>AppError: constructed Error (with .cause if provided)
AppError-->>Caller: returns AppError instance (includes .message, .options)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🧰 Additional context used🪛 markdownlint-cli2 (0.18.1)README.md29-29: Unordered list indentation (MD007, ul-indent) 29-29: Bare URL used (MD034, no-bare-urls) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull Request Overview
Adds support for ES2022 Error options to AppError to enable error chaining via cause, with corresponding test and README updates.
- AppError constructor now accepts an options property and forwards it to super(message, options).
- Introduces JSDoc typedef for ErrorOptions and updates constructor JSDoc.
- Updates unit tests and README to include the options parameter.
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/app-error.js | Adds ErrorOptions typedef and forwards options to the Error constructor. |
| src/tests/app-error.unit.js | Adds options in test cases, including a cause example. |
| README.md | Documents the new options parameter and links to MDN. |
src/app-error.js
Outdated
| constructor ({ value = null, type = null, message, me = null }) { | ||
| super(message) | ||
| constructor ({ value = null, type = null, message, me = null, options }) { | ||
| super(message, options) |
There was a problem hiding this comment.
Passing options: null (as done in the updated tests) to super(message, options) can throw a TypeError in environments that implement ES2022 Error options because ToObject(null) is invalid. Coalesce null to undefined before forwarding to super to maintain compatibility: e.g., compute const opts = options == null ? undefined : options and call super(message, opts).
| super(message, options) | |
| const opts = options == null ? undefined : options | |
| super(message, opts) |
| me: null, | ||
| options: null |
There was a problem hiding this comment.
options: null is not a valid ErrorOptions per the ES2022 Error constructor and may cause a TypeError in modern runtimes; use undefined or omit the property instead. Replace with options: undefined, or remove the options field entirely.
| me: null, | |
| options: null | |
| me: null |
| me: AppError, | ||
| options: { cause: new Error('Root cause') } | ||
| }) | ||
|
|
||
| assert.deepEqual(error instanceof AppError, true) |
There was a problem hiding this comment.
The test does not assert that the cause is propagated, which is the core behavior introduced by this PR. Add an assertion such as assert.equal(error.cause?.message, 'Root cause') and, in the tests where options is omitted/undefined, assert that error.cause is undefined.
| * value (type: mixed) | ||
| * type (String|Array|Object|Number|Boolean|Date|...) | ||
| * me (current method, no validation on the value) | ||
| * options (ErrorOptions, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/Error) |
There was a problem hiding this comment.
Clarify that options must be an object (not null) and that when provided with a cause, it will be available on error.cause. Consider adding a short example: new AppError({ message: '...', options: { cause: new Error('reason') } }) -> error.cause.message === 'reason'.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/__tests__/app-error.unit.js (1)
67-86: Add assertion to verify error.cause is set correctly.The test creates an AppError with
options: { cause: new Error('Root cause') }but doesn't verify that thecausewas actually set on the error instance. This means the primary functionality of the new feature (error chaining) is not being tested.Add an assertion after line 85 to verify the cause:
assert.deepEqual(error.me, AppError) + assert.deepEqual(error.cause instanceof Error, true) + assert.deepEqual(error.cause.message, 'Root cause') })This ensures that the ES2022 ErrorOptions are properly propagated to the native Error constructor.
🧹 Nitpick comments (2)
README.md (1)
64-92: Consider adding an example demonstrating the new options parameter.The documentation now mentions the
optionsparameter, but the example usage doesn't demonstrate how to use it with error chaining (the primary use case for this feature).Consider adding an example that shows error chaining with
cause:doSomethingWrong () { try { // Some operation that might fail } catch (originalError) { throw new NotFoundError({ value: 'test', type: String, message: 'Example text', me: this.constructor, options: { cause: originalError } }); } }This would help users understand the practical application of the new feature.
src/__tests__/app-error.unit.js (1)
25-44: Consider adding a negative test for null options.While this test verifies that passing
options: nulldoesn't break the constructor, it doesn't explicitly assert thaterror.causeis undefined in this case.For completeness, you could add an assertion:
assert.deepEqual(error.me, null) + assert.deepEqual(error.cause, undefined) })This would document the expected behavior when options is explicitly null.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
README.md(1 hunks)src/__tests__/app-error.unit.js(3 hunks)src/app-error.js(2 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
README.md
29-29: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
29-29: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (2)
src/app-error.js (2)
7-11: LGTM! Proper ES2022 ErrorOptions typedef.The typedef correctly documents the ErrorOptions interface with an optional
causeproperty, aligning with the ES2022 Error constructor specification.
35-36: LGTM! Options correctly passed to native Error constructor.The implementation properly:
- Accepts
optionsin the destructured parameter (will beundefinedif not provided)- Passes it to the
super(message, options)call, enabling ES2022 error chainingThe native Error constructor will handle storing the
causeon the instance, so it's correctly accessible viaerror.causewithout needing to explicitly store it insetValuesor include it in thevaluesgetter.
| * value (type: mixed) | ||
| * type (String|Array|Object|Number|Boolean|Date|...) | ||
| * me (current method, no validation on the value) | ||
| * options (ErrorOptions, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/Error) |
There was a problem hiding this comment.
Fix markdown formatting issues.
The line has two formatting issues flagged by markdownlint:
- List indentation is incorrect (should be 0 spaces, currently 1)
- Bare URL should be formatted as a proper link
Apply this diff to fix the formatting:
- * options (ErrorOptions, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/Error)
+* options (ErrorOptions, see <https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/Error>)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * options (ErrorOptions, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/Error) | |
| * options (ErrorOptions, see <https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/Error>) |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
29-29: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
29-29: Bare URL used
(MD034, no-bare-urls)
🤖 Prompt for AI Agents
In README.md around line 29, the list item has incorrect indentation and a bare
URL; fix by removing the leading space so the asterisk is at column 0 and
replace the raw URL with a proper markdown link (e.g. ErrorOptions linking to
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/Error)
so the line reads a properly indented list item with a formatted link.
| /** | ||
| * Options for the native Error constructor (ES2022). | ||
| * @typedef {object} ErrorOptions | ||
| * @property {unknown} [cause] | ||
| */ | ||
|
|
There was a problem hiding this comment.
This local JSDoc typedef name 'ErrorOptions' conflicts with the global ES2022 'ErrorOptions' type in TypeScript/checkJs environments, causing duplicate identifier errors. Remove this typedef and reference the built-in 'ErrorOptions' in JSDoc, or rename it (e.g., 'AppErrorOptions') and update the constructor JSDoc to use that new name.
| * @param {object=} error.type | ||
| * @param {string} error.message | ||
| * @param {object=} error.me | ||
| * @param {ErrorOptions=} error.options |
There was a problem hiding this comment.
JSDoc states 'options' is of type ErrorOptions (object), but tests pass null. Clarify the accepted types to avoid confusion and checkJs complaints. Suggest updating to: '@param {ErrorOptions|undefined|null=} error.options'.
| * @param {ErrorOptions=} error.options | |
| * @param {ErrorOptions|undefined|null=} error.options |
| value: 'test', | ||
| type: String, | ||
| message: 'Example text', | ||
| me: AppError | ||
| me: AppError, | ||
| options: { cause: new Error('Root cause') } | ||
| }) |
There was a problem hiding this comment.
The new behavior (options.cause propagation) isn't asserted. Add expectations to verify that 'error.cause' is set when provided and absent otherwise, e.g., 'assert.equal(error.cause instanceof Error, true)' and 'assert.equal(error.cause.message, 'Root cause')' for this case, and 'assert.equal(error.cause, undefined)' in the null/undefined options cases.
Extends the AppError class to accept an options object, aligning with the native Error constructor (ES2022) for enhanced error handling capabilities. This enhancement allows for specifying a 'cause' property, enabling the chaining of errors and providing more contextual information for debugging and error analysis. Updates related unit tests to reflect the added 'options' property.
18443c6 to
2af3dd8
Compare
| const opts = options == null ? undefined : options | ||
| super(message, opts) |
There was a problem hiding this comment.
[nitpick] Avoid loose equality and the temporary variable; use nullish coalescing inline. For example: super(message, options ?? undefined). This keeps the intent (treat null/undefined as absent) while remaining concise and avoiding ==.
| const opts = options == null ? undefined : options | |
| super(message, opts) | |
| super(message, options ?? undefined) |
Extends the AppError class to accept an options object, aligning with the native Error constructor (ES2022) for enhanced error handling capabilities.
This enhancement allows for specifying a 'cause' property, enabling the chaining of errors and providing more contextual information for debugging and error analysis.
Updates related unit tests to reflect the added 'options' property.
Summary by CodeRabbit
New Features
Documentation
Tests