Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix minor calendar issues with TS types #676

Merged
merged 3 commits into from
Jun 15, 2020

Conversation

justingrant
Copy link
Collaborator

@justingrant justingrant commented Jun 13, 2020

This PR updates TS types to fix a few minor issues with the initial calendar types:

  • change type of era from unknown to string | undefined
  • consistent ordering of CalendarProtocol | string
  • change Calendar.from to accept CalendarProtocol | string
  • add missing calendar param to TimeZoneProtocol.getDateTimeFor

UPDATE:

  • adds TS strict mode to the lint step
  • fixes calendar-related types that were broken in strict mode
  • adds DurationFields and TimeFields types for consistency with *Fields types that include a calendar.
  • Convert protocols from TS types to TS interfaces
  • Export protocols (for re-use) but don't export *Fields types

- change type of `era` from `unknown` to `string | undefined`
- consistent ordering of `CalendarProtocol | string`
- change `Calendar.from` to accept `CalendarProtocol | string`
- add missing calendar param to `TimeZoneProtocol.getDateTimeFor`
@codecov
Copy link

codecov bot commented Jun 13, 2020

Codecov Report

Merging #676 into main will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #676   +/-   ##
=======================================
  Coverage   94.43%   94.43%           
=======================================
  Files          17       17           
  Lines        4760     4760           
  Branches      749      749           
=======================================
  Hits         4495     4495           
  Misses        262      262           
  Partials        3        3           
Flag Coverage Δ
#test262 52.82% <ø> (ø)
#tests 89.35% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c4b7b3e...6c0ed91. Read the comment docs.

Copy link
Collaborator

@ptomato ptomato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this makes sense!

@@ -215,7 +215,7 @@ export namespace Temporal {
year(date: Temporal.Date): number;
month(date: Temporal.Date): number;
day(date: Temporal.Date): number;
era(date: Temporal.Date): unknown;
era(date: Temporal.Date): string | undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had left era as unknown because of #526 — there is a problem with using strings for eras that I'm not sure is surmountable; and I hadn't originally intended to check in the Japanese calendar. That said, this change is fine with me because the TS types should reflect the current polyfill, but I just wanted to make sure you were aware of #526.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I saw that & figured the design was a work in progress. I also noticed that {era: undefined} in the current polyfill is returned in every getFields() call, even for non-ISO calendars' Date/DateTime/etc instances. Is this the intended long-term solution, or is the goal to only add custom calendar fields to instances that have those calendars?

- tighten TS checking in the lint step by using `--strict`.
- fix a few type issues that were exposed by strict mode.
- add `DurationFields` and`TimeFields` types for consistency.
@justingrant
Copy link
Collaborator Author

Found a few more TS errors in TS's strict mode, so I just pushed another commit that adds strict mode to the lint step and fixes the errors.

- Convert protocols from TS types to TS interfaces
- Export protocols (for re-use) but don't export `*Fields` types
@justingrant
Copy link
Collaborator Author

Also, realized that if people want to build custom time zones or calendars, they'll need those protocols to be exported. And the *Fields types are better if not exported because they won't pollute autocomplete. Hence one more commit. Finally, I converted protocols from types to interfaces which I think is closer to their intent.

Copy link
Collaborator

@ptomato ptomato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@ptomato ptomato merged commit c69c89e into tc39:main Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants