feat: print AggregateError errors stack #98
Conversation
WalkthroughThe pull request introduces several modifications across various files. The GitHub Actions workflow is updated to include a specific Node.js version, while the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
test/lib/utils.test.js (4)
Line range hint
6-7: Consider using a more precise version check for AggregateError supportThe current check allows Node.js 15, but
AggregateErrorwas standardized in Node.js 16. Consider updating the version check to be more precise:-if (NODE_MAJOR_VERSION < 16) return; +// AggregateError is standardized in Node.js 16+ +if (NODE_MAJOR_VERSION < 16) { + console.log('Skipping AggregateError tests: requires Node.js 16+'); + return; +}
149-184: Well-structured test case with good coverage!The test effectively validates the formatting of AggregateError with a good mix of error types and nested causes.
Consider adding these additional test cases to improve coverage:
it('should handle edge cases in AggregateError', () => { const emptyError = new AggregateError([]); const nullError = new AggregateError([null]); const undefinedError = new AggregateError([undefined]); const msg1 = formatError(emptyError); assert(msg1.match(/AggregateError.*no errors/)); const msg2 = formatError(nullError); assert(msg2.match(/AggregateError.*invalid error/)); const msg3 = formatError(undefinedError); assert(msg3.match(/AggregateError.*invalid error/)); });
150-151: Consider removing eslint-disable by adding proper type handlingInstead of disabling eslint, consider adding proper type handling:
+// @ts-check +/** @type {typeof globalThis.AggregateError} */ +const AggregateError = global.AggregateError; -// eslint-disable-next-line no-undef const rootError = new AggregateError([
176-183: Consider enhancing assertions for more precise validationWhile regex matching is flexible, consider adding more precise assertions:
assert(msg.match(/nodejs.AggregateError: no_message/)); +// Verify exact number of errors +assert((msg.match(/\[error-\d+]:/g) || []).length === 2); +// Verify error order +const errorIndexes = msg.match(/\[error-(\d+)]:/g).map(m => parseInt(m.match(/\d+/)[0])); +assert.deepStrictEqual(errorIndexes, [0, 1]); assert(msg.match(/\[error-0]:/));lib/utils.js (1)
228-238: Consider improving AggregateError formatting readabilityWhile the implementation correctly handles AggregateError and its sub-errors, the formatting could be enhanced for better readability.
Consider this improvement:
if (err.name === 'AggregateError' && err.errors) { for (let i = 0; i < err.errors.length; i++) { const subError = err.errors[i]; const subErrorMsg = errorToString(subError, options, causeLength + 1); - errorString = util.format('%s\n[error-%d]:\n\n%s', + errorString = util.format('%s\n\n[error-%02d]:\n\n%s', errorString, i, subErrorMsg ); } }This change:
- Adds padding to error indices for consistent alignment
- Adds an extra newline before each sub-error for better visual separation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
.github/workflows/nodejs.yml(1 hunks).gitignore(1 hunks)README.md(1 hunks)README.zh-CN.md(1 hunks)lib/utils.js(3 hunks)package.json(0 hunks)test/lib/utils.test.js(1 hunks)
💤 Files with no reviewable changes (1)
- package.json
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- README.md
- README.zh-CN.md
🔇 Additional comments (7)
.github/workflows/nodejs.yml (1)
16-16: Consider removing the specific version 14.18.0
Since we're already testing on Node.js 14, testing on the specific patch version 14.18.0 might be redundant and increases CI time. Additionally, since this PR handles AggregateError support (introduced in Node.js 15.0.0), we should verify if testing on Node.js 14.x is necessary at all.
Let's verify the minimum Node.js version required in package.json:
lib/utils.js (6)
14-14: LGTM: Variable name correction
The variable name has been correctly spelled as durationRegexp.
91-91: LGTM: Consistent usage of renamed variable
The durationRegexp variable is correctly used in the console formatter.
203-205: LGTM: Added recursion protection
The max cause chain length check prevents infinite recursion when handling circular error references.
207-209: LGTM: Improved error name and message handling
The error name and message handling is more robust, with proper fallbacks and code inclusion.
Also applies to: 212-212
241-246: LGTM: Improved cause chain formatting
The cause chain formatting is consistent with the overall error formatting style.
228-238: Verify AggregateError handling with test cases
The implementation looks correct, but let's verify it with test cases.
✅ Verification successful
AggregateError handling implementation is verified with test coverage
The implementation is properly tested in test/lib/utils.test.js which verifies:
- Formatting of AggregateError with multiple nested errors
- Correct display of error indices (
[error-0],[error-1]) - Proper handling of different error types within the aggregate (Error, TypeError)
- Error cause propagation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files that verify AggregateError handling
rg -l "AggregateError" --type test
# Search for example usage or documentation
rg -l "AggregateError" --type markdown
Length of output: 102
Script:
#!/bin/bash
# Search for test files with common test file extensions
rg -l "AggregateError" "**/*test*" "**/*spec*"
# Search for example usage in documentation files
rg -l "AggregateError" "**/*.md"
# Search for actual usage of AggregateError in the codebase
rg -C 3 "AggregateError"
Length of output: 1886
[skip ci] ## [3.6.0](v3.5.0...v3.6.0) (2024-11-30) ### Features * print AggregateError errors stack ([#98](#98)) ([03c38bd](03c38bd))
closes eggjs/egg#5370
Summary by CodeRabbit
Release Notes
New Features
AggregateErrorinstances in error formatting.Bug Fixes
Chores
.gitignorefile to includepackage-lock.json.git-contributordependency frompackage.json.Documentation