| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
There was a problem hiding this comment.
This is already looking very promising!
The documentation still needs an update (we describe what properties we check in addition to enumerable ones) and I left improvement suggestions for edge cases.
Sorry, something went wrong.
There was a problem hiding this comment.
The instanceof check will work in most cases while it would not detect AggregateErrors created in a different context/realm (e.g., by creating errors in a context created by vm.createContext()). I believe it is fine to just always check for this property, in case it is not already an enumerable property.
This check would currently miss the case, if either side is an AggregateError and the other is not. That could be addressed in a similar way as I suggested above about the enumerable property.
Sorry, something went wrong.
There was a problem hiding this comment.
This is definitely the right check as long the property is non-enumerable. If it is enumerable, we'd do the check twice and this could be costly and a lot of errors are created in the wild with a cause property set as an enumerable one.
In addition, we should also verify that the properties have the same enumerability. We don't yet do that for name and message, but that would be beneficial.
const cause1Enumerable = ObjectPrototypePropertyIsEnumerable(val1, 'cause');
if (cause1Enumerable !== ObjectPrototypePropertyIsEnumerable(val2, 'cause') ||
!cause1Enumerable &&
!innerDeepEqual(val1.cause, val2.cause, strict, memos)) {
return false;
}
Sorry, something went wrong.
There was a problem hiding this comment.
Nit (just a general suggestion without need to follow-up upon): the variable names could be changed in a way that it's clearer what they stand for. E.g., this one could be:
| const e6 = new AggregateError([e1, e2], 'Aggregate Error'); | |
| const e4duplicate = new AggregateError([e1, e2], 'Aggregate Error'); |
That way it would be easy to understand in the comparison that the entries are meant to be identical.
Sorry, something went wrong.
|
@BridgeAR Thanks for your suggestion! |
Sorry, something went wrong.
|
@BridgeAR, I have update based on Non-Enumerable properties on Error, PTAL feel free |
Sorry, something went wrong.
There was a problem hiding this comment.
As far as I can tell, this means now we only compare the name and message fields when they are enumerable, which specificaly opposite from the documentation, and is a breaking change...
Sorry, something went wrong.
There was a problem hiding this comment.
As far as I can tell, this means now we only compare the name and message fields when they are enumerable, which specificaly opposite from the documentation, and is a breaking change...
FWIK, As mentioned in ECMAScript 2025, name, message, cause and errors are all non-enumerable, so in the judgement above, I just compared the attributes whether they have same enumerable property, if it's same and they are non-enumerable, then compare the attributes contents.
So if the attributes are enumerable(for example in a custom Error), the next check keyCheck will check them, in the doc change, I have just added errors, and causes attributes, and didn't change name and message, I think its doesn't break the doc, have I missed something?
Sorry, something went wrong.
There was a problem hiding this comment.
If it was a breaking change, I would expect we would see test failures.
Sorry, something went wrong.
There was a problem hiding this comment.
Would you mind adding an item to the change list in the YAML frontmatter as well? Such as:
<!-- YAML
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/51805
description: Error cause property is now compared as well.
--->
Sorry, something went wrong.
There was a problem hiding this comment.
assertDeepAndStrictEqual only accepts two arguments.
| assertDeepAndStrictEqual(e1, new Error('err', { cause: new Error('cause e1') }), AssertionError); | |
| assertDeepAndStrictEqual(e1, new Error('err', { cause: new Error('cause e1') })); |
Sorry, something went wrong.
There was a problem hiding this comment.
| assertDeepAndStrictEqual(e3, e3duplicate, AssertionError); | |
| assertDeepAndStrictEqual(e3, e3duplicate); |
Sorry, something went wrong.
There was a problem hiding this comment.
I believe the actual behavior unconditionally compares the errors property regardless of whether the error instance is an AggregateError or not. This could be reworded as
* [`Error`][] names, messages, causes, and errors are always compared, even if these are not enumerable properties.
Sorry, something went wrong.
There was a problem hiding this comment.
Resolved. Thanks for the reminder
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
PR-URL: nodejs#51805 Fixes: nodejs#51793 Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
| Back | FazBrowse Home | New Git URL |
add deep equal check for the cases below:
Fixes: #51793