deepEqual returns true for different dates

[nidu]

Hello.

MobX mobx.extras.deepEqual returns true when comparing two different Date objects. Probably just comparing dates deeply is meaningless but they are compared this way being attributes of some other objects.

jsfiddle, example from node console:

> mobx.extras.deepEqual(new Date('2016-01-01'), new Date('2017-01-01'))
true

MobX 2.70, 3.2.2. Didn't check other

Note: initially spotted when working with moment. When two moment instances created from Date objects are compared - they are equal as well.

[urugator]

Check the comment

[nidu]

@urugator i'm using MobX 2.7.0 at the moment. Does it mean i can't use asStructure modifier in reaction when there are dates in compared structures by design?

[urugator]

Dates aren't plain structures, therefore they may not be structuraly comparable...
I have a feeling that someone reported that structural comparator works for dates, but it has been some time, so it could have changed. It really depends on the Date implementation, so I wouldn't rely on it.

[nidu]

@urugator Agree that they are not meant to be structurally compared. However they can be attributes of objects which are structurally compared. I suppose easy fix would be just to make a special case in deepEqual for dates.

[urugator]

However they can be attributes of objects which are structurally compared.

Can be said about a lot of built-ins (Element, new Number, new Boolean), making a special case for each probably isn't a solution. (I agree the Date is the most common one)
Someone may actually expect false (i would) on the same dates, because they are different instances. (Not very practical, but consistent behavior)
Also note that a lot of ppl use non-standard Date implementations (momentjs). (Another special cases?)
I quess it would be a breaking change, so it wouldn't get into 2.x.
In 3.x you can define own comparator.

[nidu]

@urugator not sure there are too much builtins and handling them may not be that cumbersome (not sure Element should be treated though). And yeah, special cases are special cases, they.

About expecting false: it's not obvious behavior when you compare two different objects and they are equal. Date is special (i'd say it's close to primitive type) and should be treated specially. deepEqual saying that two different dates are equal is just a implementation detail. If dates were represented as objects with year/month/day/... fields deepEqual would behave different though these are the same dates.

About momentjs - that's not a special JS case so i don't think it requires special case in MobX. If it uses Date - it would benefit as well consequently.

[urugator]

Maybe structural comparator should call valueOf before performing the comparison?

[nidu]

Sounds like a nice idea

[mweststrate]

Since recently computed and reaction support custom comparison functions, so I think there is no need to further extend mobx for this.

[urugator]

@mweststrate
At least for the next Mobx version, I would really consider calling valueOf in comparer.structural.
The behavior would imho better reflect the purpose of structural comparator, which is to treat compared things as value like objects
valueOf is a standard way to make arbitrary types comparable exactly in this manner.
I can't imagine a situation where it would be undesirable.

[mweststrate]

Yep that makes sense. Can you add a link to this issue in the roadmap issue? Op di 5 sep. 2017 om 19:46 schreef urugator <[email protected]>:

…

@mweststrate <https://github.com/mweststrate> At least for the next Mobx version, I would really consider calling valueOf in comparer.structural. The behavior would imho better reflect the purpose of structural comparator, which is to treat compared things as value like ojects <https://en.wikipedia.org/wiki/Value_object> valueOf is a standard way to make arbitrary types comparable exactly in this manner. I can't imagine a situation where it would be undesirable. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1118 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhPzn8j4dsPPiZKdzZzQ7HOURAi1vks5sfYjmgaJpZM4Omcjt> .

[urugator]

Btw it also plays well with boxed values - currently the deepEquals fails on the same boxed values (quite unexpected tbh):

const Mobx = require("mobx");
const box1 = Mobx.observable(5);
const box2 = Mobx.observable(5);
console.log(Mobx.extras.deepEqual(box1, box2)); // false
console.log(Mobx.extras.deepEqual(box1.valueOf(), box2.valueOf())); // true

[mweststrate]

Also note that MobX is just following the general consensus here on what "deep equality" means:

// Jest:
expect(new Date("2016-01-01")).toEqual(new Date("2017-01-01")) // false

// Object.is:
Object.is(new Date("2016-01-01"), new Date("2017-01-01")) // false

// Lodash:
_.isEqual(new Date("2016-01-01"), new Date("2017-01-01")) // false

Also note that all api's in MobX that use comparision allow customizing the equals function that is used, so you can still define a comparer in your code base that works as you want.

[mweststrate]

Ok, this is funny (not). Somehow this entire conversation is based on me reading:

"deepEqual should return true for same dates"

and

mobx.extras.deepEqual(new Date('2016-01-01'), new Date('2016-01-01'))

Sorry folks!

Which also kinda kills the examples because I copy pasted them wrong :-/.

[mweststrate]

Released as 3.6.0. I feel quite embarrassed for leaving this issue unsolved for halve a year but not correctly understanding it :)

Also, the new algorithm is quite literally taken from underscore. Nice benefit is that dates are now treated as primitives so two Date objects with the same date yield true.