Updates#9
Conversation
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the deep equality comparison logic to improve maintainability and adds specialized handling for Error objects and WeakMap/WeakSet collections. The refactoring extracts object comparison logic into separate focused functions while adding support for comparing Error objects by their name, message, and cause properties.
Changes:
- Refactored object comparison by extracting
compareObjectsandequalPlainObjecthelper functions, improving code organization and readability - Added
equalErrorfunction to compare Error objects by name, message, and cause (recursively) - Added special handling for WeakMap and WeakSet to return false for different instances (only same reference is equal)
- Updated TypeScript target to ES2022 to support Error.cause property
- Added build step to CI workflow before publishing
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/index.ts | Refactored comparison logic by extracting compareObjects and equalPlainObject functions; added equalError for Error comparison; added WeakMap/WeakSet handling |
| test/objects.spec.ts | Added comprehensive test cases for Error object equality comparison covering same/different messages, types, causes, and deep causes |
| test/weakmap-weakset.spec.ts | Added new test file verifying WeakMap and WeakSet comparison behavior (same reference vs different instances) |
| tsconfig.json | Updated target from ES2020 to ES2022 to support Error.cause property |
| .github/workflows/ci.yml | Added build step before npm publish to ensure code is built before release |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (left.toString !== Object.prototype.toString) { | ||
| return left.toString() === right.toString(); | ||
| } | ||
| if (left instanceof WeakMap || left instanceof WeakSet) { |
There was a problem hiding this comment.
The WeakMap/WeakSet check is inconsistent with the pattern used for other types in this function. All other instanceof checks (Map, Set, Error, etc.) verify both left and right parameters, but this check only verifies left. While technically correct due to the constructor equality check on line 16, this inconsistency makes the code harder to understand and maintain. Consider changing to if ((left instanceof WeakMap && right instanceof WeakMap) || (left instanceof WeakSet && right instanceof WeakSet)) to match the established pattern.
| if (left instanceof WeakMap || left instanceof WeakSet) { | |
| if ((left instanceof WeakMap && right instanceof WeakMap) || (left instanceof WeakSet && right instanceof WeakSet)) { |
| it.each([ | ||
| { | ||
| description: 'errors with same message are equal', | ||
| value1: new Error('a'), | ||
| value2: new Error('a'), | ||
| expected: true, | ||
| }, | ||
| { | ||
| description: 'errors with different message are not equal', | ||
| value1: new Error('a'), | ||
| value2: new Error('b'), | ||
| expected: false, | ||
| }, | ||
| { | ||
| description: 'TypeError and Error with same message are not equal', | ||
| value1: new TypeError('a'), | ||
| value2: new Error('a'), | ||
| expected: false, | ||
| }, | ||
| { | ||
| description: 'errors with same cause are equal', | ||
| value1: new Error('a', { cause: 'x' }), | ||
| value2: new Error('a', { cause: 'x' }), | ||
| expected: true, | ||
| }, | ||
| { | ||
| description: 'errors with different cause are not equal', | ||
| value1: new Error('a', { cause: 'x' }), | ||
| value2: new Error('a', { cause: 'y' }), | ||
| expected: false, | ||
| }, | ||
| { | ||
| description: 'errors with deep cause are equal', | ||
| value1: new Error('a', { cause: { n: 1 } }), | ||
| value2: new Error('a', { cause: { n: 1 } }), | ||
| expected: true, | ||
| }, | ||
| ])('$description', ({ expected, value1, value2 }) => { | ||
| expect(equal(value1, value2)).toBe(expected); | ||
| }); |
There was a problem hiding this comment.
The error comparison tests are missing a test case for when one error has a cause property and the other doesn't (e.g., new Error('a', { cause: 'x' }) vs new Error('a')). Adding this test case would improve coverage and verify that the comparison correctly handles this edge case.
| /** | ||
| * Check if errors are equal. | ||
| */ | ||
| function equalError(left: Error, right: Error, seen: Seen): boolean { | ||
| return ( | ||
| left.message === right.message && | ||
| left.name === right.name && | ||
| compareValues(left.cause, right.cause, seen) | ||
| ); | ||
| } |
There was a problem hiding this comment.
The equalError function only compares the message, name, and cause properties of Error objects, ignoring any custom properties that may have been added to the Error instances. While this appears to be an intentional design choice to treat Errors as value objects, it creates inconsistent behavior compared to how plain objects are handled (where all properties are compared). Consider documenting this behavior in the function's JSDoc comment to make the design decision explicit.
This pull request improves the deep equality comparison logic in the codebase by refactoring the main comparison function, adding special handling for
Error,WeakMap, andWeakSetobjects, and introducing new tests to verify these behaviors. The changes enhance correctness and maintainability, especially for edge cases involving error objects and weak collections.Equality comparison improvements
compareObjectsfunction and moving plain object key/value checks to a newequalPlainObjecthelper. This makes the code clearer and easier to maintain. [1] [2] [3]WeakMapandWeakSetinstances so that they are only considered equal if they are the same reference; otherwise, they are not comparable and always return false.equalErrorhelper to compareErrorobjects by theirname,message, and (recursively) theircauseproperty, improving correctness for error comparisons. [1] [2]Testing enhancements
WeakMapandWeakSetcomparison behavior, verifying that only identical references are considered equal and that distinct instances are not comparable.CI workflow update
.github/workflows/ci.yml) before publishing the package, ensuring that the code is properly built prior to release.