Add types to Slots. #71

12wrigja · 2021-10-02T01:55:46Z

No description provided.

justingrant

Cool! I built something similar as an experiment, but I like yours better. Simpler.

I suggested a few changes, otherwise good to merge once those are fixed.

For the runtime parts of this PR (new variables, etc.), is it worth porting those changes back to proposal-temporal to reduce the difference between the codebases to reduce the complexity of future porting in the other direction?

justingrant · 2021-10-02T04:05:18Z

lib/slots.ts

+  [CALENDAR]: Temporal.Calendar;
+
+  // Date, YearMonth, MonthDay common slots
+  [DATE_BRAND]: unknown;


These should be boolean or true. Not sure which would be best, but probably true.

IIRC the date/YM/MD brand check is currently implemented using only HasSlot — it doesn't matter what's in the slot, only that it is present.

justingrant · 2021-10-02T04:06:04Z

lib/slots.ts

+  [ISO_MILLISECOND]: number;
+  [ISO_MICROSECOND]: number;
+  [ISO_NANOSECOND]: number;
+  [CALENDAR]: Temporal.Calendar;


I actually think this is CalendarProtocol, because non-built-in calendars can also be stored.

This doesn't work, as it triggers ~50 errors about assignability:

Argument of type 'CalendarProtocol' is not assignable to parameter of type 'Calendar'.ts(2345)

I've started to see this show up elsewhere when I turn on more of the compiler options. It makes sense - Calendar is intentionally different from CalendarProtocol from the perspective of users, and it seems everywhere we accept a CalendarProtocol we do call ToTemporalCalendar which should convert a CalendarProtocol to a Calendar. At least in my branch, that method isn't typed and in most of the places we do this conversion it looks a bit something like

constructor(..., calendar: Temporal.CalendarProtocol | string = ES.GetISO8601Calendar()) { ... calendar = ES.ToTemporalCalendar(calendar); // Note: if the arguments are not passed, // ToIntegerThrowOnInfinity(undefined) will have returned 0, which will // be rejected by RejectISODate in CreateTemporalDateSlots. This check // exists only to improve the error message. if (arguments.length < 3) { throw new RangeError('missing argument: isoYear, isoMonth and isoDay are required'); } ES.CreateTemporalDateSlots(this, isoYear, isoMonth, isoDay, calendar); }

which doesn't compile as CreateTemporalDateSlots shouldn't accept CalendarProtocol, only Calendar. If this is rewritten to no longer reassign the parameter, and ToTemporalCalendar is typed correctly, then I think I can say with certainty that all user calendars (and likely, all user TimeZones) are converted from the Protocol version to the internal version.

I was wondering if whether some sort of similar difference in types would be useful to enforce that these coercion methods are actually called and the results used.

Yeah, this is usually the param reassignment problem. Once we fix that, then I think most of these errors should go away-- any others can probably be addressed with type assertions or (worst case) additional variable renaming.

Too bad that TS doesn't narrow types from reassignment. Here's me making the case for why it should: microsoft/TypeScript#45870 (comment)

ToTemporalCalendar can't narrow the type, because of plain object calendars. At first glance I think it is actually correct that CreateTemporalDateSlots should accept CalendarProtocol, for that reason.

justingrant · 2021-10-02T04:06:45Z

lib/slots.ts

@@ -39,8 +42,52 @@ export const NANOSECONDS = 'slot-nanoseconds';
 // Calendar
 export const CALENDAR_ID = 'slot-calendar-identifier';

+interface Slots {
+  // Instant
+  [EPOCHNANOSECONDS]: bigInt.BigInteger; // number? JSBI?


Definitely not number. Seems like bigInt.BigInteger is the right type, and maybe change that once JSBI is in.

justingrant · 2021-10-02T05:01:48Z

lib/slots.ts

+
+  // ZonedDateTime
+  [INSTANT]: Temporal.Instant;
+  [TIME_ZONE]: Temporal.TimeZone;


I think this is TimeZoneProtocol, for same reason as CalendarProtocol above

justingrant

I tried this PR in my branch and here's a few more suggestions.

justingrant · 2021-10-03T20:05:17Z

lib/slots.ts

@@ -1,3 +1,6 @@
+import { Temporal } from '.';


I suspect that consistently only importing types (not implementations) works better. Not 100% sure, but I think I've seen cases where mixing the two causes weird loops in type resolution that the TS compiler complained about.

Suggested change

import { Temporal } from '.';

import type { Temporal } from '..';

I've been running into this issue all the time lately, where vscode suggests one or the other import and the two aren't assignable to each other. It would be great to try and 'exclude' temporal.ts (the actual module file) from being an import suggestion so that only the typings would show up instead.

Yeah, if you find out how to do that let me know! Very annoying.

justingrant · 2021-10-03T20:12:41Z

lib/slots.ts

@@ -51,9 +98,9 @@ export function HasSlot(container, ...ids) {
  const myslots = GetSlots(container);


Should probably type HasSlot while we're at it:

export function HasSlot(container: unknown, ...ids: (keyof Slots)[]) {

justingrant · 2021-10-03T20:15:08Z

lib/slots.ts

 const slots = new WeakMap();
-export function CreateSlots(container) {
+export function CreateSlots(container): void {
  slots.set(container, Object.create(null));
 }
 function GetSlots(container) {


Suggested change

function GetSlots(container) {

function GetSlots(container: AnyTemporalType) {

justingrant · 2021-10-03T20:15:45Z

lib/slots.ts

@@ -51,9 +98,9 @@ export function HasSlot(container, ...ids) {
  const myslots = GetSlots(container);
  return !!myslots && ids.reduce((all, id) => all && id in myslots, true);
 }
-export function GetSlot(container, id) {
+export function GetSlot<KeyT extends keyof Slots>(container, id: KeyT): Slots[KeyT] {


Suggested change

export function GetSlot<KeyT extends keyof Slots>(container, id: KeyT): Slots[KeyT] {

export function GetSlot<KeyT extends keyof Slots>(container: AnyTemporalType, id: KeyT): Slots[KeyT] {

justingrant · 2021-10-03T20:16:17Z

lib/slots.ts

  return GetSlots(container)[id];
 }
-export function SetSlot(container, id, value) {
+export function SetSlot<KeyT extends keyof Slots>(container: unknown, id: KeyT, value: Slots[KeyT]): void {


Suggested change

export function SetSlot<KeyT extends keyof Slots>(container: unknown, id: KeyT, value: Slots[KeyT]): void {

export function SetSlot<T extends AnyTemporalType, KeyT extends keyof Slots>(

container: T,

id: KeyT,

value: Slots[KeyT]

): void {

ptomato

Justin has done a much better review of the TypeScript nitty gritty than I could, but I wanted to add an alternative that I'd been thinking about for a while. I believe the 'slots' mechanism was used in order to mimic the specification text which talks about the implementation's private values in terms of "internal slots". In the production polyfill we're not necessarily required to stick with this mechanism.

An alternative might be to make private "implementation" classes, something like this:

class PlainDateImpl {
  #isoYear : number;
  #isoMonth : number;
  #isoDay : number;
  #calendar : CalendarProtocol;
  constructor(isoYear, isoMonth, isoDay, calendar) {
    this.#isoYear = isoYear;
    // etc
  }
  get isoYear() { return this.#isoYear; }
  get isoMonth() { ... }
  // etc
}

export class PlainDate {
  #impl: PlainDateImpl;
  constructor(isoYear, isoMonth, isoDay, calendar = undefined) {
    this.#impl = new PlainDateImpl(isoYear, isoMonth, isoDay, calendar);
  }
  get year() {
    return this.#impl.calendar.year(this);
  }
  // etc. - rest of public API goes here
}

(the private class would be necessary because other code has to access the private fields; but it would also allow optimizations such as changing functions such as DifferenceISODate which take loads of arguments, to actually be methods of PlainDateImpl instead.)

Anyway, a refactor like that would be much larger in scope than this change, but might be a good direction for changes like this to try to evolve into. I was wondering what the rest of you thought about it.

justingrant · 2021-10-04T19:24:28Z

I think using private fields would be good at some point, but I'd worry that they're new enough that some downstream users may have trouble, even if they're polyfilled. Seems like it may make sense to wait a bit and refactor later. But I agree in general that this seems like a good direction to take.

BTW, @12wrigja and I were chatting and we may end up merging a fork of this PR instead that does some other nifty things like making HasSlot into a type guard function so that if (HasSlot(foo, TIME_ZONE)) { } will narrow foo to a ZonedDateTime inside the if block. I converted this PR to draft while we figure that out.

12wrigja · 2021-10-11T23:43:52Z

Closing this PR as @justingrant merged in a better version of these changes in #74 .

Add types to Slots.

0f5dcbc

12wrigja requested a review from justingrant October 2, 2021 01:55

justingrant approved these changes Oct 2, 2021

View reviewed changes

justingrant reviewed Oct 2, 2021

View reviewed changes

justingrant approved these changes Oct 3, 2021

View reviewed changes

ptomato approved these changes Oct 4, 2021

View reviewed changes

justingrant marked this pull request as draft October 4, 2021 19:22

12wrigja closed this Oct 11, 2021

	import { Temporal } from '.';
	import type { Temporal } from '..';

		@@ -51,9 +98,9 @@ export function HasSlot(container, ...ids) {
		const myslots = GetSlots(container);

	function GetSlots(container) {
	function GetSlots(container: AnyTemporalType) {

	export function GetSlot<KeyT extends keyof Slots>(container, id: KeyT): Slots[KeyT] {
	export function GetSlot<KeyT extends keyof Slots>(container: AnyTemporalType, id: KeyT): Slots[KeyT] {

Add types to Slots. #71

Add types to Slots. #71

Uh oh!

Conversation

12wrigja commented Oct 2, 2021

Uh oh!

justingrant left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justingrant Oct 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justingrant left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ptomato left a comment

Choose a reason for hiding this comment

Uh oh!

justingrant commented Oct 4, 2021

Uh oh!

12wrigja commented Oct 11, 2021

Uh oh!

Uh oh!

justingrant Oct 2, 2021 •

edited

Loading