Add types to ecmascript.ts #74

justingrant · 2021-10-04T23:36:51Z

Make ecmascript.ts type-safe:

Improves types for slots and intrinsics
ecmascript.ts now has types throughout and compiles with noImplicitAny
Fixes BigInt errors in a few places to fix compiler errors and test failures. Will be replaced by James's work on JSBI.
Found and fixed a few index.d.ts bugs in the public types.
Overhauled how plural units are handled in TS types.
Some initial work in calendar.ts, but I paused that while James does work on that file.
Otherwise, only a few few changes outside ecmascript.ts that were required to fix compiler errors.
A new internal types file for non-public shared types.

BTW, this is part of a larger roadmap. Here's the tentative plan:

Merge Fix some param reassignments, and enable lint. #72 (preventing overwriting parameters in all files except ecmascript.ts (to prevent merge conflicts with this file)
Merge this PR after rebasing on top of Fix some param reassignments, and enable lint. #72
Prevent rewriting parameters in ecmascript.ts, just like Fix some param reassignments, and enable lint. #72 does for all other files. I'll plan to do this in another PR because this PR is already big enough!
Add parameter type annotations on non-ecmascript files. I have a regex search-n-replace solution in mind for this.
@12wrigja's big JSBI PR Use JSBI instead of big-integer #77. Probably makes sense to merge this one after the all the PRs above so we can get maximum TS safety for such a large runtime change.

justingrant · 2021-10-07T20:52:34Z

Not sure why Test262 shows as failed given that the output doesn't report any failures. Trying to re-run.

justingrant · 2021-10-07T21:21:35Z

Hmmm, looks like Test262 tests aren't actually emitting output into the GH actions log. There was a local failure but I don't see its details in CI. I'll fix the failure but seems like something isn't quite right with our CI setup.

12wrigja

Mostly just questions for my own understanding.

lib/calendar.ts

lib/ecmascript.ts

lib/internaltypes.d.ts

lib/intrinsicclass.ts

lib/slots.ts

justingrant

Thanks for reviewing so many changes! I think I've either resolved all the issues you found, or want to defer them until later.

I'll commit the final changes over the weekend and if I don't hear complaints from @ptomato then I'll go ahead and merge because I have a follow-up PR ready to add parameter and return types to all other methods. This includes cleaning up the Parameters stuff you commented about. If there are remaining issues from this PR to deal with, we can resolve them in the next one since there's a lot of overlap.

BTW, adding types to other files found a few bugs in the public types. Was a useful exercise.

lib/ecmascript.ts

justingrant · 2021-10-09T04:57:35Z

lib/ecmascript.ts

+  fallback: Allowed | 'auto',
+  disallowedStrings: ReadonlyArray<Disallowed> = [],
+  autoValue?: IfAuto
+): IfAuto extends undefined ? Allowed | 'auto' : Allowed {


Yep, I didn't figure out a way in the compiler to validate that Allowed doesn't overlap with Disallowed. Also, agreed on the overloads, because once I typed all the usage of this function then I learned that IfAuto didn't actually work. I'll push a commit with some edits. PTAL.

lib/ecmascript.ts

lib/internaltypes.d.ts

lib/intrinsicclass.ts

lib/slots.ts

This commit continues where js-temporal#74 left off by adding parameter and return types to the rest of (non-ecmascript.ts) Temporal source files. Also, it turns on --noImplicitAny. Getting this working in calendar.ts will probably require more work than I want 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 defnitely need a follow up to improve calendar.ts. Other than that one file, this should be pretty solid. One thing I need to figure out is why tests won't work. I think it's caused by importing Intl fom `..` (to pick up the public types) instead of importing it from intl.ts directly. The fix is probably in resolve.source.mjs but I could use help to figure this out.

This commit continues where js-temporal#74 left off by adding parameter and return types to the rest of (non-ecmascript.ts) Temporal source files. Also, it turns on --noImplicitAny. The worst `any` offender is calendar.ts, and fully declaring all its types is more work than I want 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 defnitely need a follow up to improve calendar.ts. Other than that one file, this commit essentially removes `any` (implicit or otherwise) from the polyfill, with only a few exceptions for truly generic code or testing propeties on `globalThis`. There's still lots of TS work remaining (e.g. strictNullChecks, strictPropertyInitialization, and cleaning up calendar.ts), 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 commit fixes those too.

This commit continues where js-temporal#74 left off by adding parameter and return types to the rest of (non-ecmascript.ts) Temporal source files. Also, it turns on --noImplicitAny. The worst `any` offender is calendar.ts, and fully declaring all its types is more work than I want 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 defnitely need a followup PR to improve calendar.ts. Other than that one file, this commit essentially removes `any` (implicit or otherwise) from the polyfill, with only a few exceptions for truly generic code or testing propeties on `globalThis`. There's still lots of TS work remaining (e.g. `strictNullChecks`, `strictPropertyInitialization`, and cleaning up calendar.ts), 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 commit fixes those too.

Ms2ger

No objection from me

12wrigja · 2021-10-11T16:59:06Z

Seems like the GH UI is doing something weird - I can't seem to respond to any of the open conversations unless I click "into" them, and even then the comments don't seem to propagate back.

I have no objections to merging this as-is and following up on my comments later, especially given some of them might be fixed by compiler flag changes (e.g. strictNullChecks).

This commit continues where js-temporal#74 left off by adding parameter and return types to the rest of (non-ecmascript.ts) Temporal source files. Also, it turns on --noImplicitAny. The worst `any` offender is calendar.ts, and fully declaring all its types is more work than I want 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 defnitely need a followup PR to improve calendar.ts. Other than that one file, this commit essentially removes `any` (implicit or otherwise) from the polyfill, with only a few exceptions for truly generic code or testing propeties on `globalThis`. There's still lots of TS work remaining (e.g. `strictNullChecks`, `strictPropertyInitialization`, and cleaning up calendar.ts), 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 commit fixes those too.

* Improves types for slots and intrinsics * ecmascript.ts now has types throughout and compiles with `noImplicitAny` (although a later PR will actually enable this option) * Fixes BigInt errors in a few places to fix compiler errors and test failures. Will be replaced by James's work on JSBI. * Fixes a few index.d.ts bugs in the public types. * Overhauls how plural units are handled in TS types. * Partially adds types to calendar.ts. (More coming later.) * A new internal types file for non-public shared types.

This commit continues where js-temporal#74 left off by adding parameter and return types to the rest of (non-ecmascript.ts) Temporal source files. Also, it turns on --noImplicitAny. The worst `any` offender is calendar.ts, and fully declaring all its types is more work than I want 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 defnitely need a followup PR to improve calendar.ts. Other than that one file, this commit essentially removes `any` (implicit or otherwise) from the polyfill, with only a few exceptions for truly generic code or testing propeties on `globalThis`. There's still lots of TS work remaining (e.g. `strictNullChecks`, `strictPropertyInitialization`, and cleaning up calendar.ts), 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 commit fixes those too.

@12wrigja

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.

@12wrigja

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.

@12wrigja

Continue where #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 changed the title ~~DO NOT MERGE: Add types to ecmascript.ts~~ DO NOT MERGE: WIP: Add types to ecmascript.ts Oct 5, 2021

justingrant force-pushed the function-types branch 12 times, most recently from 5aaf364 to 73fb640 Compare October 7, 2021 06:59

justingrant changed the title ~~DO NOT MERGE: WIP: Add types to ecmascript.ts~~ DO NOT MERGE: Add types to ecmascript.ts Oct 7, 2021

justingrant force-pushed the function-types branch 3 times, most recently from eef0d7e to 98893f3 Compare October 7, 2021 07:45

justingrant mentioned this pull request Oct 7, 2021

Unable to run test262 test suite locally #76

Closed

justingrant force-pushed the function-types branch 4 times, most recently from 40a9ed1 to f8a4a57 Compare October 7, 2021 20:28

justingrant changed the title ~~DO NOT MERGE: Add types to ecmascript.ts~~ Add types to ecmascript.ts Oct 7, 2021

justingrant requested review from 12wrigja, ptomato and Ms2ger October 7, 2021 20:44

justingrant marked this pull request as ready for review October 7, 2021 20:51

12wrigja approved these changes Oct 8, 2021

View reviewed changes

justingrant commented Oct 9, 2021

View reviewed changes

justingrant force-pushed the function-types branch from f0ace7c to 3e8a755 Compare October 10, 2021 05:46

justingrant force-pushed the function-types branch from 3e8a755 to b58d996 Compare October 10, 2021 05:53

Ms2ger approved these changes Oct 11, 2021

View reviewed changes

justingrant force-pushed the function-types branch from b58d996 to 31f1b41 Compare October 11, 2021 18:50

justingrant force-pushed the function-types branch from 31f1b41 to 04b214f Compare October 11, 2021 19:16

justingrant merged commit ac78fd9 into js-temporal:main Oct 11, 2021

justingrant mentioned this pull request Oct 11, 2021

Add types to all files and enable noImplicitAny #81

Merged

12wrigja mentioned this pull request Oct 11, 2021

Add types to Slots. #71

Closed

Add types to ecmascript.ts #74

Add types to ecmascript.ts #74

Uh oh!

Conversation

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

Uh oh!

Uh oh!

justingrant commented Oct 7, 2021

Uh oh!

justingrant commented Oct 7, 2021

Uh oh!

12wrigja left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

justingrant left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

justingrant Oct 9, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ms2ger left a comment

Choose a reason for hiding this comment

Uh oh!

12wrigja commented Oct 11, 2021

Uh oh!

Uh oh!

justingrant commented Oct 4, 2021 •

edited

Loading