Add types to all files and enable noImplicitAny
#81
Conversation
| if (!(this instanceof DateTimeFormat)) return new DateTimeFormat(locale, optionsParam); | ||
| type DateTimeFormatImpl = Intl.DateTimeFormat & PrivateProps; | ||
|
|
||
| function DateTimeFormatImpl( |
There was a problem hiding this comment.
Here's the story behind renaming this function: I wanted to only export this function as an Intl.DateTimeFormat (from the public types) so that other modules wouldn't know about its implementation details. This is what we do, for example, with other Temporal types where callers can import { Temporal } from '..'. (BTW, this works in tests but we should validate that it will also work in real apps!)
But the module resolver used in Demitasse tests (resolve.source.mjs) was special-cased for importing Temporal only. import { Temporal, Intl } from '..' caused Demitasse tests to fail.
But I still wanted other-module callers to see this function as a plain Intl.DateTimeFormat (from public types) without seeing its private properties. I couldn't figure out how to do this when it was named DateTimeFormat without getting a TS error when attempting to change its type to the public type. So I renamed the function, which required a runtime code change to update the name property.
Feel free to suggest a better way to do it!
justingrant
force-pushed
the
all-files-types
branch
from
cc12ab3 to
47bfd55
Compare
| if (maybeStringCalendar === undefined) maybeStringCalendar = GetISO8601Calendar(); | ||
| calendar = ToTemporalCalendar(maybeStringCalendar); | ||
| let calendar: Temporal.CalendarProtocol | string = maybeStringCalendar; | ||
| if (calendar === undefined) calendar = GetISO8601Calendar(); |
There was a problem hiding this comment.
Could this be
let calendar: Temporal.CalendarProtocol;
if (maybeStringCalendar === undefined) calendar = GetISO8601Calendar();
calendar = ToTemporalCalendar(maybeStringCalendar);
to avoid having to rely on TS to narrow down the type to exclude |string ?
There was a problem hiding this comment.
Yeah, I wasn't sure how to handle this. calendar starts as a string and ends up as a CalendarProtocol. Sure would be nice if microsoft/TypeScript#27706 were fixed!
I think that your proposed code won't work when maybeStringCalendar is undefined. In that case, ToTemporalCalendar(maybeStringCalendar) will throw. The code in this PR was the "least lying to TS" that I could think of without changing runtime flow from proposal-temporal.
| @@ -113,10 +166,21 @@ export function DateTimeFormat( | |||
| this[DATETIME] = datetimeAmend; | |||
| this[ZONED] = zonedDateTimeAmend; | |||
| this[INST] = instantAmend; | |||
| return undefined; // TODO: I couldn't satisfy TS without adding this. Is there another way? | |||
There was a problem hiding this comment.
This is a bit worrying... is the intent that DateTimeFormatImpl be a private impl of the DateTimeFormat ctor?
There was a problem hiding this comment.
Yes. Typing this damn function was a huge hassle. See #81 (comment). LMK if you can think of a better solution.
| if (maybeStringCalendar === undefined) maybeStringCalendar = GetISO8601Calendar(); | ||
| calendar = ToTemporalCalendar(maybeStringCalendar); | ||
| let calendar: Temporal.CalendarProtocol | string = maybeStringCalendar; | ||
| if (calendar === undefined) calendar = GetISO8601Calendar(); |
There was a problem hiding this comment.
Yeah, I wasn't sure how to handle this. calendar starts as a string and ends up as a CalendarProtocol. Sure would be nice if microsoft/TypeScript#27706 were fixed!
I think that your proposed code won't work when maybeStringCalendar is undefined. In that case, ToTemporalCalendar(maybeStringCalendar) will throw. The code in this PR was the "least lying to TS" that I could think of without changing runtime flow from proposal-temporal.
|
I'm happy to defer to @12wrigja here. |
Continue where js-temporal#74 left off by adding parameter and return types to the rest of (non-ecmascript.ts) Temporal source files. Also, turn on `noImplicitAny`. Except for calendar.ts, this PR removes `any` (implicit or otherwise) from the polyfill, with only a few exceptions for truly generic code or testing propeties on `globalThis`. For calendar.ts, `any` is removed from the top level `Calendar` implementation and some of the `impl[]` code, but fully declaring all types in that file was more work than I wanted to take on in this PR. So I did an initial pass on obvious types and assigned a lot of `any` for the rest. Will definitely need a followup PR to improve calendar.ts. @12wrigja has discussed the possibility of replacing the old-school inheritance using Object.apply with a modern `class`-based implementation. If we're going to do that, then it may make sense to fix the types then instead of on top of the current solution. Aside from calendar.ts, there's still lots of TS work remaining (e.g. `strictNullChecks`, `strictPropertyInitialization`), but at least we'll now have valid types on all functions. I didn't find any runtime errors as a result of this work (good!) but I did find quite a few problems with the public types in index.d.ts, and this PR fixes those too. Once the dust settles and the changes are reviewed, I can port a change back to proposal-temporal to keep all public types in sync.
justingrant
force-pushed
the
all-files-types
branch
from
f8ecc5f to
dd9bd43
Compare
|
OK @12wrigja, ball is in your court for the next big PR! |
|
Thanks! I'll rebase today and send for review. Thank you so much for all this work - your expertise here is very much appreciated. |
This PR continues where #74 left off by adding parameter and return types to the rest of (non-ecmascript.ts) Temporal source files.
Also, it turns on
noImplicitAny. Except for calendar.ts, this PR removesany(implicit or otherwise) from the polyfill, with only a few exceptions for truly generic code or testing propeties onglobalThis.For calendar.ts,
anyis removed from the top levelCalendarimplementation and some of theimpl[]code, but fully declaring all types in that file was more work than I wanted to take on in this PR. So I did an initial pass on obvious types and assigned a lot ofanyfor the rest. Will definitely need a followup PR to improve calendar.ts. @12wrigja has discussed the possibility of replacing the old-school inheritance using Object.apply with a modernclass-based implementation. If we're going to do that, then it may make sense to fix the types then instead of on top of the current solution.Aside from calendar.ts, there's still lots of TS work remaining (e.g.
strictNullChecks,strictPropertyInitialization), but at least we'll now have valid types on all functions.I didn't find any runtime errors as a result of this work (good!) but I did find quite a few problems with the public types in index.d.ts, and this PR fixes those too. Once the dust settles and the changes are reviewed, I can port a change back to proposal-temporal to keep all public types in sync.