feat: add structured error handling with class error hierarchy #1320
Conversation
|
@mahmoud-ghalayini Thanks for submitting such a BIG pr 😄 will start reviewing it asap. |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a comprehensive structured error handling system for rclnodejs, replacing generic JavaScript errors (TypeError, RangeError, Error) with specialized error classes organized in a clear hierarchy. The changes include 21 error classes extending from a base RclNodeError, complete TypeScript definitions, comprehensive tests, and example documentation.
Key changes:
- New error class hierarchy with base
RclNodeErrorand specialized subclasses (ValidationError, OperationError, ParameterError, TopicError, ActionError, NativeError) - Updated all error throwing sites across the codebase to use new error classes
- Added TypeScript type definitions for all error classes
- Comprehensive test suite with 100+ test cases
- Example code and documentation demonstrating error handling patterns
Reviewed Changes
Copilot reviewed 41 out of 43 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/errors.js | Core implementation of 21 error classes with structured context |
| types/errors.d.ts | TypeScript definitions for all error classes |
| test/test-errors.js | Comprehensive test suite covering all error classes |
| index.js | Exports all error classes to public API |
| lib/*.js | Updated 15+ files to use new error classes |
| test/*.js | Updated test assertions to expect new error types |
| example/error-handling/* | Examples and documentation for error handling patterns |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
minggangw
left a comment
minggangw
left a comment
There was a problem hiding this comment.
lgtm, please go ahead to merge it when comments addressed as the fix is straightforward, thanks!
mahmoud-ghalayini
force-pushed
the
feat-add-structured-error-handling-with-error-class-hierarchy-1319
branch
from
9cad456 to
9602c40
Compare
|
Thanks for the comments, all are fixed. but can we review again just in case 😆 @minggangw |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 41 out of 43 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
minggangw
left a comment
minggangw
left a comment
There was a problem hiding this comment.
I re-triggered the copilot and it found another 3 issues, please help to fix them. Also, I reviewed it manually, the rest seems good. We could split the PR into small ones in the future to facilitate the review work.
LGTM, thanks for refactoring the error hierarchy, I believe it helps the developers to identify where goes wrong easily and quickly!
mahmoud-ghalayini
force-pushed
the
feat-add-structured-error-handling-with-error-class-hierarchy-1319
branch
from
9602c40 to
3adc9f0
Compare
3 participants
Public API Changes
Added 21 new error classes (all exported from
rclnodejs):Base:
RclNodeError- Base class with rich context (code, nodeName, entityType, timestamp, details, toJSON())Validation Errors:
ValidationErrorTypeValidationError(includesargumentName,providedValue,expectedType)RangeValidationError(includesargumentName,providedValue,validationRule)NameValidationError(includesinvalidName,nameType)Operation Errors:
OperationErrorTimeoutError(includestimeoutMs)AbortError(includesreason)ServiceNotFoundError(includesserviceName)NodeNotFoundError(includesnodeName)Parameter Errors:
ParameterErrorParameterNotFoundError(includesparameterName,remoteNodeName)ParameterTypeError(includesparameterName,expectedType,actualType)ReadOnlyParameterError(includesparameterName)Topic/Action Errors:
TopicError,PublisherError,SubscriptionErrorActionError,GoalRejectedError,ActionServerNotFoundErrorNative Errors:
NativeError(wraps C++ errors withnativeMessage,operation)Breaking (but backward compatible):
TypeError/Error/RangeErrorError, so existingcatch (error)blocks still workcatch (error) { if (error instanceof rclnodejs.TimeoutError) ... })1319