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

Should Absolute.prototype.toString(timeZone) have options to omit the offset or time zone? #741

Closed
justingrant opened this issue Jul 8, 2020 · 17 comments · Fixed by #1107
Assignees
Labels
documentation Additions to documentation non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved

Comments

@justingrant
Copy link
Collaborator

justingrant commented Jul 8, 2020

From: #716 (comment):

  • Future dates are mostly stored as (wall) datetime string and time zone (separately) => as offset might change
  • Past dates as datetime string with offset (or as timestamp) => offset might also change by IANA for past dates but we generally don't want those changes for something that happened in the past in our application

Currently Absolute.prototype.toString(timeZone) always emits both offset and IANA time zone identifier. But the feedback above raises a good point: some apps might know ahead of time that they want to store the time zone but not the offset, or the offset but not the time zone. It seems reasonable for Temporal to support those use cases.

Should Absolute.prototype.toString(timeZone) provide an option to prevent emitting of either the offset or the IANA time zone identifier? This would allow app devs to plan ahead and avoid any possible parse-time conflicts without requiring the additional complexity of managing two separate strings.

Here's an idea for syntax:

const abs = Temporal.now.absolute();
const noOffset1 = abs.toString('Europe/Paris', { offset: false });
const noIdentifier1 = abs.toString('Europe/Paris', { timeZone: false });

The same options object could be used by LocalDateTime.prototype.toString.

This issue is related to #716 (which is also about handling offset vs. timezone conflicts) but this issue deals with preventing the same conflict during persistence instead of during parsing like #716. Unfortunately, I don't think that one of them removes the need for other because persisted strings may be produced by or digested by non-Temporal code.

Note that omitting the offset would make persistence non-round-trippable because the same DateTime+TimeZone pair can correspond to two different Absolute values, e.g. after backwards DST transitions.

@thojanssens
Copy link

Supporting non-ISO datetime strings (with bracketed time zone) for parsing discussed in #716 is one thing. To me the only advantage is to support interop.

But outputting such strings, I'm really against. Worse to be able to omit the offset but keep the time zone? Are we inventing yet another new extension to ISO here?

ISO in any case should be the default for output. As mentioned in #716, sending a non-conform ISO string will make my server code crash for example. Having to add an option every time to stick to the ISO standard ({ timeZone: false }) seems so wrong to me.

@justingrant
Copy link
Collaborator Author

To clarify, the time zone suffix is only added when the timeZone parameter is supplied and an IANA time zone identifier is provided. Absolute.prototype.toString with no parameters will return an ISO string that ends with "Z". Examples:

Temporal.now.absolute().toString()
// => "2020-07-09T03:23:19.986994478Z"

Temporal.now.absolute().toString('-0700')
// => "2020-07-08T20:23:14.478676279-07:00"

Temporal.now.absolute().toString('America/Los_Angeles')
// => "2020-07-08T20:24:52.496999986-07:00[America/Los_Angeles]"

This issue is about adding optional behavior when an IANA time zone is supplied to toString(). Other cases won't change.

@thojanssens
Copy link

Dumping an ISO string for an absolute and a time zone is not trivial?

const absolute = Temporal.now.absolute()
absolute.toString(Temporal.TimeZone.from('America/Los_Angeles').getOffsetStringFor(absolute))

@justingrant
Copy link
Collaborator Author

Some feedback related to the default for the time zone display option:

@niklasR said at #703 (comment):

What I am more concerned about though, is the toString() output. The docs says that it "returns the date in ISO 8601 date format", for both DateTime and Absolute, and as far as I can see ISO 8601 does not include IANA names at all. If a timezone is specified in the toString(), the offset takes that into account, and surely we should stick with the standard?

@thojanssens said in #703 (comment):

And if we really want to output a non-ISO string (which I think we shouldn't because of my arguments in the other issues, see below)

Temporal.now.absolute().toString('America/Los_Angeles', { timeZone: true })

If we adopt the proposal in this issue, then it makes sense to keep this feedback in mind when we choose defaults.

Also, if LocalDateTime is adopted, then it could make sense to make { timeZone: false } the default for Absolute.prototype.toString() and { timeZone: true } the default for LocalDateTime.prototype.toString().

@thojanssens
Copy link

Also, if LocalDateTime is adopted, then it could make sense to make { timeZone: false } the default for Absolute.prototype.toString() and { timeZone: true } the default for LocalDateTime.prototype.toString().

That will be unintuitive. I call toString with a given time zone and I get an iso string without time zone. I call toString on an object containing a time zone but now the time zone is included in the string.

In both cases the time zone is provided, but the toString output is different. I understand it's called from a different object, but as soon as you give the time zone to absolute, you have the same information, why should the output be different:/

@ptomato
Copy link
Collaborator

ptomato commented Jul 16, 2020

After participating in the discussion in our weekly meeting about this, someone mentioned that serializing an Absolute with a time zone will no longer be necessary if we have (the type currently known as) LocalDateTime, because instead you would convert to LocalDateTime.

My preference would be to remove the time zone argument from Temporal.Absolute.toString(). The return value is always an ISO 8601 string with a Z.

I don't think I'd be in favour of an option to leave the IANA name out of the toString of LocalDateTime, and rather put the burden of that problem on the parsing side (#716). But maybe for the use case of someone who absolutely cannot have brackets past the end of their ISO string, we should have a separate toISOString method in LocalDateTime, or some ergonomic way, given an IANA time zone, to get the corresponding offset time zone for a particular Absolute, or both. (e.g. tz.getOffsetTimeZoneFor(absolute))

@justingrant
Copy link
Collaborator Author

justingrant commented Jul 16, 2020

My preference would be to remove the time zone argument from Temporal.Absolute.toString(). The return value is always an ISO 8601 string with a Z.

Agreed.

But maybe for the use case of someone who absolutely cannot have brackets past the end of their ISO string, we should have a separate toISOString method in LocalDateTime, or some ergonomic way, given an IANA time zone, to get the corresponding offset time zone for a particular Absolute, or both. (e.g. tz.getOffsetTimeZoneFor(absolute))

Agreed. Using the current LDT design, you'd do this:

`${ldt.toDateTime()}${ldt.timeZoneOffsetString}`

I'm not sure any further ergonomicity (is that a word?) is required. Especially not if #742 is resolved in a way that we can have a LocalDateTime.prototype.dateTime property:

`${ldt.dateTime}${ldt.timeZoneOffsetString}`

If we do offer a convenience method for this format, it should probably be on Absolute (or TimeZone), not LocalDateTime, because once you strip off the IANA identifier then you no longer have a LocalDateTime; that string wouldn't be valid for parsing back into LocalDateTime.from. It's just an Absolute with an offset. So I could see something like Absolute.prototype.toStringWithOffset or Absolute.prototype.toOffsetString(tz).

Also, I probably wouldn't call it getISOString, because Absolute.prototype.toString is already in ISO format. It's the offset that makes it different, not its ISO spec compatibility.

@thojanssens
Copy link

thojanssens commented Jul 17, 2020

I prefer @ptomato 's argument that Absolute should stick to UTC output, if we have a zoned datetime. Makes sense.

So I could see something like Absolute.prototype.toStringWithOffset or Absolute.prototype.toOffsetString(tz).

One could ask why don't we have a toStringWithTimeZone. I think these kind of functions reduce the quality of the API.

I'd rather see a toISOString() in the zoned datetime class. Yes, the output won't output enough information to get parsed again (we lose the tz Id), but the user would've called explicitly toISOString instead of toString; he is aware of that.

Also, I probably wouldn't call it getISOString, because Absolute.prototype.toString is already in ISO format.

That rejoins my point about working with non iso strings.

But if we suppose then that Temporal works with the extended format by default, in the end, the output for an absolute is also in the extended format, it's just that it doesn't have a time zone (well, utc).

In other words: all toString methods for any object return the format defined for Temporal. It just happens that the output of e.g. absolute is ISO compatible. Then we have an extra method in the zoned datetime for enforcing ISO. Makes all sense to me.

@justingrant
Copy link
Collaborator Author

In today's meeting, we tentatively decided to remove the timeZone argument from Absolute.prototype.toString() after LocalDateTime is added to Temporal.

@ptomato ptomato self-assigned this Aug 25, 2020
ptomato added a commit that referenced this issue Aug 25, 2020
Instead of choosing a time zone in which to serialize an Absolute, the
recommended way to keep a time stamp with a time zone is LocalDateTime
(either serialized or not.)

Closes: #741
@ptomato
Copy link
Collaborator

ptomato commented Aug 25, 2020

I have this in a branch at https://github.com/tc39/proposal-temporal/tree/716-741-absolute-zone-names but it should not be merged until LocalDateTime lands.

ptomato added a commit that referenced this issue Aug 26, 2020
Instead of choosing a time zone in which to serialize an Absolute, the
recommended way to keep a time stamp with a time zone is LocalDateTime
(either serialized or not.)

Closes: #741
@ptomato
Copy link
Collaborator

ptomato commented Sep 2, 2020

The new proposal (#703 (comment)) is to keep the time zone argument in Absolute.toString but only print the offset.

@ptomato
Copy link
Collaborator

ptomato commented Sep 2, 2020

(That seems fine to me.)

ptomato added a commit that referenced this issue Sep 2, 2020
Instead of choosing a time zone in which to serialize an Absolute, the
recommended way to keep a time stamp with a time zone is LocalDateTime
(either serialized or not.)

Closes: #741
@ptomato
Copy link
Collaborator

ptomato commented Sep 3, 2020

A counter proposal from today's meeting was that the time zone argument should be dropped, since we are getting a way to emit a string with an offset and no bracket in LocalDateTime.toString already.

After considering it, I personally am convinced to stick to Justin and Philipp's last proposal, to keep the time zone argument but only print the offset. The reason being that Absolute.from accepts a date/time/offset combination, Absolute.toString should also emit it.

@justingrant
Copy link
Collaborator Author

Agreed. The basic idea is that Absolute has two valid canonical formats: one that ends in Z and another that ends in an offset. "Z" is the default, but if you're interoperating with another system that uses the other format, then you should be able to round-trip that format in both parsing and toString.

@justingrant
Copy link
Collaborator Author

justingrant commented Sep 4, 2020

2020-09-04 meeting decision:

  • toString will take an optional property bag option
  • The property bag will accept an optional timeZone property
  • If the timeZone property is present, the result should be the offset format. If not, it should return the Z format.
  • If the timeZone property is present, it must be a TimeZoneLike (string | TimeZoneProtocol). If undefined, emit the Z format (not the system time zone!).
  • Brackets should never be included in the result.

@ljharb
Copy link
Member

ljharb commented Sep 4, 2020

I think it's very unidiomatic to have "absent" and undefined do different things; the language tries very hard not to distinguish these in API methods. null should throw, but imo undefined should indeed use the default.

@justingrant
Copy link
Collaborator Author

I think it's very unidiomatic to have "absent" and undefined do different things; the language tries very hard not to distinguish these in API methods. null should throw, but imo undefined should indeed use the default.

Oops, agreed. I edited the comment to the specify the idiomatic behavior.

@ptomato ptomato added this to the Stable proposal milestone Sep 17, 2020
@ptomato ptomato added documentation Additions to documentation spec-text Specification text involved non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! labels Sep 18, 2020
ptomato added a commit that referenced this issue Nov 2, 2020
I've taken the liberty of renaming this, otherwise it has a conflicting
meaning compared with the 'timeZone' option passed to Instant.toString()
(which will be added in #741.)

Making the distinction between 'timeZone' and 'timeZoneName' in this way
aligns exactly with what Intl.DateTimeFormat does.

See: #703
ptomato added a commit that referenced this issue Nov 2, 2020
I've taken the liberty of renaming this, otherwise it has a conflicting
meaning compared with the 'timeZone' option passed to Instant.toString()
(which will be added in #741.)

Making the distinction between 'timeZone' and 'timeZoneName' in this way
aligns exactly with what Intl.DateTimeFormat does.

See: #703
ptomato added a commit that referenced this issue Nov 2, 2020
I've taken the liberty of renaming this, otherwise it has a conflicting
meaning compared with the 'timeZone' option passed to Instant.toString()
(which will be added in #741.)

Making the distinction between 'timeZone' and 'timeZoneName' in this way
aligns exactly with what Intl.DateTimeFormat does.

See: #703
Ms2ger pushed a commit that referenced this issue Nov 3, 2020
I've taken the liberty of renaming this, otherwise it has a conflicting
meaning compared with the 'timeZone' option passed to Instant.toString()
(which will be added in #741.)

Making the distinction between 'timeZone' and 'timeZoneName' in this way
aligns exactly with what Intl.DateTimeFormat does.

See: #703
ptomato added a commit that referenced this issue Nov 3, 2020
This adds an option for the time zone in which to print an Instant,
instead of having it be a separate argument. Also removes printing the
bracketed name from Instant.

Closes: #741
ptomato added a commit that referenced this issue Nov 3, 2020
This adds an option for the time zone in which to print an Instant,
instead of having it be a separate argument. Also removes printing the
bracketed name from Instant.

Closes: #741
ptomato added a commit that referenced this issue Nov 3, 2020
This adds an option for the time zone in which to print an Instant,
instead of having it be a separate argument. Also removes printing the
bracketed name from Instant.

Closes: #741
ptomato added a commit that referenced this issue Nov 4, 2020
I missed these in #741 because of the CI job failing.
Ms2ger pushed a commit that referenced this issue Nov 4, 2020
I missed these in #741 because of the CI job failing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Additions to documentation 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.

4 participants