Skip to content
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

Instant vs ZonedInstant #42

Closed
ljharb opened this issue Jul 27, 2017 · 64 comments
Closed

Instant vs ZonedInstant #42

ljharb opened this issue Jul 27, 2017 · 64 comments
Labels
behavior Relating to behavior defined in the proposal question

Comments

@ljharb
Copy link
Member

ljharb commented Jul 27, 2017

I asked in the meeting about this, and got an explanation, but wanted to discuss more offline.

Since ZonedInstant has a potential timezone of "UTC", it seems to be that anything you want to require "Instant" for, you can also require "ZonedInstance with a UTC timezone" for - it feels very "strongly typed language" to me to have disparate types for something that's otherwise equivalent.

Is there a reason we couldn't halve the number of globals, and condense Instant and ZonedInstant into the same name?

@mattjohnsonpint
Copy link
Collaborator

var i = new Instant(1514764800000);
var zi = i.withZone('UTC');
var d = zi.toCivilDate();  // 2018-01-01

vs

var i = new Instant(1514764800000);
var d = i.toCivilDate();  // 2018-01-01

Ok, that makes sense, but is that always what we want to do?

var i = new Instant(1514764800000);
var zi = i.withZone('America/Los_Angeles');
var d = zi.toCivilDate();  // 2017-12-31

Without an interim object, we'd have two choices:

  1. var i = new Instant(1514764800000);
    var d = i.toCivilDate('America/Los_Angeles');  // 2017-12-31
  2. var zi = new ZonedInstant(1514764800000, 'America/Los_Angeles');
    var d = zi.toCivilDate();  // 2017-12-31

So assuming you meant the second option, the downside is that two ZonedInstant objects might have the same instant, but different time zones. In that case it would be difficult to apply equality and comparison operations to them.

var zi1 = new ZonedInstant(1514764800000, 'UTC');                 // "2018-01-01T00:00:00Z"
var zi2 = new ZonedInstant(1514764800000, 'America/Los_Angeles'); // "2017-12-31T16:00:00-08:00"

How does one equate or compare these two objects?

With a separate Instant object, we can offer an API that allows comparison and equality. We could then prohibit comparison operations on ZonedInstant (always false, or throw, etc. TBD) but still allow equality if both timestamp and time zone matched.

@mattjohnsonpint
Copy link
Collaborator

Also, WRT option 1, I'd say that is not acceptable because it prohibits retaining time zone or offset information in the result of parsing a string that might contain such information.

@ljharb
Copy link
Member Author

ljharb commented Jul 27, 2017

We could prohibit comparison operations on ZonedInstance whenever the zone isn't "UTC" just as easily, could we not?

@rxaviers
Copy link
Member

API-wise, instead of a I would prefer b below:

// a)
let i = new Instant(1514764800000);
let zi = new ZonedInstant(1514764800000, 'America/Los_Angeles');

// b)
let i = new Instant(1514764800000);
let zi = new Instant(1514764800000, {timeZone: 'America/Los_Angeles'});

@rxaviers
Copy link
Member

rxaviers commented Jul 27, 2017

@mj1856 can you show me an example how to use temporal to check if date A in timezone X is equal to date B in timezone Y? For example, how to assert that 1:10PM in America/Los_Angeles means the same instant that 5:10PM in America/Sao_Paulo (both if we consider Jul 27, 2017 as date)?

@apaprocki
Copy link
Contributor

@rxaviers For that specific comparison, you'd want a.toInstant() === b.toInstant(). Personally, I wouldn't want to see equality on ZonedInstant that implicitly coerces to UTC.

@maggiepint
Copy link
Member

I would say that two ZonedInstants in different time zones that reflect the same point in time are unequal - that's not necessarily what the user wants. So basically, what @apaprocki said.

@domenic
Copy link
Member

domenic commented Jul 27, 2017

@apaprocki to be clear, you wouldn't use ===, since the return values would be different objects. Right?

@mattjohnsonpint
Copy link
Collaborator

Note that Date equality for either == or === doesn't work either. Should Instant?

All below are true:

new Date(0) != new Date(0)
new Date(0) !== new Date(0)
+new Date(0) == +new Date(0)
+new Date(0) === +new Date(0)
new Date(0) <= new Date(0)
new Date(0) >= new Date(0)
new Date(0) < new Date(1)
new Date(1) > new Date(0)

@maggiepint
Copy link
Member

Let's not operator overload :-)

@mattjohnsonpint
Copy link
Collaborator

mattjohnsonpint commented Jul 27, 2017

@rxaviers - RE the two options you gave, I'd prefer a, given the time zone is required. This is also being discussed in #41

@rxaviers
Copy link
Member

rxaviers commented Jul 27, 2017

@mj1856 how do I get a zoned date in the user's environment date please? (sorry if this was already discussed somewhere else)

@domenic
Copy link
Member

domenic commented Jul 27, 2017

What two options did I give?

@mattjohnsonpint
Copy link
Collaborator

@domenic - sorry, meant @rxaviers

@mattjohnsonpint
Copy link
Collaborator

Or did you mean that there would only be Instant and time zone would be optional?

@ljharb
Copy link
Member Author

ljharb commented Jul 27, 2017

@mj1856 that seems much more reasonable to me than having two "types".

@rxaviers
Copy link
Member

@mj1856 yes, your last comment :); specially depending on your answer for #42 (comment)

@maggiepint
Copy link
Member

There's massive value to the code clarity you get in differentiating between a point on the global timeline with no context, and a point for which we have local context. I'll have to come up with some larger code samples to clarify.

@ljharb
Copy link
Member Author

ljharb commented Jul 27, 2017

@maggiepint since timezone and "type" would only be present when constructing the instance (there's no type annotations elsewhere that would be relevant), I'd love to see the code examples for that.

@rxaviers
Copy link
Member

@maggiepint me too, thanks.

@simonbuchan
Copy link

As a strawman, you could replace ZonedInstant with TimeZone and make the user keep track:

const instant = new Instant(1514764800000);
const timeZone = new TimeZone('America/Los_Angeles');
// whichever you prefer, or both:
const civilDateFromTimeZone = timeZone.toCivilDate(instant);  // 2017-12-31
const civilDateFromInstant = instant.toCivilDate(timeZone);  // 2017-12-31

Of course, instant.toCivilDate('America/Los_Angeles') as mentioned also works fine (unless there's some perf issue?).

One problem with this would be than you can't directly .plus() it, would it ever be the case that instant.plus(x).toCivilDate(z) would give different results to instant.toCivilDate(z).plus(x)? Would that be problematic?

I'm guessing ZonedInstant basically exists for having the thing to toString()? This is a bigger issue, for sure, and would probably be enough for me to lean towards keeping it, but it should probably be trying to dovetail with Intl.DateTimeFormat in some form, which itself handles timezone conversions. E.g.:

const format = new Intl.DateTimeFormat('en-AU', {
  hour: 'numeric', minute: 'numeric', second: 'numeric', 
  timeZone: 'Australia/Sydney',
  timeZoneName: 'short'
});
new Instant(1514764800000).toString(format); '11:00:00 AM AEDT'

I'm pretty sure you wouldn't want to combine the other format data into a ZonedFormattedInstant, so maybe ZonedInstant is not needed either?

I mention this largely because it's a bit of a weird concept for me to have a combined instant in time and a time zone: in my head time zones are what you use to convert between instants and local date and times!

@rxaviers
Copy link
Member

Just to keep track... On #84 (comment), @gibson042 said:

I dispute that a separate type is critical for conveying that semantic (as opposed to e.g. nullable time zone data on a single Instant type), but the semantic itself does seem to matter and is not captured by setting time zone to UTC.

@rxaviers
Copy link
Member

In terms of API, would it make sense to consider something like the below as an alternative to having two types?

const instant = new Instant(0);
instant.toString(); // "1970-01-01T00:00:00.000000000Z"
instant.timeZone; // null
instant instanceof Instant; // true

// Equivalent to new Instant(0, {timeZone: "UTC"});
const utcInstant = instant.withZone('UTC');
utcInstant.toString(); // "1970-01-01T00:00:00.000000000+00:00"
utcInstant.timeZone; // "UTC"
utcInstant instanceof Instant; // true

With respect to comparison, it could throw if comparing Instant instances on different time zones, but compare normally if they are the same.

@ljharb
Copy link
Member Author

ljharb commented Sep 24, 2018

Why would ducktyping the value of a property be a better design than two distinct types, when "the zone doesn't matter" is a distinct thing from "it has a zone"?

@rxaviers
Copy link
Member

Because in practice both are so similar that is simpler to have one type?

It would be good if @maggiepint could proceed with her examples #42 (comment)

@ljharb
Copy link
Member Author

ljharb commented Sep 24, 2018

It may be simpler from a technical perspective, but I think it's more complex from a mental model perspective, because people don't tend to think that a fundamental semantic of an object is represented by the value of a single property on it (and when they do, bugs often abound).

@gibson042
Copy link
Collaborator

gibson042 commented Sep 24, 2018

Time value is certainly fundamental to the model of an instant, but is time zone or even UTC offset? They're arguably more like decoration. Time zone can obviously influence comparison class methods, but in my opinion should have no effects on the operation of <, >, <=, or >= operators—instants, with or without zone information, are fundamentally temporal.

I'm open to persuasion on the merits of a separate class, but find the code samples currently in this issue to be unconvincing. What is an example that would be more difficult, complex, or confusing with zone-optional Instant as opposed to zoneless Instant and zone-mandatory ZonedInstant?

@mattjohnsonpint
Copy link
Collaborator

const zi1 = ZonedInstant.fromString("2018‑01-01T00:00:00-08:00[America/Los_Angeles]");
const zi2 = ZonedInstant.fromString("2018‑01-01T02:00:00-05:00[America/New_York]");

By their instants, zi2 is before zi1 by one hour. By their local times, zi1 is before zi2` by two hours. Simply sorting the strings or their local times will give a different result than sorting their instants.

I tend to agree that if we have to choose one or the other, we chose the instant. That is one of the benefits of calling it a "zoned instant". However there are cases that are less clear:

const zi3 = ZonedInstant.fromString("2018‑01-01T00:00:00-08:00[America/Los_Angeles]");
const zi4 = ZonedInstant.fromString("2018‑01-01T03:00:00-05:00[America/New_York]");

Both zi3 and zi4 have the same instant, even though the two objects aren't "equal". So which comes first in a sort? What does comparing them return? It's much less clear what to do.

@timrwood
Copy link
Contributor

I think the main use case of ZonedInstant is to convert back and forth between an Instant and a DateTime in a specific time zone.

What if we dropped the Instant part of ZonedInstant and instead had a Zone object that knew how to convert an Instant to a DateTime and back? Here is how that could change the example in the readme.

let chicagoZone = new Zone('America/Chicago')
let sydneyZone = new Zone('Australia/Sydney')
let dateTimeInChicago = new CivilDateTime(2000, 12, 31, 23, 59)
let instant = chicagoZone.instantAt(dateTimeInChicago)
let dateTimeInSydney = sydneyZone.dateTimeAt(instant)
dateTimeInChicago.toString() // 2000-12-31T23:59:00.000000000
dateTimeInSydney.toString()  // 2001-01-01T16:59:00.000000000

@rxaviers
Copy link
Member

"Simpler mental model perspective" - this is based on preference, what could be simpler for you, could be confusing for me. 🤷‍♂️ - @ljharb sorry, I won't debate preferences

@rxaviers
Copy link
Member

rxaviers commented Sep 26, 2018

@timrwood, thanks for the example, but it's not clear to me how it helps define the need for both classes (instead of one). Using today's spec your example would be written as the below, right?

let dateTime = new CivilDateTime(2000, 12, 31, 23, 59);

// Instant considering dateTime is based in Chicago:
let instant = dateTime.withZone('America/Chicago');

let dateTimeInSydney = instant.withZone('Australia/Sydney').toLocalDateTime();
// ... or alternatively:
// let dateTimeInSydney = new ZonedInstant(instant, 'Australia/Sydney').toCivilDateTime();

dateTime.toString();          // "2000-12-31T23:59:00.000000000"
dateTimeInSydney.toString();  // "2001-01-01T16:59:00.000000000"

If instead we follow the single class proposal above, nothing changes...

@pipobscure
Copy link
Collaborator

pipobscure commented Sep 26, 2018

@timrwood one of the aims I have for these Objects is that they are neutral in terms of culture. (i.e. methods specific to a calendar do not belong on Instant / ZonedInstant / Zone)

CivilDateTime (et al) are specific to a calendar and way of time-keeping. As such, I don't think Zoned or Instant should have any knowledge of them.

The consequence is that:

zoned.toCivilDateTime(); // BAD

That means it would mean for your proposal:

let civil = CivilDateTime.fromZoneInstant(zone, instant);
let instant = civil.withZone(zoned);
let zone = civil.withInstant(instant); // find the timezone where this Instant is that CivilDateTime

@mattjohnsonpint
Copy link
Collaborator

At a certain point, a lot of this becomes subjective. For example, the difference that @timrwood describes is similar to the difference between JSR310 and Time4J. Both models work, it's really just about which model we want to bake in to the language. We can make objective statements about pros and cons of each model, but we shouldn't be trying to figure out which one is more correct than the other. That would be futile.

@mattjohnsonpint
Copy link
Collaborator

(More on this difference in the Time4J docs)

@simonbuchan
Copy link

@rxaviers, you seem to have missed the point of my comment, sorry. I don't care what the name is except for what it implies about the concept, and I think the concept of "instant + time zone" is wrong, which is what is causing the disagreement in this thread.

My suggestion was to say "do I still want to compare by point in time if this type doesn't have instant in the name?"

Given your example above, I would not expect CivilDateTime#withZone() to return a ZonedInstant, but rather, perhaps, ZonedDateTime, and for that to have a #toInstant() that is comparable for point in time comparisons. I would guess ZonedDateTime would be either incomparable or lexicographically compared (roughly, compare iso strings)

Does that make it clearer?

@rxaviers
Copy link
Member

@simonbuchan thanks for the clarification. Therefore, I understand you're challenging where "timezone" should be: In *Instant* or in *DateTime*? My opinion: (a) I agree your question have impact to what we are discussing here, (b) but anyway, I encourage we don't mix topics but discuss this separately (perhaps reusing #84). Sounds reasonable?

@mattjohnsonpint
Copy link
Collaborator

@simonbuchan - Would you please restate your specific objection with "instant + time zone" is wrong? I think I'm following you, just want to be super clear. Thanks.

@mattjohnsonpint
Copy link
Collaborator

@timrwood and @simonbuchan have pitched similar proposals here
and here that are interesting alternatives.

I'm not completely opposed, but the questions I have with that model are:

  • Why do we need an object at all? Why not pass the time zone id as a string?
  • How would operations like plus work accross DST transitions?
  • How would one obtain a time zone offset or a datetimeoffset like string?

@ljharb
Copy link
Member Author

ljharb commented Sep 26, 2018

Passing things around as a string is harder to validate (both for correctness of the data, and correctness of "this is the right kind of thing"), harder to type for those using a type system, and harder to reason about.

@maggiepint
Copy link
Member

To my mind, the debate on whether we need ZonedInstant at all is one of those things that will boil down to developer preference every time, and developer preference will vary across everyone.

At the end of the day, there is a lot of gravity toward a ZonedInstant type object (regardless of name), in Java, and in several other datetime libraries around. That being said, I feel like existing forward momentum to have the object overrides other concerns.

@ljharb
Copy link
Member Author

ljharb commented Sep 26, 2018

I will note tho; if we had a Zone object, then i wouldn't feel the need to have "ZonedFoo" objects - or at least, those objects could have a nullable/Zone zone field, and I think i'd be content with that.

@mattjohnsonpint
Copy link
Collaborator

While a Zone (or TimeZone) object might add some validation, I don't think it's absolutely required. Anywhere we need a time zone, we can pass as a string.

We're not talking about having a variety of "zoned" objects. There is only one, and it is currently ZonedInstant, and might possibly be renamed to ZonedDateTime, but having a Zone object doesn't necessarily solve all of the problems such that we could remove the other ZonedInstant/ZonedDateTime object. If anything, we'd have to replace it with an OffsetDateTime object, and then we'd be passing zone objects around to various functions. I'm not sure the trade off is worth it.

@ljharb
Copy link
Member Author

ljharb commented Sep 27, 2018

A TimeZone object would be capable of normalizing input and/or providing validation in a way that strings would not, without also needing to create an arbitrary date and/or time to associate with it (and implicitly get the zone validation in the process).

@mattjohnsonpint
Copy link
Collaborator

Sure. It would give abilities like:

TimeZone.isValid('America/New_York') // true
TimeZone.isValid('America/Seattle') // false
TimeZone.isValid('-12:34') // true
TimeZone.validTimeZoneIds // ['SYSTEM', 'UTC', 'America/New_York', 'Asia/Tokyo', ...]

However, the last one is interesting because we are also allowing arbitrary time zone offsets like shown above it, and those wouldn't be in the list.

Either way, I think we can work on this for stage 3. It certainly could be a standalone object - it doesn't have to be passed as parameter. In other words,

const zdt = new ZonedDateTime('America/New_York', 2018, 1, 2, 3, 4, 5);
vs
const zdt = new ZonedDateTime(new TimeZone('America/New_York'), 2018, 1, 2, 3, 4, 5);

even if we have a TimeZone object for validation and listing, I think I still prefer the first one for constructing a zdt.

@gibson042
Copy link
Collaborator

The ambiquity remains even with two classes (we need an example here)

val = 1538058600000000n;
unzoned = new Instant(val);
zoned1 = new ZonedInstant(val, 'Pacific/Auckland');
zoned2 = new ZonedInstant(val, 'America/New_York');
[
  zoned1 > unzoned,
  zoned1 < unzoned,
  zoned2 > unzoned,
  zoned2 < unzoned,
  zoned1 > zoned2,
  zoned1 < zoned2
];
// → [?, ?, ?, ?, ?, ?]

@mattjohnsonpint
Copy link
Collaborator

We are removing valueOf, so > and < will not work. We thus have ability to create our own comparison operators if/when necessary.

@ljharb
Copy link
Member Author

ljharb commented Sep 27, 2018

@mj1856 since toString will still be there, < and > would work fine as string comparisons.

(agreed that even with a TimeZone object, we'd want to accept strings or TimeZone objects anywhere in the API that took either)

@gibson042
Copy link
Collaborator

Except that a) string comparison is only useful for comparing temporal instants that use exactly the same time zone designator, and b) abstract relational comparison strongly prefers numbers anyway (toString is only invoked after failing to find both @@toPrimitive and valueOf).

@ljharb
Copy link
Member Author

ljharb commented Sep 27, 2018

The hint isn't helpful unless we make valueOf or Symbol.toPrimitive throw (which is an option).

@mattjohnsonpint
Copy link
Collaborator

@ljharb - Thanks for clarifying that point. So if valueOf throws, then string comparison does not happen? I assume there is an order of precedence?

@gibson042
Copy link
Collaborator

valueOf is only invoked when @@toPrimitive is undefined, but if it is invoked and throws, then the exception will propagate up: http://jsbin.com/vozipotufo/edit?js,console

@gibson042
Copy link
Collaborator

I opened #92 for discussion around relational comparison, even though it is somewhat intertwined with this discussion.

@gibson042
Copy link
Collaborator

For better or worse, things seem to have stabilized around keeping ZonedInstant a distinct class from Instant (and also adding one for TimeZone). We can reopen or start a new issue if someone wants to raise this banner again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behavior Relating to behavior defined in the proposal question
Projects
None yet
Development

No branches or pull requests

10 participants