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 time zone offsets be signed Durations? #935

Closed
gibson042 opened this issue Sep 22, 2020 · 21 comments · Fixed by #1035
Closed

Should time zone offsets be signed Durations? #935

gibson042 opened this issue Sep 22, 2020 · 21 comments · Fixed by #1035
Assignees
Labels
documentation Additions to documentation non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved

Comments

@gibson042
Copy link
Collaborator

gibson042 commented Sep 22, 2020

getOffsetNanosecondsFor and timeZoneOffsetNanoseconds seem tremendously cumbersome for the overwhelming majority of time zone offsets that have zero values for seconds and fractional seconds. Rather than propagating huge integers, perhaps we should be dogfooding Temporal.Duration instances and letting any author who actually wants nanoseconds to use round.

@gibson042 gibson042 changed the title Should timeZoneOffsetNanoseconds be replaced with a signed Duration? Should time zone offsets be signed Durations? Sep 22, 2020
@justingrant
Copy link
Collaborator

justingrant commented Sep 23, 2020

Interesting idea. Seems like a nice usability win if we can make it work. The main concern I'd have for this would be ambiguity in the string format between PT5H30M and +05:30. Given our new pattern where strings are valid anywhere a Temporal instance is allowed, will users be able to pass either string format? Should this be considered an alternate format for Duration generally?

If do end up parsing +05:30 as a duration generally, some issues to work through:

  • Is the sign required? What about 05:30?
  • AFAIK +02 is also a valid ISO 8601 offset. Would this also be parseable into a Duration?
  • Are leading zeroes always required, or is +5:30 or 5:30 also OK?
  • Are we worried about potential ambiguity of 05:30 (as a general format for Duration) meaning PT5H30M or PT5M30S

Note that I'm not arguing that we should parse offsets into Duration outside the context of TimeZone and LDT. I don't have an opinion about that question. But I am pointing out some questions to address if we did it.

Also, what about ldt.duration.toString()? Will it be weird if that format doesn't match .timeZoneOffsetString? Should we add a toString option to format it as a time zone offset?

What about localDateTime.prototype.toJSON? Would the offset property value be PT5H30M or +05:30?

If we made this change, then we'd probably want to rename the property to timeZoneOffset or just offset.

@justingrant
Copy link
Collaborator

justingrant commented Sep 23, 2020

Also, I just remembered why I specified this property as a number in the first place: because non-scalar fields are harder to deal with (SES, lazy initialization, can't use === or <, etc) Given that setting the offset is a really unusual use case, we'd have to see if the complexity is worth it.

@ptomato
Copy link
Collaborator

ptomato commented Sep 25, 2020

I'm not opposed to changing offset-nanoseconds to offset-duration throughout, and s/offsetNanoseconds/offset/. We had a similar discussion in #349 but at the time durations were not signed.

If we do this, I do feel strongly that we should not add "+05:30" as a sanctioned string serialization format for Duration. Rather, keep it exclusively as part of an ISO string, and if you need it separately there's always timeZone.getOffsetStringFor().

@pipobscure
Copy link
Collaborator

getOffset() gets the string, and drop getOffsetNanoseconds()

@ptomato ptomato added documentation Additions to documentation non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved labels Oct 2, 2020
@justingrant
Copy link
Collaborator

justingrant commented Oct 3, 2020

Here's consensus I heard at 2020-10-02 meeting. (1)-(5) are straightforward. (6) requires decisions.

Make the following changes to the ZonedDateTime type:

  1. Remove timeZoneOffsetNanoseconds field

  2. Rename timeZoneOffsetString property to offset and make it a field to replace the numeric one we just deleted

  3. with and from will accept canonical (e.g. +05:30) non-canonical forms (e.g. +02) of the offset string

  4. getFields and .offset will always emit the canonical form of the offset string

  5. offset will be a string, not a Duration

  6. ISSUE: how should we handle the case where a time zone has an offset that's not aligned on a one-minute granularity?

  • Option 6.1: we don't support offsets more granular than minutes. Creating a new TimeZone instance with the offset nanoseconds not aligned on a minute boundary will throw.
  • Option 6.2: we come up with a new string format for sub-minute offsets
  • Option 6.3: the offset property truncates or rounds to the minute, which means that ZonedDateTime instances won't be round-trippable via ZonedDateTime.from(zdt.getFields()).
  • Option 6.4: offset and getFields() throw if the offset isn't on a minute boundary. This avoids the round-trip issue at the cost of randomly crashing inside library code if you pass it a custom time zone with a weird offset.

BTW, I realized after the meeting that the unappetizing options in (6) was the main reason why I originally opted to use the nanoseconds property instead of a string.

  1. Based on the changes above, should anything change about the APIs on TimeZoneProtocol too? If so, what should change?

@gibson042
Copy link
Collaborator Author

I would vote for 6.3, and hopefully the result can round-trip with offset: 'prefer' (but if not, then c'est la vie). As for 7, the TimeZone protocol should still support nanosecond precision—this is a "porcelain" convenience, but the underlying plumbing ought to remain complete (if ugly).

@ryzokuken
Copy link
Member

with and from will accept canonical (e.g. +05:30) non-canonical forms (e.g. +02) of the offset string

Could you please remind me why this was deemed necessary?

Regarding the options, I'd personally prefer 6.1 because we've failed to come up with (IIRC) use-cases for sub-minute offsets. 6.4 is less preferable, because it's essentially the same, but passive-aggressive, which breaks the "err sooner rather than later" principle. 6.2 is fine, as long as we move ahead with the format on the standards track similar to the calendars discussion.

@gilmoreorless
Copy link
Contributor

  1. Remove timeZoneOffsetNanoseconds field
  2. offset will be a string, not a Duration

Maybe I've missed something here, but does that mean there's no longer a way to get the offset as a numeric value? If so, that's a complete reversal of several previous discussions.

#349 and #579 both questioned why the offset was previously only returned as a string instead of a numeric value. (I'm using "numeric value" to cover both nanoseconds and Durations.) When I first looked over the cookbook examples I also questioned this — #240 (comment)

UTC offset for a zoned event, as a number of seconds — I'm really surprised this isn't part of the Temporal API, especially given the complexity of getting the value. I mainly say that because the legacy Date returns the offset as a numeric value via .getTimezoneOffset() (albeit as minutes in the "wrong" direction). It would be odd to say that Temporal is a replacement for Date while not providing equivalent functionality for the offset.

I also saw in the meeting notes about this decision that "It's not like you're doing maths with the offset."
I can think of at least 3 data visualisation projects I've done that have required doing maths with a date/time and/or time zone's offset. Translating an offset into an x/y coordinate on a graph is going to be a real pain if there's only a string to work with.

(Of course, if I have missed something, and a numeric value is still easy to obtain, then feel free to ignore everything above. 😄)

@gilmoreorless
Copy link
Contributor

Oh also...

Option 6.1: we don't support offsets more granular than minutes. Creating a new TimeZone instance with the offset nanoseconds not aligned on a minute boundary will throw.

That will cause problems with existing IANA zones. See #339 (comment)

@ptomato
Copy link
Collaborator

ptomato commented Oct 5, 2020

does that mean there's no longer a way to get the offset as a numeric value?

You can still get the numeric value from the TimeZone object, with tz.getOffsetNanosecondsFor(instant). You just can't get it from the ZonedDateTime object directly. (That means "no" on point number 7)

@gilmoreorless
Copy link
Contributor

Hrm, so to mimic an existing usage of the legacy Date...

Instead of getting the numeric offset directly, I have to get the time zone object, then ask it for the numeric offset of an instant that I also have to get from the original object.

// Before
someZonedDateTime.timeZoneOffsetNanoseconds
// After
someZonedDateTime.timeZone.getOffsetNanosecondsFor(someZonedDateTime.instant)

That will work for my use cases. But frankly, I don't see how this helps resolve feedback about the API being too verbose.

@justingrant
Copy link
Collaborator

(Bundling up responses to multiple folks.)

@gilmoreorless That will cause problems with existing IANA zones. See #339 (comment)

Good points. I'd forgotten about historical timezones with sub-minute offsets. This seems like a good reason to support round-tripping ZDT instances via strings that use the actual offsets. Otherwise you can end up with weird results, e.g. times that should be equal will return .equals(other)=== false.

@ryzokuken 6.2 is fine, as long as we move ahead with the format on the standards track similar to the calendars discussion.

After thinking about this problem more, I think 6.2 this is the best option: if there's a sub-minute offset (which currently is only for historical values), then we should emit it in toString() e.g. '+12:30:45.123456789'. And we should work with standards bodies to accept that format. Without doing this, then ZDT string persistence would be lossy for these values, which is very unexpected and AFAIK unlike all other Temporal types. We could always add a toString() option to hide the (currently) non-standard offset values.

@gilmoreorless I also saw in the meeting notes about this decision that "It's not like you're doing maths with the offset."
I can think of at least 3 data visualisation projects I've done that have required doing maths with a date/time and/or time zone's offset. Translating an offset into an x/y coordinate on a graph is going to be a real pain if there's only a string to work with.

I agree. Having the offset as a numeric value is helpful. Regardless of what we decide with this issue, it seems helpful to retain the numeric convenience property on ZonedDateTime, e.g. .offsetNanoseconds.

Another thing that occurred to me: parsing offset strings is relatively common. Should there be static convenience methods on TimeZone for this purpose, e.g. convertOffsetStringToNanoseconds() and convertOffsetNanosecondsToString()?

@gilmoreorless But frankly, I don't see how this helps resolve feedback about the API being too verbose.

Good point. Regardless of which property is a field it seems sensible to keep both properties and to make the property names shorter, e.g. timeZoneOffsetNanoseconds => offsetNanoseconds and timeZoneOffsetString => offsetString

@gibson042 I would vote for 6.3, and hopefully the result can round-trip with offset: 'prefer' (but if not, then c'est la vie).

IMHO the main round-trip issue is string serialization, which is not helped by the offset option.

with and from will accept canonical (e.g. +05:30) non-canonical forms (e.g. +02) of the offset string

@ryzokuken Could you please remind me why this was deemed necessary?

IMHO, it would be weird if with (and the property-bag form of from) didn't accept the same string format that the string form of from does. This is analogous to how we handle timezone IDs, where the strings are always canonicalized.

@gibson042
Copy link
Collaborator Author

Remember that this is a concern about nanoseconds being overly cumbersome for the overwhelming majority of offsets that are in fact aligned on 15-minute boundaries (and all but a handful of those actually on hour boundaries). I proposed Duration as a means of simplifying common cases without precluding the uncommon ones (e.g., zdt.getOffset().round({largestUnit: "nanoseconds"}) rather than zdt.timeZoneOffsetNanoseconds), but the champions decided to expose offset strings in lieu of numbers. Standards action to allow serialization of sub-minute offsets as in "1936-01-01T00:00+00:19:32.13[Europe/Amsterdam]" would be great, but I guess it wouldn't be too bad if Temporal supports that even without such upstream changes.

@justingrant
Copy link
Collaborator

Sounds like more discussion may be needed to finalize the decisions in this issue. I added it back to the design milestone.

BTW, in the meantime I updated the ZDT prototype and proposal in #700, as well as the ZDT docs in #979, to shrink the property names to offsetString and offsetNanoseconds, because at least those changes seem non-controversial.

@ptomato
Copy link
Collaborator

ptomato commented Oct 13, 2020

Can we see if we can resolve this online without waiting until the next meeting? Would a combination of Richard's and my comments have broader support? Namely:

  • (6) how should we handle the case where a time zone has an offset that's not aligned on a one-minute granularity?
    • The offset property is a ±HH:MM string, so it would not round-trip unless you specified offset: 'prefer' in the options.
  • (7) should anything change about the APIs on TimeZoneProtocol too?
    • No.

@justingrant
Copy link
Collaborator

Yep, I'd like to resolve online too! I dug into the options of (6) and I think it's actually more complicated than the 4 original options in #935 (comment).

So here's an updated set of decisions to make. Some combinations of choices aren't valid, but some of these decisions are mutually exclusive. LMK if I missed any decisions or options and we can iterate.

1. How should Temporal behave when executing a TimeZone method for an Instant that has a sub-minute offset in that time zone?

  • 1a. Don't throw, and return or use the full precision
  • 1b. Don't throw, but require any compliant TimeZone implementation (built-in: via our code and spec; custom: via docs and assertions in Temporal code that uses them) to round or truncate the offset to the nearest minute inside any any Temporal operation that uses the offset, including ZDT's offset and offsetNanoseconds properties and all TimeZone methods. Note that this option, if chosen, probably makes most of the choices below moot.
  • 1c. Throw. Note that this option, if chosen, probably makes most of the choices below moot.

2. How should ZonedDateTime's and Instant's toString behave with sub-minute offsets in the TimeZone?

  • 2a. Emit full precision (e.g. 1936-01-01T10:23+00:19:32.13[Europe/Amsterdam]), with an option to limit precision to match current ISO standard just like we will have options to omit the offset or the bracketed zone from the output.
  • 2b. Truncate or round the offset to the nearest minute, but with an option to emit full precision. Note that if the standard is later expanded to support sub-minute offsets, this option would probably prevent us from adopting it by default because changing behavior would break existing apps. I'd be concerned about this lack of forwards-compatibility.
  • 2c. Throw by default and require an option to define whether the offset should be displayed with full precision or not. This seems like a safer option than (2b) because it better maintains forwards compatibility.

3. How should Instant.from(s: string) behave with sub-minute offsets in the string?

  • 3a. Accept with full precision, and work to standardize the sub-minute offset format
  • 3b. Throw because it's not in the current standard and the ISO string is invalid.
  • 3c. Throw by default because it's not in the current standard and the ISO string is invalid, but provide an option to allow the full precision to be parsed.
  • 3d. Round or truncate the offset to the nearest minute by default, but provide an option to throw in this case.
  • 3e. Round or truncate the offset to the nearest minute by default, without an option to throw in this case.

4. How should ZonedDateTime.from(s: string) behave with sub-minute offsets in the string?

  • 4a. Accept with full precision, and work to standardize the sub-minute offset format. Don't provide any options to throw with sub-minute offsets.
  • 4b. Throw because it's not in the current standard and the ISO string is invalid, regardless of the offset option used.
  • 4c. Throw by default, but the caller could use offset: 'use' to opt into parsing the sub-minute offset (at the cost of losing the ability to identify cases where timezone changes caused an offset vs. timezone conflict) or offset: 'prefer' | 'ignore' to ignore the offset and use the TimeZone's offset (at the cost of losing the ability to round-trip clock times that are repeated by backward transitions AND losing the ability to identify cases where timezone changes caused an offset vs. timezone conflict.)

5. How should ZonedDateTime.from(o: object) behave with sub-minute offsets?

  • 5a. Accept with full precision, and work to standardize the sub-minute offset format. Don't provide any options to throw with sub-minute offsets.
  • 5b. Throw because sub-minute offset strings are invalid, regardless of the offset option used.
  • 5c. Throw by default, but the caller can use offset: 'prefer' | 'ignore' to ignore the offset and use the TimeZone's offset, at the cost of losing the ability to round-trip clock times that are repeated by backward transitions AND losing the ability to identify cases where timezone changes caused an offset vs. timezone conflict.
  • 5d. Same as (5c), but also allow offset: 'use' to opt into parsing the sub-minute offset, at the cost of losing the ability to identify cases where timezone changes caused an offset vs. timezone conflict.

6. How should ZonedDateTime.with({ offset }) behave with sub-minute offsets?

  • 6a. Accept with full precision, and work to standardize the sub-minute offset format. Don't provide any options to throw with sub-minute offsets.
  • 6b. Throw because sub-minute offset strings are invalid, regardless of the offset option used.
  • 6c. The default prefer behavior will pull the sub-minute offset from time zone, at the cost of losing the ability to round-trip clock times that are repeated by backward transitions AND losing the ability to identify cases where timezone changes caused an offset vs. timezone conflict. The user can force throwing using offset: 'reject'.
  • 6d. Same as (6c), but also allow offset: 'use' to opt into parsing the sub-minute offset, at the cost of losing the ability to identify cases where timezone changes caused an offset vs. timezone conflict.

7. How should ZonedDateTime.from (either string or object form) behave when the offset in the input string is on a minute boundary but the TimeZone has a sub-minute offset?

  • 7a. Don't throw, and silently use the TimeZone's sub-minute offset (not the offset in the string) if the TimeZone's offset is <= 60 seconds different from the string's offset. Note that this would lead to different output of Instant.from depending on whether the IANA suffix is present in the input string. IMHO, this is probably (not 100% sure) better than unexpectedly throwing when a valid string is parsed.
  • 7b. Throw because the offset doesn't match the TimeZone and offset: 'reject' is the default option. The caller could use offset: 'ignore' or offset: 'prefer' to get the sub-minute offset from the TimeZone instead, at the cost of losing the ability to detect cases where timezone changes caused an offset vs. timezone conflict AND losing the ability to parse any clock times repeated by a backwards transition.

8. How should ZonedDateTime's string offset property behave with sub-minute offsets?

  • 8a. Emit full precision
  • 8b. Round or truncate to the nearest minute
  • 8c. Throw

9. How should ZonedDateTime's offsetNanoseconds property behave with sub-minute offsets in the TimeZone?

  • 9a. Emit full precision
  • 9b. Round or truncate to the nearest minute
  • 9c. Throw

There's no perfect choice here. My current medium-strength preference would be to pick the (a) options for all of them, meaning:

  • Work to standardize the sub-minute-offset syntax alongside other ISO standards extensions we're proposing like timezones and calendars.
  • Emit and parse sub-minute offsets by default. This behavior aligns with how we treat other extensions to the ISO standard like our bracketed timezone syntax, where the default behavior enables lossless round-tripping.
  • Provide toString options to emit minute-only offsets, similar to how we provide other options to turn off other standards extensions.
  • Silently accept and adjust minute-aligned offsets when parsing if the TimeZone is within 60 seconds, to make it easier to parse standardized times from other platforms without exceptions.

My second choice is probably (2a) which is to force all TimeZone instances (built-in and custom) to only use minute-aligned offsets. This would presumably foreclose the ability to have a TAI timezone or other leap-second solutions via custom timezones.

Currently, my only strong opinion would be to be NOT choose (2b) because it'd prevent us from matching a future standard where sub-minute offsets are allowed.

Let's discuss.

@gibson042
Copy link
Collaborator Author

Having mulled things over more, I think the deviation from ISO 8601 to include increased offset precision in the obvious way (and pushing for standardization thereof) makes the most sense.
1a, 2a, 3a (with option to disable parsing the not-technically-ISO strings, e.g. "strict"), 4?? (should match 3 with the added internal consistency restrictions), 5?? (should match 4), 6?? (should match from), 7?? (existing consistency restrictions), 8a, 9a. In short, I think I agree with @justingrant but there may be some differences down in the weeds.

@justingrant
Copy link
Collaborator

Cool. If I understand your proposal correctly, I have a concern:

3a (with option to disable parsing the not-technically-ISO strings, e.g. "strict")

I see a lot of value in limiting toString to the current standard because other platforms will expect it, even after new standards are defined. I'm less sure that for parsing there's enough value to warrant an extra option for this case.

If the option is expected to change behavior if a new standard is approved (which it'd probably be if its name were strict) then we'd risk breaking apps that depended on the previous behavior. So if we did do this then I'd recommend instead to have an option named something like acceptSubMinuteOffset with default of true whose behavior didn't change under a new standard.

But I'm not sure that this option is needed on entry into Temporal given how rare it is and given that any app concerned about interop will already be restricting the output via toString. So whether or not we had an option on parsing, the output would be identical.

Also, there's a trivial userland workaround for developers who really care about this:

if (ZonedDateTime.from(s).offsetNanoseconds % 6e10) throw new RangeError('invalid offset');

Also, we could wait to see if lack of a parsing option generates lots of complaints, and if it does then such an option could always be added later.

Finally, there will always be "stickler" developers who will see that there's an option for standards compliance and will mandate it for their codebases. This would make it harder for them to transition if a new standard is approved.

@gibson042
Copy link
Collaborator Author

OK, I find that argument convincing.

@justingrant
Copy link
Collaborator

Meeting 2020-10-15:

justingrant added a commit to justingrant/proposal-temporal that referenced this issue Oct 19, 2020
* Shorten `offsetString` property to just `offset`
* Remove `offsetNanoseconds` from ZDT fields. Replace it with `offset`.
* NOTE: subminute offsets aren't supported yet, but will be later (tc39#935)
justingrant added a commit to justingrant/proposal-temporal that referenced this issue Oct 19, 2020
* Shorten `offsetString` property to just `offset`
* Remove `offsetNanoseconds` from ZDT fields. Replace it with `offset`.
* NOTE: subminute offsets aren't supported yet, but will be later (tc39#935)
@ptomato ptomato self-assigned this Oct 20, 2020
@ptomato
Copy link
Collaborator

ptomato commented Oct 20, 2020

For avoidance of doubt: the historical mean time of Amsterdam is indeed UTC+00:19:32.13, but the tzdata database only supports whole-second offsets, so it is rounded to UTC+00:19:32. I don't think anything needs to change about the resolution of this issue.

justingrant added a commit to justingrant/proposal-temporal that referenced this issue Oct 21, 2020
* Shorten `offsetString` property to just `offset`
* Remove `offsetNanoseconds` from ZDT fields. Replace it with `offset`.
* NOTE: subminute offsets aren't supported yet, but will be later (tc39#935)
ptomato added a commit that referenced this issue Oct 22, 2020
This is necessary for round-tripping of some named time zones. In
particular we are testing with Europe/Amsterdam before 1937.

See: #935
ptomato added a commit that referenced this issue Oct 22, 2020
Allows time zone offsets such as +00:19:32.13 in Temporal.Instant.from,
Temporal.TimeZone.from, and new Temporal.TimeZone.

See: #935
ryzokuken pushed a commit that referenced this issue Oct 22, 2020
This is necessary for round-tripping of some named time zones. In
particular we are testing with Europe/Amsterdam before 1937.

See: #935
ryzokuken pushed a commit that referenced this issue Oct 22, 2020
Allows time zone offsets such as +00:19:32.13 in Temporal.Instant.from,
Temporal.TimeZone.from, and new Temporal.TimeZone.

See: #935
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.

6 participants