Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Better conversion between legacy Date and Temporal #515

Closed
sffc opened this issue Apr 21, 2020 · 13 comments · Fixed by #957
Closed

Better conversion between legacy Date and Temporal #515

sffc opened this issue Apr 21, 2020 · 13 comments · Fixed by #957
Assignees
Labels
behavior Relating to behavior defined in the proposal documentation Additions to documentation enhancement non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved

Comments

@sffc
Copy link
Collaborator

sffc commented Apr 21, 2020

I was working on a test project where I am parsing a CSV file that has dates of the form: "1/22/20" to mean January 22, 2020. I do not advocate for using such strings for interop, but the reality is that they exist, and that the old Date constructor can sometimes parse them.

So, right now, I'm tempted to do ugly things like:

// Step 1: parse the date using the JS Date constructor.
// jsDate is set to midnight on 2020-01-22 in my current time zone.
let jsDate = new Date("1/22/20");

// Step 2: convert to a Temporal.Absolute.
let absolute = Temporal.Absolute.fromEpochMilliseconds(jsDate);

// Step 3: convert to a Temporal.DateTime using my current time zone.
let dateTime = absolute.inTimeZone(Temporal.now.timeZone());

// Step 4: get out the date
let date = dateTime.getDate();

Really? That's unintuitive and prone to error, like how the Date constructor implicitly uses my local time zone, and I have to know to use that time zone when decoding, or else my code won't work in time zones east of UTC.

Why can't we add to the spec:

Date.prototype.toTemporalAbsolute = function() {
  return Temporal.Absolute.fromEpochMilliseconds(this.valueOf());
}

Date.prototype.toTemporalDateTime = function(timeZone) {
  const absolute = this.toTemporalAbsolute();
  // NOTE: Use the system time zone as default, like legacy Date.
  const defaultTimeZone = Temporal.now.timeZone();
  return absolute.inTimeZone(timeZone ?? defaultTimeZone);
}

Keep the conversion methods on Date to keep the Temporal namespace tidy.

@ptomato
Copy link
Collaborator

ptomato commented Apr 22, 2020

I think the answer up until now has been "If you have it in some custom format, write a parser specifically for that format." However, I agree that it's very easy and obvious to do the wrong thing here with legacy Date as a shortcut, and fall into the time zone trap that you mentioned. So I think some sort of answer is needed.

@littledan
Copy link
Member

For this use case: The JS spec doesn't ensure that the legacy Date parser will make that work. I think it'd be better to use Temporal.Date.from here.

For conversion to from legacy Dates in general: I'd be up for adding this if people feel like it's useful. However, based on previous discussions with @pdunkel , I thought we'd leave this for the cookbook entry.

@sffc sffc changed the title How to create a Temporal.Date from a non-ISO string? Better conversion between legacy Date and Temporal May 19, 2020
@sffc
Copy link
Collaborator Author

sffc commented May 19, 2020

I was thinking more about the Temporal.ZonedAbsolute data model from #569. Legacy Date is the same data model as Temporal.ZonedAbsolute, with two exceptions:

  • No calendar field
  • Millisecond resolution (instead of nanosecond resolution)

I wonder if some of @justingrant's use cases would be covered if we simply added first-class conversion methods between legacy Date and Temporal.

  • Date.prototype.toAbsolute() returns a Temporal.Absolute with the same timestamp as the legacy Date
  • Date.prototype.toDateTime(tz) returns a Temporal.DateTime with the same date and time fields as the legacy Date, with an optional time zone argument (defaults to the legacy Date's time zone)
  • Date.from(obj, tz) accepts a Temporal.Absolute or Temporal.DateTime and returns a legacy Date, truncated to the nearest millisecond, with a required time zone argument

@justingrant
Copy link
Collaborator

@sffc Legacy Date is the same data model as Temporal.ZonedAbsolute, with two exceptions:

  • No calendar field
  • Millisecond resolution (instead of nanosecond resolution)

This list omits the most important difference: there's no support with Date for doing operations in time zones other than the environment's timezone or UTC. The most obvious example of this is when a server app needs to parse, display, and/or manipulate data in the client's time zone.

So if you deal with calendar data from multiple timezones (like that server app case) it becomes really challenging to use Date properly. Even basic debugging and console output is a PITA.

The result is that most developers end up doing the same thing with legacy Date that we currently have to do with Temporal: build userland data structures that track both the instant and the timezone.

  • Date.from(obj, tz) accepts a Temporal.Absolute or Temporal.DateTime and returns a legacy Date, truncated to the nearest millisecond, with a required time zone argument

This solves part of the problem (easily getting TZ data into legacy Date) but what about the rest? How can I easily format values in that timezone? How can I add/subtract/diff? How can I ensure DST is handled according to that timezone's rules? And so on. I'd still have to round-trip back to Temporal to do anything interesting... and I still need to manually maintain my {Date, TimeZone} data structure because many method calls requires (or should require, for DST safety!) the timeZone parameter.

The big win would be persistence of the timezone and instant together. This is particularly useful because most of the time the "desired timezone" of date/time data is known when the data enters the app, and never changes. True, parsing and emitting a UTC-serialized version of the date/time is important, but honestly that's less of a "change time zone" use-case (because it's not persistent) and more like an alternate display format like toString vs. toLocalizedString vs toISOString.

if we simply added first-class conversion methods between legacy Date and Temporal

Regardless of the ZonedAbsolute decision, I think it'd be good to have conversion methods to help with legacy code interop.

But the Temporal API is soooooo much better:

  • immutable
  • chainable
  • less verbose field accessors
  • first-class support for dates without times
  • arithmetic
  • consistency
  • no weird accumulated legacy crud like a parse method that is deprecated, or a getTime() method that actually gets a timestamp not a dateless time value.

So for "instants that know their timezone" use-cases developers would choose between an ancient, awkward API or a modern, ergonomic API that invites DST bugs. Seems like a non-ideal choice.

I'm not against extending legacy Date with a persisted timezone that would affect toString/toLocaleString, setXXX methods, getXXX methods, etc. But honestly I'd worry about the corner cases and breaking-the-web implications of making such a big change to an old API.

And it still doesn't address the ergonomics and feature gaps with legacy Date, so I don't think it's a good alternative to ZonedAbsolute.

BTW, if we do have an interop API I'd suggest that it should only offer two types: toAbsolute() and toZonedAbsoute(tz), because those two are most similar to the existing Date API. I wouldn't recommend interop with toDateTime() because of the "novices will cause DST bugs" reasons I noted in earlier comments.

@sffc
Copy link
Collaborator Author

sffc commented May 19, 2020

This list omits the most important difference: there's no support with Date for doing operations in time zones other than the environment's timezone or UTC.

I was talking solely about the data model. ZonedAbsolute and legacy Date have the same data model, with the two exceptions I listed. However, there is a large chunk of functionality that Temporal types can do that legacy Date can't do, like arithmetic and proper time zone conversions.

This solves part of the problem (easily getting TZ data into legacy Date) but what about the rest?

Let's look at your use cases one by one.

How can I easily format values in that timezone?

Formatting is supported by legacy Date. You can already pass a time zone argument:

new Date().toLocaleString(undefined, {
  timeZone: "Asia/Singapore",
  timeZoneName: "long"
});
// "5/19/2020, 10:31:38 AM Singapore Standard Time"

How can I add/subtract/diff?

By design, you would go into Temporal for that, and the new methods would make it easier.

How can I ensure DST is handled according to that timezone's rules?

Let Temporal do that for you. If you need to add absolute time, go to Temporal.Absolute. If you need to add wall clock time, go to Temporal.DateTime.

I still need to manually maintain my {Date, TimeZone} data structure because many method calls requires (or should require, for DST safety!) the timeZone parameter. ... I'm not against extending legacy Date with a persisted timezone that would affect toString/toLocaleString, setXXX methods, getXXX methods, etc. But honestly I'd worry about the corner cases and breaking-the-web implications of making such a big change to an old API.

Legacy Date already has a timezone slot. However, it is not configurable (it always defaults to your local time) and it is only an offset in minutes. Implicit in my suggestion would be to explore whether we could attach an IANA timezone to legacy Date that corresponds to the timezone offset. This wouldn't break the web.

const date = new Date();

// Existing function: Returns 300 in America/Chicago during the summer
console.log(date.getTimezoneOffset());

// New function: Returns a Temporal.TimeZone for America/Chicago
console.log(date.getTimeZone());

// New function: Sets a time zone on an existing Date
date.setTimeZone("Asia/Singapore");
console.log(date.getTimezoneOffset());

I'm just giving this as a possible alternative to Temporal.ZonedAbsolute.

Adding Temporal.ZonedAbsolute opens a whole can of worms that we've been avoiding for the two years that this proposal has been at Stage 2. We removed this type because of the challenges it posed with regard to type conversion, arithmetic, et cetera, instead opting for the cleaner Absolute and DateTime model.

@ryzokuken ryzokuken added behavior Relating to behavior defined in the proposal enhancement labels May 20, 2020
@ryzokuken ryzokuken added this to the Stage 3 milestone May 20, 2020
@littledan
Copy link
Member

In general, Temporal doesn't have a minimal API, so I don't see it as out of the question to add something like this. This issue isn't the first time something had been raised. I'm not sure whether this is the kind of thing where a cookbook entry is enough.

It's fine to ship the initial polyfill without this, but we need to conclude on whether we add this small feature before Stage 3; it would be part of the spec text and polyfill before then. Or, we may decide to omit it.

@justingrant
Copy link
Collaborator

Implicit in my suggestion would be to explore whether we could attach an IANA timezone to legacy Date that corresponds to the timezone offset.

@sffc - Oops, I misunderstood your proposal. I like it more now. :-) If legacy Date could persistently store a time zone then many developers would have an easier time with many important use cases without having to learn a new API. So I think that's a good thing in general for the Web, regardless of what happens on the Temporal side.

I'm less enthusiastic about easy conversions from legacy Date to Temporal because I'd worry that if it's too easy to convert, then novice devs can get into trouble because the Temporal API is brand new (so no copypasta how-to content in Google) and Temporal is close enough to legacy Date to seem familiar, but different enough to introduce bugs via DST issues or 1-based vs. zero-based month.

So, at least for Temporal's first release until Google and StackOverflow can build up a good library of "How to use Temporal" content, I think there may be some benefit in keeping the two worlds somewhat separate so that devs have to opt into using Temporal instead of just following a legacy Date method call. Conversions in the other direction from Temporal to legacy seem like a good idea because it's probably safe to assume that devs who opt into Temporal already know how to use legacy Date. Also lots of existing APIs date Date parameters.

FWIW, I don't think enhancing Date will remove the need for something like ZonedAbsolute. I still think that having such a type would be a big usability win and would make a lot of use-cases easier. I've been working through some ideas about how such a type could work, including method-by-method details for what kinds of options would make sense and how DST corner cases should be addressed. So far it seems promising. I'll share more details soon.

@littledan
Copy link
Member

I don't think it is possible to change legacy Date's data model to include a timezone. I share the worry that, if we make it too easy to keep using legacy Date, people may not migrate properly. I agree that this is unrelated to whether we include a Temporal.ZonedTimestamp type.

@ptomato
Copy link
Collaborator

ptomato commented Sep 17, 2020

What's concretely left on this issue to discuss, now that LocalDateTime is definitely in, and nearing completion?

As I understand it, the open question is whether to add two legacy Date methods: Date.prototype.toAbsolute() and Date.prototype.toDateTime() (and maybe Date.prototype.toUTCAbsolute() and Date.prototype.toUTCDateTime() to match the pattern of existing legacy Date methods)?

@sffc
Copy link
Collaborator Author

sffc commented Sep 17, 2020

I have a preference to limit the conversion method to only Date.prototype.toAbsolute() since Date.prototype.toDateTime() and Date.prototype.toUTCDateTime() introduce an implicit calendar. If we add them, we should follow the same pattern we decide in #292 for Absolute.prototype.toLocalDateTime.

Date.prototype.toUTCAbsolute() is completely unnecessary; there's no such thing as a UTC Absolute.

@ptomato
Copy link
Collaborator

ptomato commented Sep 17, 2020

The existing pattern is that the presence or absence of "UTC" refers to how the this legacy Date object is interpreted, not to whatever comes after "UTC" in the method name. But you're right! There's no difference for toAbsolute. I think that's a good reason to have only toAbsolute, as well.

@justingrant
Copy link
Collaborator

I agree with only one conversion method to Absolute.

Should it be called toTemporalAbsolute to help users understated what it is and where to find it?

@ptomato
Copy link
Collaborator

ptomato commented Sep 18, 2020

Meeting, Sept. 18: We agree on adding a method to legacy Date. It will be called toTemporalInstant() (since the name of Absolute has been changed)

@ptomato ptomato added documentation Additions to documentation non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved labels Sep 18, 2020
@ptomato ptomato self-assigned this Oct 2, 2020
ptomato added a commit that referenced this issue Oct 2, 2020
This is in response to feedback that this conversion should be less
verbose.

No unit tests currently included because I found out that the test code
doesn't import the shim but instead imports individual objects from
lib/temporal.mjs, and I didn't see a quick way to fix that. Anyway, the
method is tested in the cookbook already.

Closes: #515
ptomato added a commit that referenced this issue Oct 5, 2020
This is in response to feedback that this conversion should be less
verbose.

No unit tests currently included because I found out that the test code
doesn't import the shim but instead imports individual objects from
lib/temporal.mjs, and I didn't see a quick way to fix that. Anyway, the
method is tested in the cookbook already.

Closes: #515
ptomato added a commit that referenced this issue Oct 5, 2020
This is in response to feedback that this conversion should be less
verbose.

No unit tests currently included because I found out that the test code
doesn't import the shim but instead imports individual objects from
lib/temporal.mjs, and I didn't see a quick way to fix that. Anyway, the
method is tested in the cookbook already.

Closes: #515
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
behavior Relating to behavior defined in the proposal documentation Additions to documentation enhancement non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants