-
Notifications
You must be signed in to change notification settings - Fork 157
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix binary comparison operators #517
Comments
The problem is fixed if we make We could concatenate the parts together such that 2020-04-21 turns into 20200421n on valueOf(), except we'd need to do some kind of inversion for the negative years, because February is after January even in negative years according to the ISO calendar. (Right?) |
Hmm, this is a problem. I hadn't realized that I think ideally we should somehow point people to the Temporal.X.compare() static methods when they use comparison operators. I don't think we should try to make the comparison operators "work" unless they can work 100% of the time like they would if operator overloading existed. It's also the case that you cannot compare Temporal.Durations at all, so using We'd have to do this in such a way that it wouldn't preclude overloading the comparison operators on Temporal types if/when operator overloading is added to the language. |
If we bite the bullet and introduce The alternative is to make But, I have to say that I do have a certain affection for making |
as product-developer, comparing isostrings is legitimate and efficient when 99% of ux-applications only concern with date-ranges its easier to do the above (and handle/invalidate uncommon date-ranges separately on per-ux-basis), than to use some builtin/overloaded temporal comparison operator. |
Another off-the-wall idea: restrict Temporal to only represent dates with positive ISO years, and use the extended ISO 8601 syntax "YYYYYY-MM-DD", such that string comparison always works. |
If adding |
Reading the spec for how
The primitive types are Undefined, Null, Boolean, String, Symbol, Number, and BigInt. So, in conclusion, we're stuck with either String or a numeric type if we want to make |
How about making Does this approach preclude operator overloading, if that ever happens? If this does work then should we remove the compare() methods? |
What's the benefit of not defining it ourselves? That seems like a recipe for bugs. |
My hand-wavy reasoning was that if it was implementation-defined, then users couldn't write code that depended on it being something specific, and it would leave us free to implement it via operator overloading in the future... emphasis on hand-wavy. |
They'd still write code based on "whatever each of the browsers chose", and that would in fact force us to support all of those implementations eventually. |
Or, more likely, they'd write code based only on "whatever Chromium does" and claim that any other behaviour was a bug. (I'm thinking of Twitter's recent problem with caching DMs in Firefox for example.) |
The valueOf representations could be (all BigInt):
In addition, I would say that you can also pass these values back into the constructors. If the constructor receives exactly one numeric argument, then that argument should be interpreted as the corresponding valueOf representation. (The calendar would still be allowed as the second argument.) This is easy in the Temporal.Date and Temporal.DateTime constructors, since we already require at least three numeric arguments for the normal case. For the Temporal.Time constructor, we could start requiring at least hour, minute, and second, such that we can cleanly identify when the call to the constructor is passing the valueOf representation. |
MonthDay could end up being a string, not a bigint? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I believe we've previously discussed comparison with @pipobscure and @gibson042 . I pushed for I don't think we can make comparisons enabled through Can we make the |
I think the best way to do that would be a |
@gibson042 I don't know if I agree with seeing The effect of your suggestion would to be to make Temporal objects act like NaN in comparisons, which might be nice, since it'd make all comparisons return false. Another thing we could do is make |
I guess the relevant difference between |
But remember that |
This comment has been minimized.
This comment has been minimized.
Right, it was removed because we didn't want this particular comparison path to be taken. @sffc points out in this issue, that change didn't have the intended effect, since now the comparison is between strings. So, I propose that we include a |
I can see the concerns about Making I wasn't around for #74, but I'm really not convinced by the premise. If the concern is about people comparing objects of different types, that's a problem that ESLint and TypeScript should solve. |
Let me try to put the debate in different terms. Consider how this decision affects ergonomics and Stack Overflow questions. There are two types of competing questions that end users might have:
In my mind, the second question is far worse than the first. Although JavaScript Date has flaws, it works the way JavaScript developers expect it to work: you can call |
Right, I guess this is just a difference in design philosophy. Correctness vs ergonomics is a spectrum, and it might not be best for users to be excessively picky/pedantic here. The history is that we decided to be pedantic, and include an explicit My take: by separating datetimes into several different types, Temporal is pushing against JavaScript's weak typing history. I see the strongly typed aspect of Temporal to really be a core design principle here. So I'd prefer 2: I think errors are far preferable to getting the wrong answer. I could see permitting coercion for certain things, but only when we feel like we can get a correct answer. But that's just my take; I can see us making some other tradeoff. I just want us to make this tradeoff understanding the history and the failure modes. |
Meeting, May 28: We've gathered feedback, and after considering it we intend to ship the initial polyfill with a |
This adds a valueOf() method to Absolute, Date, DateTime, Duration, MonthDay, Time, and YearMonth, which throws a TypeError. This is in order to prevent spurious comparisons using <, <=, >, and >= from working. Closes: #517
This adds a valueOf() method to Absolute, Date, DateTime, Duration, MonthDay, Time, and YearMonth, which throws a TypeError. This is in order to prevent spurious comparisons using <, <=, >, and >= from working. Closes: #517
This adds a valueOf() method to Absolute, Date, DateTime, Duration, MonthDay, Time, and YearMonth, which throws a TypeError. This is in order to prevent spurious comparisons using <, <=, >, and >= from working. Closes: #517
I'm new to this proposal, and I have to say that I can see why it doesn't make sense to have (The excellent docs pages asked for feedback, here it is) |
Thanks, I've been mulling over this and I think it's a good discussion to have in the context of a future addition to Temporal: js-temporal/proposal-temporal-v2#6 |
People are going to use
>
and<
on Temporal objects, whether we like it or not. It mostly works:However, it breaks in certain cases such as negative dates:
I think the reason is that
<
and>
implicitly usetoString()
, and it just so happens to be that string comparison on the ISO string works for positive dates.Related: #293
The text was updated successfully, but these errors were encountered: