Compare errors by identity in _.isEqual

[randym]

Edit bij @jgonggrijp: tldr link.

Is this intentional?

var a = new Error('a')
var b = new Error('b')
_.isEqual(a, b) // true

This holds true for extensions as well...

class A extends Error { constructor(message) { super(message); this.enumerable = 'Klaatu barada nikto' } }
var e1 = new A('Fear has been substituted for reason.')
var e2 = new A('Join us and live in peace')
_.isEqual(a, b) // true

Other built in types (e.g. Date, RegExp, Symbol) have various methods to coerce for comparison. Error comparison ends up in deep equality comparison, which relies on Object.keys and ends up comparing empty arrays.

Happy to send in a PR if this is unintentional, also happy to work around it if it is.

[jgonggrijp]

Errors are special because all their properties are prototype getters. Why do you want to compare them for equality?

[randym]

Thank you for the very prompt reply @jgonggrijp

I came across this in a client's library that uses backbonejs. Registered listeners should receive notification when attributes change. One of the attributes was an error. Changes to that attribute did not generate any notifications. Dug in and arrived here.

[jgonggrijp]

Welcome. Makes sense. A few more questions:

• Why do those models have errors as attribute values?
• How would you fix _.isEqual for errors?
• Alternatively, how would you work around the issue?

Sorry to be so inquisitive. Errors are special beasts, so I'm still undecided on what would be the most reasonable course of action.

[randym]

No worries - I share your trepidation to jump onto a solution - I expect the authors, contributors and maintainers of such a long standing library have already spent considerable time on this.

• Why do those models have errors as attribute values?

The model manages facets of a speech recognition process that can contain an errors.

• How would you fix -.isEqual for errors

My first consideration would be to explicitly include both standard, and non-standard properties listed on MDN in the keys used for deep compare. I believe it would return the least surprising result and reasonably handle enumerable properties added when extending Error.

something along the lines of:

// isEqual#122
if (className === '[object Error]') {
  _keys =_keys.concat(['a bunch of properties'])
  length = _keys.length
}

• Alternatively, how would you work around the issue?

For this client, I generated a string to represent the error and used that value instead - and left a nice note to let everyone else know that they cannot use error objects as I expect anyone, myself included, would look at that code in six months and ask 'Why are we creating a custom string representation here?'

[jgonggrijp]

I'm glad we're on the same page with regard to not jumping to solutions.

I don't think the fix you're proposing for _.isEqual would work, because this line checks whether each key is an own property of b, which in the case of Error at least two are guaranteed not to be give your modification:

https://github.com/jashkenas/underscore/blob/d9741b32f29ddc24ac94f9ee8d073599948945e3/modules/isEqual.js#L124

Semantically, the most correct thing to do might actually be to check for object identity, i.e., a === b. The reason for this is that Errors are inherently unique, because they represent an exception that was thrown somewhere. It is technically impossible to faithfully clone an Error for this same reason.

That would mean that _.isEqual(error1, error2) always returns false unless error1 === error2. How would that work for your situation? If you want nonidentical errors to compare equal under some conditions, I'm thinking you might be better off just storing theError.message in that model attribute.

[randym]

You are absolutely correct. !has(a, key) properties would need to be excluded from whatever was added to _keys. There are other options as well, such as using a separate while block entirely or extracting out the Array, Object and Error comparison processes.

Regardless, your idea is great. It would be a clean, minimal change that precisely solves this specific problem.

Is there any concern that any implementation would be a a breaking change?

Cloning methods based on getOwnPropertyNames may be expecting underscore to treat those 'clones' as equal when comparing error instances.

[jgonggrijp]

Is there any concern that any implementation would be a a breaking change?

Yes, breaking changes are a concern. If a breaking change is purely a bugfix (bugfixes are in a sense always "breaking" changes), then we can get away with putting it in a minor update, but otherwise, it needs to be postponed until Underscore 2.0. An example of such a change is #2815.

Errors are a bit of an edge case, which always makes it a bit fuzzy whether you're actually making the code more correct or just making a different arbitrary choice. While I do believe that errors are unique, there might be somebody out there using _.isEqual to check whether two errors have the same type. I wonder what @jashkenas thinks so I'll attract his attention.

Cloning methods based on getOwnPropertyNames may be expecting underscore to treat those 'clones' as equal when comparing error instances.

Fortunately, this won't change; the following code already returns false because _.isEqual takes constructors and string tags into account.

var x = new Error();
_.isEqual(x, _.clone(x));

Also, the following would continue to return true.

_.isEqual(_.clone(x), _.clone(x));

[jgonggrijp]

TLDR @jashkenas:

• @randym has a plausible use case for comparing Error objects using _.isEqual.
• The problem is that Error objects always compare equal if they have the same constructor, even if they have a different .message, were thrown on a different line, etcetera.
• Semantically speaking, the most correct thing to do might be to compare Error instances by identity, i.e., return false unless error1 === error2.
• This is however also quite a radical change. Somebody out there might be using _.isEqual to test whether two errors have the same type, which would break.
• Do you think we could defend such a change as purely a bugfix, or would it require a major version bump?

[meyraa]

I have the same issue after updating from 1.10.2 to 1.11.0:

var oldFile= { content: new Buffer("foo") }
var newFile= { content: new Buffer("bar") }

// Version 1.10.2 
_.isEqual(oldFile, newFile) // false

// Version 1.11.0 
_.isEqual(oldFile, newFile) // true

[jgonggrijp]

@meyraa I don't think this is the same issue. Moreover, it appears I fixed the comparison of Buffers in #2884. Please check whether you still run into your problem with the bleeding edge version (or esm). You can use that until version 1.12 is released, which should be soon (#2878).

[meyraa]

@jgonggrijp Thank you, the bleeding edge version works fine.

[jgonggrijp]

@jashkenas informed me he won't have time to look at this issue for a while. I'm not in a rush, so I'm just leaving this open so I can give it more thought.

@captbaritone @michaelficarra @paulfalgout @joshuacc (or anyone else) if you feel like giving an opinion, I'm interested.

[jashkenas]

I think you’ve all arrived at very reasonable conclusions here. Special casing isEqual for object identity comparison for Errors specifically seems like a nice improvement to make.

To the larger point about version numbers — cases like this are a clean example of why I do not believe in Semantic Versioning. As long as one man’s bugfix is another man’s breaking change, the scheme is unworkable. But that leaves us with the pragmatic decision: to avoid the potential for anger, we should wait to release this change until 2.0.

[jgonggrijp]

Thanks for stepping in anyway, @jashkenas! While I'm a bit more optimistic about semver, I can follow your reasoning. I'll slap a "change" label onto this and I'll also create a 2.0 milestone while I'm at it.