Compile with `strictNullChecks`/`strictPropertyInitialization` (focus on calendar.ts and ecmascript.ts) #109

justingrant · 2021-12-13T11:18:28Z

This PR focuses on unblocking strictNullChecks and strictPropertyInitialization in ecmascript.ts and calendar.ts. It's intended as a complement to @jens-ox's work in #105 to remove GetIntrinsic.

This PR:

Enables strictNullChecks: true and strictPropertyInitialization: true in tsconfig.json.
Improves calendar.ts TS types (this was most of the work in this PR)
Removes all unnecessary use of any throughout the polyfill
Updates types of ecmascript.ts and fixes a number of type bugs
Refactors a few types in intl.ts
A handful of trivial type changes in other files (ecmascript.ts and calendar.ts were 95%+ of the work).

There is a one-line runtime change (by itself in commit fac8f4a) but otherwise there should be no runtime-visible changes in this PR, only TS type changes. This means that, aside from that one line in calendar.ts, the transpiled output should be the same before and after this PR, except for parentheses used for type casts.

Other runtime changes were split out into other PRs, all of which have already been merged except #112 which will hopefully land soon. This PR is rebased on #112.

ptomato

I didn't go through calendar.mjs and ecmascript.mjs too carefully, as this is a draft, but let me know what kind of reviews would be helpful! FWIW, it would be easier to review if you could split it into a series of commits that each fix one type of bug (although I realize that may not be possible; when you fix the types on one thing, maybe it "cascades" into another thing?)

lib/calendar.ts

ptomato · 2021-12-13T22:45:26Z

lib/calendar.ts

@@ -1650,9 +1726,10 @@ function adjustEras(erasParam: InputEra[]): { eras: Era[]; anchorEra: Era } {
  // Ensure that the latest epoch is first in the array. This lets us try to
  // match eras in index order, with the last era getting the remaining older
  // years. Any reverse-signed era must be at the end.
-  eras.sort((e1, e2) => {
+  ArraySort.call(eras, (e1, e2) => {


Not something that I think should be addressed in this PR, but just a note for later. For fixing the kind of defensive bugs as described #1911 this isn't enough — overwriting Function.prototype.call would still mess this up. We'd need to do something like in Jordan's https://github.com/ljharb/call-bind#readme utility.

Yeah I noticed that too, but use of call was so pervasive I figured we'd save that one until later. Also I was tired. :-)

lib/ecmascript.ts

justingrant

I didn't go through calendar.mjs and ecmascript.mjs too carefully, as this is a draft, but let me know what kind of reviews would be helpful!

@ptomato Lol, I actually didn't expect any reviews at all until this was further along, so your initial notes are more than I expected! I'll ping when it's ready for a more thorough review.

The best assistance at this point would be to help review and land the proposal-temporal PRs listed in the OP of this PR, because those all came from issues I discovered here while making these TS changes. The sooner those PRs are merged, the sooner this PR can get wrapped up.

FWIW, it would be easier to review if you could split it into a series of commits that each fix one type of bug (although I realize that may not be possible; when you fix the types on one thing, maybe it "cascades" into another thing?)

My current plan is to merge all the related proposal-temporal PRs, then port those PRs here, and then rebase this PR on top which should reduce its scope a lot. Furthermore, be removing those PRs it should limit this PR to almost entirely type changes: either none or only a few runtime-visible changes and zero changes that affect tests. It should be both easier and safer to review a big PR with only type changes vs. a mix of type a runtime changes. At least that's the goal. 😄

But you're right-- it's hard to make these kinds of type changes piecemeal, because once you pull on a thread of types it's hard to split things out. Luckily, once the runtime changes are removed it should be a lot clearer what's different.

Sound like an OK plan? Holler with any suggestions to make it easier.

lib/calendar.ts

justingrant · 2021-12-13T23:32:54Z

lib/calendar.ts

@@ -1650,9 +1726,10 @@ function adjustEras(erasParam: InputEra[]): { eras: Era[]; anchorEra: Era } {
  // Ensure that the latest epoch is first in the array. This lets us try to
  // match eras in index order, with the last era getting the remaining older
  // years. Any reverse-signed era must be at the end.
-  eras.sort((e1, e2) => {
+  ArraySort.call(eras, (e1, e2) => {


Yeah I noticed that too, but use of call was so pervasive I figured we'd save that one until later. Also I was tired. :-)

lib/ecmascript.ts

justingrant · 2021-12-16T07:00:58Z

I split this PR up into a bunch of commits to make reviewing easier, esp. for calendar.ts.

It's currently rebased on top of #112 which will hopefully land soon.

justingrant · 2021-12-16T07:01:44Z

Oh, and it's ready for review now. Reviews may not happen until after the new year.

A later commit is going to type all inLeapYear methods as only needing the year in the input, not a full date. This inLeapYear implementation is the only one where this type limitation needed a runtime change, so it's split into its own commit.

Declare an improved set of types for non-ISO calendars. They'll get used in later commits.

This commit is a search-n-replace to use the new calendar types defined in the previous commit: * nonIsoGeneralImpl => nonIsoImpl (the variable name) * NonIsoGeneralImpl => NonIsoImpl (the type) * typeof nonIsoGeneralImpl & { helper: NonIsoHelperBase } => NonIsoImplWithHelper * nonIsoHelperBase => helperSharedImpl (variable) * NonIsoHelperBase => HelperSharedImpl (type). The old NonIsoHelperBase interface is still present (under the new name) and the code uses the superset of both old and new same-named interfaces. The old one will be removed in the next commit once code using it is migrated to the new "split" interface with HelperSharedImpl and HelperPerCalendarImpl. But at least in the next commit there won't be lots of rename-only diffs. * Temporal.AssignmentOptions['overflow'] => Overflow * Temporal.ArithmeticOptions['overflow'] => Overflow (same values as AssignmentOptions enumeration) There's a lot more one-off changes coming up, but by breaking out these generic renames it will hopefully make everything else easier to review.

With the easy renames out of the way in the last commit, this commit does the hard work of using the new calendar types: * Strongly type all method parameters and remove all use of `any` * Remove now-unnecessary type casts because parameter types are now more accurate. * Ensure all code compiles with strictNullChecks:true and strictPropertyInitialization: true * Remove obsolete types

This commit makes ecmascript.ts changes for TS: * Support `strictNullChecks: true`. This means that any type that could possbly be undefined or null needs to be declared as such. This leads to a lot of types needing to be redefied to add or to exclude `undefined`. * Support `strictPropertyInitialization: true`. This means that all variables must be explicitly declared before they're used. * Remove use of `any` and replace with more precise types. Unlike calendar.ts, there were no bulk renames in this file. Just lots of one-off changes.

* Refactor type names for clarity * Add Temporal.Instant to list of accepted types

Previously, GetIntrinsic returned a type that could be `undefined`. That's fixed in this commit. Note that intrinsics.ts may be removed completely in js-temporal#105; if that PR lands first then we'll remove this commit.

This commit makes a handful of type changes in plaintime.ts, timezone.ts, and zoneddatetime.ts to handle parameters or variables that may be undefined or null. There's only a few changes.

Finally, enable strictNullChecks and strictPropertyInitialization in tsonfig.json.

ptomato

Great stuff, I think you'll find that my comments reflect my ignorance of TypeScript idioms rather than problems with the code. Thanks, by the way, for the separate commits and the explanations in the commit messages, that really made reviewing this a breeze.

ptomato · 2022-01-07T01:09:37Z

lib/calendar.ts

@@ -1691,8 +1691,11 @@ const makeHelperGregorian = (id: BuiltinCalendarId, originalEras: InputEra[]) =>
    eras,
    anchorEra,
    calendarType: 'solar',
-    inLeapYear(calendarDate /*, cache */) {
-      const { year } = this.estimateIsoDate(calendarDate);
+    inLeapYear(calendarDate /*, cache: OneObjectCache */) {


Do any of the overriding calendars use the month and day to determine inLeapYear? If not, maybe we could just pass year: number to this method instead of what is effectively calendarDate: { year: number }?

lib/calendar.ts

lib/ecmascript.ts

lib/intl.ts

lib/intrinsicclass.ts

lib/plaintime.ts

justingrant

Thanks so much @ptomato for the thorough review! There were two things you found that should result in code changes, but both of them are wider in scope than this PR, so I split them into separate issues.

For your other comments, I don't think changes are needed, but if you disagree please let me know! Otherwise, I think the PR code is still good to merge unless I hear differently from @12wrigja. I'll wait a few days for James to catch up before merging.

lib/plaintime.ts

lib/intrinsicclass.ts

lib/calendar.ts

justingrant · 2022-01-08T04:39:44Z

lib/calendar.ts

@@ -1613,39 +1536,42 @@ const helperIslamic: HelperSharedImpl = ObjectAssign({}, helperSharedImpl, {
  DAYS_PER_ISLAMIC_YEAR: 354 + 11 / 30,
  DAYS_PER_ISO_YEAR: 365.2425,
  constantEra: 'ah',
-  estimateIsoDate(this: typeof helperIslamic & HelperSharedImpl, calendarDate) {
+  estimateIsoDate(
+    this: HelperPerCalendarImpl & { DAYS_PER_ISLAMIC_YEAR: number; DAYS_PER_ISO_YEAR: number },


It's going away in ES6, so no need to alias here.

lib/intl.ts

lib/ecmascript.ts

justingrant · 2022-01-08T06:23:52Z

lib/ecmascript.ts

@@ -2160,7 +2164,7 @@ export function ToTemporalTimeZone(temporalTimeZoneLikeParam: TimeZoneParams['fr
  if (IsObject(temporalTimeZoneLike)) {
    if (IsTemporalZonedDateTime(temporalTimeZoneLike)) return GetSlot(temporalTimeZoneLike, TIME_ZONE);
    if (!('timeZone' in temporalTimeZoneLike)) return temporalTimeZoneLike;
-    temporalTimeZoneLike = (temporalTimeZoneLike as { timeZone: typeof temporalTimeZoneLike }).timeZone;
+    temporalTimeZoneLike = (temporalTimeZoneLike as unknown as { timeZone: typeof temporalTimeZoneLike }).timeZone;


Good catch, you found a bug in the public types of TimeZone.from! The docs say that { timeZone: Temporal.TimeZoneProtocol } should also be accepted, and so does the spec. But the public types disagree. Once the public types change, we can remove the as unknown here.

This issue is wider in scope than this PR and affects proposal-temporal too, so I opened #118 and will remove this cast as part the fix for that issue.

lib/calendar.ts

lib/ecmascript.ts

lib/intrinsicclass.ts

lib/timezone.ts

justingrant

I just pushed changes to resolve open issues in code review comments, so unless there are additional issues I'll plan to merge this and then finalize #113 to convert calendar.ts to ES6.

lib/timezone.ts

lib/plaintime.ts

lib/calendar.ts

justingrant mentioned this pull request Dec 13, 2021

Polyfill: fix a few non-ISO calendar issues tc39/proposal-temporal#1977

Merged

justingrant force-pushed the ts-undefined-types branch from ea2259e to f7c07fd Compare December 13, 2021 16:54

ptomato reviewed Dec 13, 2021

View reviewed changes

justingrant commented Dec 14, 2021

View reviewed changes

justingrant mentioned this pull request Dec 15, 2021

Catch up on proposal-temporal PRs #111

Merged

justingrant force-pushed the ts-undefined-types branch from 8975e67 to ad748e0 Compare December 16, 2021 00:23

justingrant mentioned this pull request Dec 16, 2021

Port non-ISO calendar fixes from proposal-temporal PRs #1977 and #1983 #112

Merged

justingrant force-pushed the ts-undefined-types branch 2 times, most recently from 4e7373d to c8f02f0 Compare December 16, 2021 06:56

justingrant marked this pull request as ready for review December 16, 2021 07:01

justingrant requested review from 12wrigja and ptomato December 16, 2021 07:01

justingrant changed the title ~~WIP: strictNullChecks/strictPropertyInitialization for calendar.ts and ecmascript.ts~~ Compile with strictNullChecks/strictPropertyInitialization (focus on calendar.ts and ecmascript.ts) Dec 16, 2021

justingrant added 9 commits December 16, 2021 17:01

Add new calendar types

7b1c123

Declare an improved set of types for non-ISO calendars. They'll get used in later commits.

Tighten types of intl.ts

196295e

* Refactor type names for clarity * Add Temporal.Instant to list of accepted types

Support strictNullChecks in intrinsics.ts

bfa0110

Previously, GetIntrinsic returned a type that could be `undefined`. That's fixed in this commit. Note that intrinsics.ts may be removed completely in js-temporal#105; if that PR lands first then we'll remove this commit.

strtictNullChecks in PT, TZ, and ZDT code

2885500

This commit makes a handful of type changes in plaintime.ts, timezone.ts, and zoneddatetime.ts to handle parameters or variables that may be undefined or null. There's only a few changes.

tsconfig changes

d5a6c76

Finally, enable strictNullChecks and strictPropertyInitialization in tsonfig.json.

justingrant force-pushed the ts-undefined-types branch from c8f02f0 to d5a6c76 Compare December 17, 2021 01:01

This was referenced Dec 17, 2021

Refactor non-ISO calendars to use ES6 classes #113

Merged

GetIntrinsic: remove from PlainDate, add monkeypatching test #105

Open

ptomato approved these changes Jan 7, 2022

View reviewed changes

justingrant commented Jan 8, 2022

View reviewed changes

12wrigja approved these changes Jan 10, 2022

View reviewed changes

justingrant commented Jan 11, 2022

View reviewed changes

lib/timezone.ts Show resolved Hide resolved

lib/plaintime.ts Show resolved Hide resolved

lib/calendar.ts Show resolved Hide resolved

lib/calendar.ts Show resolved Hide resolved

Changes from code reviews

245b568

justingrant force-pushed the ts-undefined-types branch from 43db4be to 245b568 Compare January 11, 2022 07:53

justingrant merged commit 0de68cd into js-temporal:main Jan 11, 2022

justingrant mentioned this pull request Jan 13, 2022

Enable strictNullChecks and strictPropertyInitialization in TS compilation #98

Closed

justingrant linked an issue Jan 13, 2022 that may be closed by this pull request

Enable strictNullChecks and strictPropertyInitialization in TS compilation #98

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compile with `strictNullChecks`/`strictPropertyInitialization` (focus on calendar.ts and ecmascript.ts) #109

Compile with `strictNullChecks`/`strictPropertyInitialization` (focus on calendar.ts and ecmascript.ts) #109

justingrant commented Dec 13, 2021 •

edited

Loading

ptomato left a comment

ptomato Dec 13, 2021

justingrant Dec 13, 2021

justingrant left a comment

justingrant Dec 13, 2021

justingrant commented Dec 16, 2021

justingrant commented Dec 16, 2021

ptomato left a comment

ptomato Jan 7, 2022

justingrant left a comment

justingrant Jan 8, 2022

justingrant Jan 8, 2022

justingrant left a comment

Compile with strictNullChecks/strictPropertyInitialization (focus on calendar.ts and ecmascript.ts) #109

Compile with strictNullChecks/strictPropertyInitialization (focus on calendar.ts and ecmascript.ts) #109

Conversation

justingrant commented Dec 13, 2021 • edited Loading

ptomato left a comment

Choose a reason for hiding this comment

ptomato Dec 13, 2021

Choose a reason for hiding this comment

justingrant Dec 13, 2021

Choose a reason for hiding this comment

justingrant left a comment

Choose a reason for hiding this comment

justingrant Dec 13, 2021

Choose a reason for hiding this comment

justingrant commented Dec 16, 2021

justingrant commented Dec 16, 2021

ptomato left a comment

Choose a reason for hiding this comment

ptomato Jan 7, 2022

Choose a reason for hiding this comment

justingrant left a comment

Choose a reason for hiding this comment

justingrant Jan 8, 2022

Choose a reason for hiding this comment

justingrant Jan 8, 2022

Choose a reason for hiding this comment

justingrant left a comment

Choose a reason for hiding this comment

Compile with `strictNullChecks`/`strictPropertyInitialization` (focus on calendar.ts and ecmascript.ts) #109

Compile with `strictNullChecks`/`strictPropertyInitialization` (focus on calendar.ts and ecmascript.ts) #109

justingrant commented Dec 13, 2021 •

edited

Loading