search

js-temporal / temporal-polyfill

Polyfill for Temporal (under construction)
ISC License
529 stars 28 forks source link

Editorial: Simplify operations (#2248) #169

Closed 12wrigja closed 2 years ago

12wrigja commented 2 years ago

Similar to other rebases, I intend for this to be merged as-is without squashing.

The typing for GetTemporalUnit is interesting - ideally, I'd like to find a way to avoid all the casts within the function itself and avoid needing overloads, but I can't seem to get TS to recognize that when the default empty array (typed as never[]) is used as the extraValues property that it expands to never in the return type (which is the union identity).

justingrant commented 2 years ago

Sorry I'm coming late to this party! I had it starred in my email app but had a busy few weeks and didn't get to it until now.

The typing of these Largest/Smallest unit methods was one of the 2 hardest TS challenges when I originally did the TS conversion. The combination of allowing and disallowing options was painful to make typed.

I have one question on the new implementation. If I remember correctly, some of the methods allowed auto but others didn't. Has that distinction been retained in the new TS types, so we'll catch the case of someone passing auto to a method that shouldn't allow it?

12wrigja commented 2 years ago

I had it starred in my email app

In the future, feel free to drop a line here saying something like "id like to review this before it's merged" and I'm happy to wait for your review.

some of the methods allowed

I think you are referring to ToSmallestTemporalUnit didn't accept auto, but ToLargestTemporalUnit did. I didn't notice that when I was merging this PR, and dropped that detail. There's another commit upstream that further modifies the new impl signature, so I'll incorporate this change into that PR.