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

More spec ports, and small updates to GetTemporalUnit typings. #183

Merged
merged 2 commits into from
Oct 10, 2022

Conversation

12wrigja
Copy link
Contributor

Like other spec port PR's, I will merge this as-is without squashing to retain history.

@12wrigja
Copy link
Contributor Author

FYI that I'll be on vacation till the 6th of September (2022), so I won't respond till then. There's no rush on this review.

I've asked some colleagues to take a look at some of the more complex type-system implementation I used for PrepareTemporalFields to see if there's any way to simplify it further whilst retaining the useful properties the current impl has:

  • preparing fields returns an object with all the given field names as required properties with the types from the "potential owners" of that field.
  • all the other properties of the potential owners that weren't specificed in the field name list are marked as optional - this allows them to be assigned to without casting / complain from tsc. This isn't perfect - it does mean in some cases casts are still required to change the properties type after assignment (e.g. ecmascript.ts L4574 is a good example).

Copy link

@shicks shicks left a comment

Choose a reason for hiding this comment

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

See also the final state of the playground I was working with while experimenting. I have a few links to earlier versions of it throughout the review (as well as a few other links to separate playgrounds, so there's that).

lib/ecmascript.ts Outdated Show resolved Hide resolved
lib/ecmascript.ts Outdated Show resolved Hide resolved
lib/ecmascript.ts Outdated Show resolved Hide resolved
lib/ecmascript.ts Outdated Show resolved Hide resolved
lib/ecmascript.ts Outdated Show resolved Hide resolved
lib/ecmascript.ts Outdated Show resolved Hide resolved
lib/ecmascript.ts Outdated Show resolved Hide resolved
lib/ecmascript.ts Outdated Show resolved Hide resolved
lib/ecmascript.ts Outdated Show resolved Hide resolved
lib/ecmascript.ts Outdated Show resolved Hide resolved
@12wrigja 12wrigja requested a review from shicks September 8, 2022 03:20
lib/ecmascript.ts Outdated Show resolved Hide resolved
lib/ecmascript.ts Outdated Show resolved Hide resolved
UPSTREAM_COMMIT=1e9ea70f694b2afc0f18a1faae64a8b3cf0604b6

Co-authored-by: James Wright <[email protected]>
@12wrigja
Copy link
Contributor Author

12wrigja commented Oct 4, 2022

@justingrant @ptomato I think Steve and I are done with back-and-forth on this, and I'd appreciate if you could take a look. Thanks!

Copy link
Contributor

@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 although I didn't deep-dive into all the changes to the typing of PrepareTemporalFields.

Separately, if it turns out that the types of PrepareTemporalFields are that convoluted, it may be worth deviating from the spec text there (in a way that's not observable from JS, of course) and having separate operations for PreparePlainDateFields, etc. if their types are easier to express. But that would be out of scope for this PR.

lib/ecmascript.ts Show resolved Hide resolved
@12wrigja 12wrigja merged commit 92569be into js-temporal:main Oct 10, 2022
@12wrigja 12wrigja deleted the spec_ports_4 branch October 10, 2022 18:03
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.

4 participants