Conflict between docs and polyfill for cloning using TimeZone.from and Calendar.from

[justingrant]

The TimeZone docs say:

This static method creates a new time zone from another value. If the value is another Temporal.TimeZone object, a new object representing the same time zone is returned.

But this isn't actually true in the current polyfill implementation. If the argument is an object, then it's returned as-is without creating a new object.

proposal-temporal/polyfill/lib/timezone.mjs

Lines 244 to 251 in c929bd1

	static from(item) {
	if (typeof item === 'object' && item) return item;
	const timeZone = ES.TemporalTimeZoneFromString(ES.ToString(item));
	const result = new this(timeZone);
	if (!ES.IsTemporalTimeZone(result)) throw new TypeError('invalid result');
	return result;
	}
	}

Which one is correct?

The Calendar class has the same docs vs. code mismatch:

This static method creates a new calendar from another value. If the value is another Temporal.Calendar object, a new object representing the same calendar is returned.

proposal-temporal/polyfill/lib/calendar.mjs

Lines 107 to 112 in c929bd1

	static from(item) {
	if (ES.IsTemporalCalendar(item) || (typeof item === 'object' && item)) return item;
	const stringIdent = ES.ToString(item);
	return GetBuiltinCalendar(stringIdent);
	}
	}

BTW, I found this because I was wondering about the TS signature of TimeZone.from. Currently it's this:

static from(timeZone: Temporal.TimeZone | string): Temporal.TimeZone;

But shouldn't it accept a Temporal.TimeZoneProtocol | string instead, to support cloning custom time zone objects? Or is this use case deliberately excluded because we don't want to clone objects that Temporal didn't create? If so, then should the polyfill enforce that exclusion by throwing if an object is passed that's not a Temporal.TimeZone? I assume whatever we do here we'd also want to do in Calendar. FWIW, the Calendar class accepts a CalendarProtocol in its TS typings for from.

[gibson042]

from functions should always return new objects; cf. #341 and #232.

[justingrant]

@gibson042 - what about custom timezone and calendar implementations? Should from deep-clone those objects? Shallow-clone? Throw?

[ptomato]

The current behaviour is on fairly shaky ground because of custom time zone and calendar implementations. I think we don't want to make people override Temporal.{TimeZone,Calendar}.from if they don't have to, so cloning them based on their names would be difficult.

@Ms2ger has been advocating for making TimeZone.from and Calendar.from not return new objects for that reason.

[gibson042]

I'm not familiar with the current data models for those types, but the primary purpose of from is deserialization so throwing an exception seems like a good starting point that can be adjusted if necessary.

[ryzokuken]

I thought from accepted objects for the sole purpose of turning foo-like options bags into an instance of class foo. What is the use-case for passing existing instances or protocol objects into from? Also, thinking about it more, while it clearly makes sense to have from accept said options bags for Date, Time, et. al., what is the use-case of passing an object in TimeZone.from or Calendar.from in the first place?

[ptomato]

Yeah, that makes sense.

[justingrant]

what is the use-case of passing an object in TimeZone.from or Calendar.from in the first place?
What is the use-case for passing existing instances or protocol objects into from?

I ran into this when I was trying to clone Temporal objects. That's probably the only use case.

[justingrant]

Meeting 2020-08-13: consensus is that Temporal.Calendar.from and Temporal.TimeZone.from should no longer accept objects, only strings.

One thing that I don't think we discussed in the meeting: should the implementation of methods like LocalDateTime.from or Date.from clone any Temporal.Calendar or Temporal.TimeZone objects provided by users (by extracting their string ids and passing them to the constructor or from) before storing them? Or is cloning unnecessary because we can't do it for user timezones or calendars anyway?

[ptomato]

I think cloning should be unnecessary in those cases, in order to be consistent with custom calendars.

[ptomato]

@Ms2ger I started on this before I noticed you had already started. Luckily nothing I did overlapped with what you had already done, because I started with the documentation. I pushed my work to your branch, feel free to squash it into yours.

[justingrant]

Don't forget to update the TS types to remove object types. Thanks!

[justingrant]

One thing that I don't think we discussed in the meeting: should the implementation of methods like LocalDateTime.from or Date.from clone any Temporal.Calendar or Temporal.TimeZone objects provided by users (by extracting their string ids and passing them to the constructor or from) before storing them? Or is cloning unnecessary because we can't do it for user timezones or calendars anyway?

@ptomato I think cloning should be unnecessary in those cases, in order to be consistent with custom calendars.

TypeScript helped me find a problem with this approach. Some methods on TimeZoneProtocol are optional. Here's the current TS declaration:

  export interface TimeZoneProtocol {
    id?: string;
    getOffsetNanosecondsFor(instant: Temporal.Instant): number;
    getOffsetStringFor?(instant: Temporal.Instant): string;
    getZonedDateTimeFor?(instant: Temporal.Instant, calendar?: CalendarProtocol | string): Temporal.ZonedDateTime;
    getDateTimeFor(instant: Temporal.Instant, calendar?: CalendarProtocol | string): Temporal.DateTime;
    getInstantFor?(dateTime: Temporal.DateTime, options?: ToInstantOptions): Temporal.Instant;
    getNextTransition?(startingPoint: Temporal.Instant): Temporal.Instant | null;
    getPreviousTransition?(startingPoint: Temporal.Instant): Temporal.Instant | null;
    getPossibleInstantsFor(dateTime: Temporal.DateTime): Temporal.Instant[];
    toString(): string;
    toJSON?(): string;
  }

Assuming that this declaration is correct, then the only required methods are getOffsetNanosecondsFor, getDateTimeFor, getPossibleInstantsFor, and toString.

Before ZonedDateTime this was not a problem because custom time zone objects are not persisted anywhere. So the only place you could get a TimeZoneProtocol was userland code.

But with ZonedDateTime, there's now a timeZone property on every ZDT instance. If this property is typed as a Temporal.TimeZone then where do the other properties and methods come from? However, if this property is typed as a TimeZoneProtocol, then the timeZone property will be severely handicapped because userland code can't safely call all methods, only those 4 required ones.

I can think of two ways to solve this:

1. Change TimeZoneProtocol so that implementing all methods are required. The easy way for implementers to satisfy this would be to inherit from TimeZone. If they want cross-realm support then they'd need to implement each method themselves.
2. Change ZonedDateTime's constructor to clone the TimeZone instance and add any methods that are missing with implementations that will work across realms. I'm not sure if this is actually possible, but I'm sure @gibson042 will know.

I assume that (1) seems like the safer approach, given that custom timezones is an advanced use case. Making it harder seems like a reasonable compromise to better support non-advanced use cases of calling TimeZone methods and properties.

[justingrant]

Despite today's long discussion, I wonder if we're not actually disagreeing on the outcome? For example, I wrote up some docs content for the custom time zone docs. Would the content below be OK for the custom time zone docs?

Temporal's internal implementation only requires the following four methods from a custom time zone:

getOffsetNanosecondsFor(instant: Temporal.Instant): number;
getDateTimeFor(instant: Temporal.Instant, calendar?: CalendarProtocol | string): Temporal.DateTime;
getPossibleInstantsFor(dateTime: Temporal.DateTime): Temporal.Instant[];
toString(): string;

Any custom time zone implementation that implements those four methods will return the correct output from any Temporal property or method. If you're writing a custom time zone and you're only calling Temporal code (not anyone else's code that also makes Temporal calls) then only the four methods above are sufficient.

However, most userland code will assume that custom time zones act like built-in Temporal.TimeZone objects, which implement additional properties and methods. If you want to interoperate with userland libraries or other code that you didn't write, then you should also implement the properties and methods below:

id: string;
getOffsetStringFor(instant: Temporal.Instant): string;
getZonedDateTimeFor(instant: Temporal.Instant, calendar?: CalendarProtocol | string): Temporal.ZonedDateTime;
getInstantFor(dateTime: Temporal.DateTime, options?: ToInstantOptions): Temporal.Instant;
getNextTransition(startingPoint: Temporal.Instant): Temporal.Instant | null;
getPreviousTransition(startingPoint: Temporal.Instant): Temporal.Instant | null;
toJSON(): string;

The easiest way to implement these properties and methods is to subclass the built-in Temporal.TimeZone which contains the following default implementations:

Members	Default Implementation
id, toJSON	Returns the result of toString()
getNextTransition, getPreviousTransition	Returns null
getOffsetStringFor	Creates an offset string using the result of getOffsetNanosecondsFor
getZonedDateTimeFor	Creates a Temporal.ZonedDateTime instance using this and the provided Temporal.Instant and Temporal.Calendar inputs.
getInstantFor	Creates a Temporal.Instant instance by calling getPossibleInstantsFor and, if there's more than one, using the provided disambiguation option to pick the correct result.

Here's an example of how this works, using a fictional time zone whose offset is always 3:14:15.926 behind UTC before UNIX epoch and 3:14:15.926 ahead of UTC after UNIX epoch.

const beforeEpochZone = new Temporal.TimeZone('-03:14:15.926');
const afterEpochZone = new Temporal.TimeZone('+03:14:15.926');
const OFFSET_NS = Temporal.Duration.from('PT3H14M15.926S').total({ unit: 'nanoseconds' });
class PiZone extends Temporal.TimeZone {
  constructor() {
    super('π');
  }
  getOffsetNanosecondsFor(instant) {
    return instant.offsetNanoseconds > 0 ? OFFSET_NS : -OFFSET_NS;
  }
  getDateTimeFor(instant, calendar) {
    return instant.offsetNanoseconds < 0 ? beforeEpochZone : afterEpochZone.getDateTimeFor(instant, calendar);
  }
  getPossibleInstantsFor(dateTime) {
    return (dateTime.year < 1970 ? beforeEpochZone : afterEpochZone).getPossibleInstantsFor(dateTime);
  }
  toString() {
    return 'π';
  }
}

If folks are OK with the content above, then I'd recommend we change the TS type of ZonedDateTime.prototype.timeZone to Temporal.TimeZone, reflecting what we know will be common practice among non-TS developers which is to assume that the timeZone property will act like built-in TimeZone objects.

[ptomato]

To me this text is too long given the tiny, advanced-user edge case that it documents, but I think it's a good start and I'd be happy to edit it down a bit. I think you are right that everyone is agreed on this part. I've opened #1040 for documenting this.

I'd recommend we change the TS type of ZonedDateTime.prototype.timeZone to Temporal.TimeZone, reflecting what we know will be common practice among non-TS developers which is to assume that the timeZone property will act like built-in TimeZone objects.

This is what you and @pipobscure disagree on. Personally I would prefer that we don't change the TypeScript type, so if you write TypeScript code you can make your own choice whether you want to treat it as a potentially minimally-implemented TimeZoneProtocol (you leave the type as is and TypeScript will prevent you from calling one of the potentially missing methods); or you explicitly choose to ignore that possibility by saying zdt.timeZone as Temporal.TimeZone;. But as I said in the meeting, I don't have enough TypeScript experience to say one of those is objectively better than the other. I've opened #1041 for you and Philipp to hash that out, but as it's not a blocker for submitting the proposal to TC39 I haven't put it in the milestone.

Having opened those two issues, I believe we can close this one?

[justingrant]

Yep let's close this issue. Thanks! Also, the sample implementation of a custom time zone in my original post had some bugs. I fixed it above. Would you be OK to edit #1040 as well to keep the code correct in both places?

[ptomato]

Will do, thanks.