LocalDate to LocalDate comparison via opertors

[paplorinc]

Hey,

We're trying out js-joda as a moment alternative for TypeScript. So far we like it, and have a question.

Knowing that these return the expeced results:

LocalDate.of(2017,1,10)  < LocalDate.of(2017,1,10) // false
LocalDate.of(2017,1,10) <= LocalDate.of(2017,1,10) // true
LocalDate.of(2017,1,10)  > LocalDate.of(2017,1,10) // false
LocalDate.of(2017,1,10) >= LocalDate.of(2017,1,10) // true
LocalDate.of(2016,1,10)  < LocalDate.of(2017,1,10) // true

etc.
(Note, == or === doesn't seem to be working however...)

Is it valid to compare LocalDates and the rest via operators, as it seems to be working correctly, but couldn't find it in the docs (or even where the valueOf was overridden to allow this behavior).

Thanks,
Lőrinc

[phueper]

Hi @paplorinc,

thanks for reporting and for using js-joda.

To be honest, i haven't thought about using operators with joda objects, it might be a good idea to use / implement valueOf for the joda date objects, but that is currently not the case.

The fact that the comparison using <, >, <=, >= works is very probably coincidence, maybe has something to do with the joda objects implementing toString in a way that a comparison might work? But i'm really not sure...

In summary... i think using operators to compare joda Objects is an interesting idea, would be a topic for the roadmap but is currently not in our focus. To compare js-joda objects you should use the compareTo and equals methods, e.g. https://js-joda.github.io/js-joda/esdoc/class/src/LocalDate.js~LocalDate.html#instance-method-compareTo ...

Regards, Pattrick

[augusthur]

Equals (==) operator is necessary if you want to use Maps or Sets.

For example:

let da = LocalDate.parse("2012-12-24");
let db = LocalDate.parse("2012-12-24");
let dateSet = new Set();
dateSet.add(da);
dateSet.add(db);
dateSet.size // will be 2, storing both identical dates.

A workaround is to store dates as strings but makes code less readable.

[pithu]

Hi @paplorinc

as far as i understand the es6 spec, that's a problem with the Set specification http://www.ecma-international.org/ecma-262/6.0/#sec-set.prototype.add.

The method add is performing a value equal check for objects. So even implementing valueOf with something like LocalDate.prototype.valueOf = function() { return this.toString() } do not help. No idea how this could be fixed from js-joda side.

What about using immutable.js? They perform an Object.equals() check if exists, so with that library, it should work as you expect.

[augusthur]

You are right @pithu,
From js-joda side nothing can be done because Map and Set use SameValueZero algorithm.

[dgreene1]

One way to provide users a way to prevent issues with this would be to provide a set of ESLint rules would warn or error when someone tries to use a comparison operator.

[InExtremaRes]

Another way is implementing a valueOf method that always throws. That way an implicit convertion to a primitive value, like using comparison/arithmetic operators, will fail. The error message can give a hint about using instead compareTo, plus/minus, etc.

[dgreene1]

I would very much love @InExtremaRes's solution (#157 (comment)).

Here's why. I work very hard at every company I work at to evangelize the use of this library. Then it's always very sad to say "please use js-joda, it's great! oh... but make sure that no one ever uses the === operator since it has a bug.

The benefits of adopting the runtime assertion error with valueOf would be clearly communicating the bug so users could be informed to use .equals instead.

Like:

throw new Error('Please use .equals instead');

[InExtremaRes]

@dgreene1 The case against == is very different to comparisons like <, >, etc. The fact that == gives always false for two different temporal objects is not a bug of js-joda, is how javascript works. For instance:

{} === {}; // false
[] === []; // false;
new Date(0) === new Date(0); // false
new Set([1]) === new Set([1]); // false
(()=>{}) === (()=>{}); // false
// ... and so on

An equality check (either == or ===) always check that the objects are the same instance, not that have the same "content" (except for primitives). So the fact that two different LocalDate instances always give false is expected and consistent, and there is nothing we can do to avoid using == (as far as I know), except a linter rule, of course.

I think that instead of saying something like "oh... but make sure that no one ever uses the === operator since it has a bug" you should say to them "oh... BTW instances of js-joda has an equals method in case you need to check for value equality instead of identity". Every JS developer should learn the difference sooner or after (and better sooner than after).

On the other hand, the problem with operators like < is that sometimes they work, but that is only accidental. When you compare two objects as in o1 < o2, JS tries to convert them to primitives, ideally to numeric ones; since temporal objects doesn't have valueOf nor they have Symbol.toPrimitive JS tries instead converting to strings and calls toString() in each instance, then compares the strings, and since toString() returns an ISO string (like "2020-07-14") the comparison probably works due the lexicographical order. But lexicographical order fails when the dates are negative or the year is greater than 9999, for instance.

Implementing a valueOf method that throws will prevent implicit conversions to primitives, as for using <, + or similar operators (those that you expect to work with numbers), giving a hint of using compareTo, plus, minus, toString, etc. The operator == doesn't convert to primitives and is therefore out of this case.

[dgreene1]

Short version:

Let’s please introduce the valueOf assertion like you suggested.

Long version:

Good point. I forgot that the comparison operators are the more problematic issue since most JS devs probably would be looking for a .equals or .isSame method. I’m polyglot (C#, JS, Perl, bash, Kotlin), so I sometimes forget nuances like this. But we should still do something about the comparison operators.

[pithu]

I agree with @InExtremaRes that this is not a js-joda bug, but the way how javascript is specified.

Furthermore loose comparison with type coercion is problematic and leads to a lot of new questions, eg it should be symmetric to equals and compareTo what's quite difficult, because you can start to compare different types like LocalDate with LocalDateTime, whats equal to, compare apples and oranges. Another example is, what is a comparable primitive value of a ZonedDateTime ? So implementing valueOf seems not the wright thing to me.

I am not sure if throwing an exception when calling (implicit) valueOf is a good alternativ. Is this a common practice for other libraries? Isn't that somehow a wtf moment, if this code throws an exception if (date1 <= date2) {...} ? Isn't that simply something you never do in javascript ? Beside that, it's a breaking change.

[dgreene1]

Here’s a compromise that would allow us to make it not a breaking change:

If we allow the consumer to set some kind of global configuration similar to a strict mode. If jsJoda determines we’re in this new strict mode, it would throw.

In that way, only people like myself and @InExtremaRes who want it can opt into the valueOf throw (or any future validation that is above and beyond the regular level of concern).

[InExtremaRes]

@pithu

I am not sure if throwing an exception when calling (implicit) valueOf is a good alternativ. Is this a common practice for other libraries?

Throwing in valueOf is not common. I have used a library that throws but cannot remember which one. However the TC39 temporal proposal has recently decided implementing a valueOf that always throws (example in the spec) for the same reasons discussed here; if the proposal move forwards in that state it will set a precedent inside the standard.

Isn't that somehow a wtf moment, if this code throws an exception if (date1 <= date2) {...} ? Isn't that simply something you never do in javascript ?

I think the problem is what I said before: it works so many times to start believing it's by design. Many developers will make a few tests and will see the results are what they expect, without thinking much about that and very likely without testing corner cases. Furthermore, Date.prototype.valueOf is implemented to return the same as Date.prototype.getTime, that is, the milliseconds since the epoch, and so the comparisons works, and many are used to that behavior. But as you said, it doesn't fit properly to a library like this, so better not trying something similar.

IMO this library could just throw, it's already type-safe in many ways that restricting the coercion to a primitive doesn't seem unaligned from his goals, even if it's not that common to throw in JS. Another option is to always return NaN so at least a comparison always returns false and doesn't foolish anyone, but since that would be a breaking nonetheless, fail early and loud seems better to me, the error message can hive a hint of what to do. For those using something like if (date1 < date2) (God forgive them) and that really want to keep that behaviour, can convert manually to string: if(date1.toString() < date2.toString()) so at least will be conscious of what they're doing (so they can mourn on deep sorrow).

[pithu]

Furthermore, Date.prototype.valueOf is implemented to return the same as Date.prototype.getTime, that is, the milliseconds since the epoch, and so the comparisons works, and many are used to that behavior.

Good point, for developers used to the native Date behaviour the wtf moment could be even bigger, because comparison seems to work at first glance. This can lead to ugly and hard to find bugs.

However the TC39 temporal proposal has recently decided implementing a valueOf that always throws (example in the spec) for the same reasons discussed here;

Another good point.

Let's assume we implement that valueOf throws a TypeError. For what classes should we implement it ?

LocalDate, LocalTime, LocalDateTime, OffsetTime, OffsetDateTime, Instant, ZonedDateTime, anything else ?

[InExtremaRes]

Let's assume we implement that valueOf throws a TypeError. For what classes should we implement it ?

LocalDate, LocalTime, LocalDateTime, OffsetTime, OffsetDateTime, Instant, ZonedDateTime, anything else ?

I guess for every Temporal/TemporalAccessor and TemporalAmount?

Probably the next question would be: If one class can have a proper valueOf that returns something meaningful but most will throw, should that class be different? For instance DayOfWeek, Month and Year can just return .value() in valueOf (Duration might return something with trade-offs at least the return was a bigint) allowing to compare implicitly, but your concern about people doing something like if (MONDAY < APRIL) without any warning still holds. Most won't do that on purpose but mistakes happens.

So IMO (and pretending that valueOf will throw) all TemporalAccesor and TemporalAmount subclasses should throw except if a very good reason is found to opt-in something different in that particular case, pondering if worth the inconsistency.

[InExtremaRes]

@pithu @phueper I'm thinking about implementing this now since some breaking PRs have been recently merged and is very likely the next will be a major version. Is that OK?

I think the way to go is make every Temporal, TemporalAmount and enum-like instances have a prototype function valueOf that throws.

[dgreene1]

@pithu, I see that @InExtremaRes's PR was merged, but when I try to use a comparison operate it does not throw. Note, I'm trying this in the example imports that you put in the demo site:

Is this only occurring due to the way that it's imported into the demo site? Or is still a present issue in JsJoda?

Update: I did see that a comparison operator is no longer possible when I try this in Typescript, which is AMAZING, which means that @InExtremaRes deserves some kind of volunteer awards for fixing this issue.

So maybe we can close this issue now as part of housekeeping? Hopefully after we have a clear explanation for why it's still possible in the debugger in chrome/firefox? Maybe it's that it's an ES5 import? (I'm just guessing since it seems like the PR left legacy support for things like IE11)

[pithu]

The javascript on the entry page is outdated, it should be removed. Please use https://js-joda.github.io/js-joda/ to test the latest version of the library.
btw equal comparison with and without type (== and === conversion is still valid.

UPDATE: i just removed the outdated lib from https://js-joda.github.io