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.p.toString using {timeZone: 'UTC'} option emits +00:00 offset, not Z as expected #2057

Closed
justingrant opened this issue Feb 13, 2022 · 4 comments
Labels
meeting-agenda normative Would be a normative change to the proposal spec-text Specification text involved

Comments

@justingrant
Copy link
Collaborator

While working on an update to the docs, I saw behavior of Instant.p.toString that I suspect is a behavior that we didn't intend.

s = '2022-02-28T03:06:00Z';
offset = Temporal.TimeZone.from(s); // => UTC
instant = Temporal.Instant.from(s);
instant.toString();
// => '2022-02-28T03:06:00Z'
instant.toZonedDateTimeISO('UTC');

instant.toString({ timeZone: offset });
// expected: '2022-02-28T03:06:00Z'
// actual: '2022-02-28T03:06:00+00:00'

I expected the UTC timezone to result in a "Z" string in the output, just like the default behavior if no timeZone option is provided.

I don't remember discussing how to handle the UTC timezone in the original design of the timeZone option (see #741 (comment); the meeting notes don't have enough detail to know for sure) but IIRC the intent of the option was to make it easy to round-trip timestamp strings. But the current behavior makes round-tripping harder, because the simplest round-tripping code fails to roundtrip strings with a Z offset.

roundtripTimestamp = s => Temporal.Instant.from(s).toString({ timeZone: s});
roundtripTimestamp('2022-02-28T03:06:00+01:00'); // =>  '2022-02-28T03:06:00+01:00'
roundtripTimestamp('2022-02-28T03:06:00Z'); // => '2022-02-28T03:06:00+00:00'
@ptomato
Copy link
Collaborator

ptomato commented Feb 14, 2022

This was intended at least by me; that was how I interpreted the consensus:

If the timeZone property is present, the result should be the offset format. If not, it should return the Z format.

I'd say the important thing for round-tripping is that you serialize a Temporal object into a string and then deserialize it back into a Temporal object, and get an identical object back, which is no problem in this case. I don't think we've been concerned about deserializing an object into a string and getting the same string back (which is already broken by, for example, case insensitivity, decimal separators, and basic vs. extended format.)

@justingrant
Copy link
Collaborator Author

I think the difference between case insensitivity, decimals, etc. is that those changes don't affect how other Temporal APIs behave. But +00:00 vs. Z offsets have different behavior in several Temporal APIs. So we'd end up with cases where round-tripping through Temporal.Instant.toString({timeZone}) would change behavior, not just cosmetic display:

sZ = '2022-02-28T03:06:00Z';
s0 = Temporal.Instant.from(sZ).toString({ timeZone: sZ});
userTz = 'Europe/Paris';

Temporal.ZonedDateTime.from(`${sZ}[${userTz}]`);  // OK
Temporal.ZonedDateTime.from(`${s0}[${userTz}]`);  // RangeError: Offset +00:00 is invalid for 2022-02-28T03:06:00 in Europe/Paris

Temporal.PlainDate.from(sZ); // RangeError: Z designator not supported for PlainDate
Temporal.PlainDate.from(s0); // OK

On the other hand, I could also see an argument that Instant.p.toString({timeZone}) is specifically designed to emit the numeric UTC offset, and if the user wants the Z then they can omit the option.

What do you think?

@ptomato
Copy link
Collaborator

ptomato commented Feb 15, 2022

I don't think appending a bracketed time zone to a string and feeding that to ZonedDateTime.from is something that we should encourage or support. We can't stop people from doing it, but it is very much not how Temporal is supposed to work, and I don't care if it breaks in some cases. I don't see it as any different from

sdate = plainDate.toString();
stime = plainTime.toString();
Temporal.PlainDateTime.from(`${sdate}T${stime}`);

which also works only some of the time but enough that people might do it carelessly, we also have no way to stop people from doing it, but is also very much not how Temporal is supposed to work.

I see the timeZone option when serializing Instant as a convenience if you need that particular string format with the UTC offset. It's somewhat out of place because in no way is an offset or time zone part of Instant's data model, but it's worth including because that string format would be inconvenient to get otherwise. Having the timeZone option sometimes not produce that string format and instead produce the same string format as omitting the option, seems counterproductive to me.

@ptomato ptomato added spec-text Specification text involved meeting-agenda normative Would be a normative change to the proposal labels May 6, 2022
@ptomato
Copy link
Collaborator

ptomato commented May 12, 2022

In the champions meeting of 2022-05-12 we decided not to change this behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meeting-agenda normative Would be a normative change to the proposal spec-text Specification text involved
Projects
None yet
Development

No branches or pull requests

2 participants