feat: change error comparison algorithm #57
Conversation
| var leftHandKeys = getEnumerableKeys(leftHandOperand); | ||
| var rightHandKeys = getEnumerableKeys(rightHandOperand); | ||
| if (leftHandType === 'Error') { | ||
| addExtraErrorKeys(leftHandKeys); | ||
| addExtraErrorKeys(rightHandKeys); |
There was a problem hiding this comment.
I think it'd be simpler just to do:
leftHandkeys.push('name');
leftHandKeys.push('message');
rightHandKeys.push('name');
rightHandKeys.push('message');Calling out to a function and checking if those keys are already in the index is "probably" (my hand-wavy intuition) more costly than just checking the property for equality twice over. Certainly the code would be cleaner though
There was a problem hiding this comment.
I like the simpler form aesthetically, but the problem is that if one Error object is assigned a custom name property (thus shadowing its prototype's name) and the other one isn't, then name gets added twice to the list of keys for the object that has the custom name property (once from getEnumerableKeys, and once from the manual push), whereas the other object only gets it added once from the manual push. The comparison then fails due to a difference in length of the two key arrays.
|
Whoops, looks like |
BREAKING CHANGE: This commit drops support for versions of Node that are no longer maintained.
|
Refactored the tests and snuck in another commit to drop support for unmaintained Node versions and update Travis config. |
|
Still LGTM. Another @chaijs/deep-eql member should review+merge. |
There was a problem hiding this comment.
I'm very happy with this change.
I've left two minor comments which are up to you to change or not if you agree with them.
I think that especially the one about moving EXTRA_ERROR_KEYS to inside the function could be useful, but I won't block merging because of that.
| @@ -108,9 +108,10 @@ The primary export of `deep-eql` is function that can be given two objects to co | |||
| - All own and inherited enumerable properties are considered: | |||
| - `eql(Object.create({ foo: { a: 1 } }), Object.create({ foo: { a: 1 } })).should.be.true;` | |||
| - `eql(Object.create({ foo: { a: 1 } }), Object.create({ foo: { a: 2 } })).should.be.false;` | |||
| - When comparing `Error` objects, `name` and `message` properties are also considered, regardless of enumerability: | |||
| - `eql(Error('foo'), Error('foo')).should.be.true;` | |||
| - `eql(Error('foo'), Error('bar')).should.be.false;` | |||
There was a problem hiding this comment.
Maybe worth using an example of an error with a property being set explicitly different, such as code, but with the same message and type being false.
index.js
Outdated
| @@ -25,6 +25,9 @@ FakeMap.prototype = { | |||
| }, | |||
| }; | |||
|
|
|||
| // Non-enumerable keys to include in comparison of Error objects | |||
| var EXTRA_ERROR_KEYS = [ 'name', 'message' ]; | |||
There was a problem hiding this comment.
Perhaps we could move this to be just above addExtraErrorKeys since it's only used there?
Also, I wast thinking that these could be used inline on addExtraErrorKeys or even just declared inside that function since they are only used there if that doesn't hurt performance.
BREAKING CHANGE: Previously, `Error` objects were compared using strict equality. This commit causes `Error` objects to be compared using deep equality instead, but treats them as a special case by including their `name` and `message` properties in the comparison, regardless of enumerability.
|
Just pushed another update:
|
|
👍 |
BREAKING CHANGE: Previously,
Errorobjects were compared usingstrict equality. This commit causes
Errorobjects to be comparedusing deep equality instead, but treats them as a special case by
including their
nameandmessageproperties in the comparison,regardless of enumerability.
This PR is intended as a tamer,
Error-specific alternative to #56. While working on #56, I decided to shift course due to the following:nameproperty ofErrorobjects achieved something similar to comparing constructors, and was similar in nature to comparing the non-enumerablemessageproperty.stackproperty because it's already skipped by default due to being non-enumerable.