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

Feedback from Google C++ date library authors #139

Closed
sffc opened this issue Jun 11, 2019 · 10 comments
Closed

Feedback from Google C++ date library authors #139

sffc opened this issue Jun 11, 2019 · 10 comments
Assignees
Labels
behavior Relating to behavior defined in the proposal
Milestone

Comments

@sffc
Copy link
Collaborator

sffc commented Jun 11, 2019

I have reached out to @devjgm and @devbww, who have worked extensively on date libraries in C++, to provide feedback on the ergonomics of Temporal. I've asked them to post their feedback here in this thread.

@devjgm
Copy link

devjgm commented Jun 14, 2019

I don't know much about JavaScript, but I expect that this is much nicer than what exists today :-)

I've skimmed most of the markdown files. In my head, I'm comparing the concepts presented here to the concepts we used in https://abseil.io/docs/cpp/guides/time. Some thoughts:

  1. An Instant doesn't need to document or expose its epoch. An epoch is only necessary when converting to some other time representation, such as a count of fortnights since July 4, 2002. And in this case, you need the other time representation's epoch. We do not expose absl::Time's epoch, and we think this was a very good decision.

  2. We resisted the urge to have a hybrid civil-absolute time type, like OffsetDateTime or ZonedDateTime, and after years of customers using Abseil Time, I'm confident it was the right decision. Instead, we have very clearly separated "absolute times" (i.e., absl::Time or your Instant), "civil times", and a "TimeZone" object that provides all the rules for mapping between the two (that's all time zone's do, after all). This clear separation allows users to work within a single time domain and never care about time-zone oddities like DST or other weird offset shifts. Those complexities only show up when converting between time domains, so it limits the places where programmers need to think about them. Additionally, a ZonedDateTime object would be confusing for users to reason about. For example, since it really represents an absolute point in time, would it compare equal to another ZonedDateTime that represented the exact same instant, but was using a different zone so had different civil fields? Whatever the answer, some users would be surprised by it.

  3. Does this library support some notion of taking the date Jan 31, YYYY and "adding a month" to it? If so, what is the answer? Whatever the answer, I imagine some users would be surprised by it. FTR, in Abseil time, we make such questions fail to compile; we require the question to be asked in a non-ambiguous way, so users are never surprised by the result.

  4. "Durations can only be created through the minus method on temporal objects." Is there some way to create an object that represents, say, "30 seconds"?

@devbww
Copy link

devbww commented Jun 14, 2019

I (of course?) agree with what @devjgm had to say.

The only specific thing I have to add at the moment concerns, "Negative durations make no sense." I would take the opposite position. Indeed, if "Durations can only be created through the minus method on temporal objects," like toA - toB, what is the value of toB - toA?

@kaizhu256
Copy link
Contributor

  1. We resisted the urge to have a hybrid civil-absolute time type ... This clear separation allows users to work within a single time domain and never care about time-zone oddities like DST or other weird offset shifts.

i prefer going further and restricting datetime-arithmetic to only UTC-time. its unrealistic to expect integrated-systems to perform arithmetic-calculations exclusively in javascript -- some of the logic will inevitably be offloaded to databases and non-javascript systems, creating consistency issues. the only way to mitigate this painpoint is by forcing all these heterogenous systems to do datetime-arithmetic in UTC.

Is there some way to create an object that represents, say, "30 seconds"?

again, if you want it to be consistent with parallel calculation done in databases, you have to restrict arithmetic exclusively to UTC-time.

@ryzokuken ryzokuken added the behavior Relating to behavior defined in the proposal label Jul 12, 2019
@kaizhu256
Copy link
Contributor

Is there some way to create an object that represents, say, "30 seconds"?

lets take real-world example from https://www.google.com/flights/.

  1. flight hx68 departs 2020-03-08T11:55:00+08:00[HKT] from hong-kong
    and arrives 2020-03-08T09:50:00-07:00[PDT] in los-angeles.
    how do you use temporal-classes to calculate the flight-time difference (775 minutes)?

  2. how do you use temporal-classes to calculate the inverse?
    -- given flight hx68 departs 2020-03-08T11:55:00+08:00[HKT]
    with 775 minutes flight-time, what is the local arrival-time in los-angeles?
    note there's a +01:00 DST handover in los-angeles while in-flight.

@gibson042
Copy link
Collaborator

lets take real-world example from https://www.google.com/flights/.

Perfect; thank you!

  1. flight hx68 departs 2020-03-08T11:55:00+08:00[HKT] from hong-kong
    and arrives 2020-03-08T09:50:00-07:00[PDT] in los-angeles.
    how do you use temporal-classes to calculate the flight-time difference (775 minutes)?

Ideally, difference(earlierTemporal, largestUnit):

let departure = ZonedDateTime.fromString('2020-03-08T11:55:00+08:00[Asia/Hong_Kong]');
let arrival = ZonedDateTime.fromString('2020-03-08T09:50:00-07:00[America/Los_Angeles]');
let difference = arrival.difference(departure, 'minutes');

But if that method doesn't make the cut, then (arrival.instant.epochSeconds - departure.instant.epochSeconds) / 60 already works in the currently-documented surface area and polyfill.

  1. how do you use temporal-classes to calculate the inverse?
    -- given flight hx68 departs 2020-03-08T11:55:00+08:00[HKT]
    with 775 minutes flight-time, what is the local arrival-time in los-angeles?
    note there's a +01:00 DST handover in los-angeles while in-flight.

Ideally, departure.plus({minutes: 775}).withZone('America/Los_Angeles'). But with the current polyfill, Instant.fromEpochSeconds(departure.instant.epochSeconds + 775 * 60).withZone('America/Los_Angeles').

@kaizhu256
Copy link
Contributor

what if i'm unsure whether or not los-angeles is under daylight-savings? will this work?

let departure = ZonedDateTime.fromString('2020-03-08T11:55:00+08:00[Asia/Hong_Kong]');
-let arrival = ZonedDateTime.fromString('2020-03-08T09:50:00-07:00[America/Los_Angeles]');
+let arrival = ZonedDateTime.fromString('2020-03-08T09:50:00[America/Los_Angeles]');
let difference = arrival.difference(departure, 'minutes');

@gibson042
Copy link
Collaborator

No, I don't think so. But you can do

let arrival = CivilDateTime.fromString('2020-03-08T09:50:00').withZone('America/Los_Angeles');

@kaizhu256
Copy link
Contributor

sorry for being annoying, but you can solve this problem just as well with 2 static-functions that convert between [utc] ISOString <-> [local] ISOString:

function getUTCFromLocal (isostringUtc, zone) {...}
function getUTCToLocal (isostringLocal, zone) {...}
var duration;
var arriveLocal;
var arriveUtc;
var departUtc;




/*
 * 1. get flight-duration from (arrive - depart)
 */
departUtc = getUTCFromLocal("2020-03-08T11:55:00", "Asia/Hong_Kong");
arriveUtc = getUTCFromLocal("2020-03-08T09:50:50", "America/Los_Angeles");
duration = (
    new Date(arriveUtc).getTime() - new Date(departUtc).getTime()
) / (60 * 1000);
// departUtc = "2020-03-08T03:55:00.000Z"
// arriveUtc = "2020-03-08T16:50:00.000Z"
// duration = 775



/*
 * 2. get [local] flight-arrive from (depart + duration)
 */
departUtc = getUTCFromLocal("2020-03-08T11:55:00", "Asia/Hong_Kong");
arriveUtc = new Date(
    new Date(departUtc).getTime()
    + 775 * 60 * 1000
).toISOString();
arriveLocal = getUTCToLocal(arriveUtc, "America/Los_Angeles");
// departUtc = "2020-03-08T03:55:00.000Z"
// arriveUtc = "2020-03-08T16:50:00.000Z"
// arriveLocal = "2020-03-08T09:50:50.000+07:00"

instead of burdening tc39/w3c/mozilla members with tedious-specs dealing with temporal-class-interoperabilities,
you can have a simpler proposal that manipulates only string (easily spec'd by regexp), number, and bigint.

@pipobscure
Copy link
Collaborator

@devjgm thank you so much for taking the time. I've taken a lot of what you've said into the next version of the polyfill.

  1. The Instant does not expose epochXXXSeconds anymore. Instead there is a get method to retrieve it. Leaving Instant more generic.
  2. I have remove OffsetDateTime. Alas, we really do need that troublesome hybrid. However your comments and edgecase experience is valuable. I hope I've now taken those into account and documented them well. For example there is no zonedDateTime.withZone() because it's ambiguous. Instead we've added explicit methods.
  3. We do support that kind of addition. There is however a very clear algorithm. I've begun documenting them in my answer for proposal: improve the behavior of DateLike.prototype.with({month}) methods #150 and implemented it in the polyfill. This is going into the spec-text soon.
  4. That comment was actually important. It lead to Duration.fromString() and the means for passing in strings and DurationLike objects anywhere a Duration is required. So you could just pass in 'PT30S'.

I hope these conclusions make sense. This has been really valuable. I'll now close this issue and ask for a bit of time. By the end of September the spec-text will have been updated so that I can give an update to TC39 (2019-10-01T09:00:00-04:00[America/New_York] to about 2019-10-01T17:00:00-04:00[America/New_York])

@justingrant
Copy link
Collaborator

Hi @devjgm - re: your feedback above:

Additionally, a ZonedDateTime object would be confusing for users to reason about. For example, since it really represents an absolute point in time, would it compare equal to another ZonedDateTime that represented the exact same instant, but was using a different zone so had different civil fields? Whatever the answer, some users would be surprised by it.

What were other confusing things users found confusing with a combined type?

I'm asking because I've suggested having a combined {Absolute,TimeZone} type that encodes DST best practices in order to help newbies avoid accidentally committing DST bugs. So I'm interested to learn what the tradeoffs of having such a type might be for JS developers. I realize that Google's C++ engineers might have a higher bar for complexity but a lower bar for inconsistency compared to novice app devs in the JS community. ;-) But I still expect that your experience here is very valuable context.

BTW, your library is a great design. It's been decades since I wrote any C++ but I wish I'd had this back in the day!

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
Projects
None yet
Development

No branches or pull requests

8 participants