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 TS types for required CalendarProtocol methods #1964

Merged
merged 1 commit into from
Dec 9, 2021

Conversation

justingrant
Copy link
Collaborator

The docs say:

The other, more difficult, way to create a custom calendar is to create a plain object implementing the Temporal.Calendar protocol, without subclassing. The object must implement all of the Temporal.Calendar properties and methods except for id, fields(), mergeFields(), and toJSON().

The TS types currently allow many CalendarProtocol methods to be undefined other than the 4 above. This PR aligns the TS types to the docs.

@sffc @ptomato holler if the docs are wrong.

The docs state that all CalendarProtocol methods are required to be
implemented except `id`, `fields()`, `mergeFields()`, and `toJSON()`.
This PR aligns the TS types to the docs.
@codecov
Copy link

codecov bot commented Dec 8, 2021

Codecov Report

Merging #1964 (55db928) into main (f2405e1) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1964   +/-   ##
=======================================
  Coverage   95.00%   95.00%           
=======================================
  Files          19       19           
  Lines       10946    10946           
  Branches     1739     1739           
=======================================
  Hits        10399    10399           
  Misses        531      531           
  Partials       16       16           
Flag Coverage Δ
test262 80.04% <ø> (ø)
tests 89.83% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


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 f2405e1...55db928. 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.

LGTM, in order to have a fully functional plain-object calendar all these methods are required.

To be clear, you can leave them out and nothing will complain so long as you don't use any features that require them, which we do in some of our tests. But that doesn't seem like the use case that TypeScript bindings should address.

@ptomato ptomato added the no-spec-text PR can be ignored by implementors label Dec 9, 2021
@ptomato ptomato merged commit 10937da into tc39:main Dec 9, 2021
ptomato pushed a commit to js-temporal/temporal-polyfill that referenced this pull request Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-spec-text PR can be ignored by implementors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants