feat(isUtcLeap): Add function to determine a leap year in UTC to avoi…

[RyanClementsHax]

…d timezone issues

• Add isUtcLeap that is the same as isLeap but uses getUTCFullYear to avoid timezone issues
• Add tests for isUtcLeap
• Fix tests for isLeap that only failed on machines running in GMT-

Implements #3294

[CLAassistant]

All committers have signed the CLA.

[lionel-rowe]

TBH the whole idea of determining whether something's a leap year from a Date object seems massively error-prone and difficult to reason about. For example, a developer might reasonably believe isUTCLeap will interpret a Date object as if it's UTC even when it was constructed in a non-UTC timezone, which isn't true.

// in my current timezone, UTC+8
isLeap(new Date(2000, 0 /* zero-indexed month */, 1)) // true (consistently true everywhere)
isUTCLeap(new Date(2000, 0 /* zero-indexed month */, 1))) // false (but would be true if I lived somewhere with a zero or negative UTC offset)
isLeap(new Date("2000-01-01")) // true (but would be false if I lived somewhere with a negative UTC offset)
isUTCLeap(new Date("2000-01-01")) // true (consistently true everywhere)

IMO it'd be better to simply deprecate or remove the (year: Date) => boolean signature.

[RyanClementsHax]

I'm ok with either. I can update the PR if needed. How will I know if this is the way we want to go? Does a maintainer need to give the final say?

[lionel-rowe]

I'm ok with either. I can update the PR if needed. How will I know if this is the way we want to go? Does a maintainer need to give the final say?

I'd assume so — I'm just a random contributor so I can't speak authoritatively, just adding my 2 cents as I've come across far too many Date-related timezone bugs. Incidentally, this type of confusion is a big part of the motivation for Temporal, though that's still at stage 3 and AFAIK not implemented in any browsers yet, even behind a flag.

[iuioiua]

How come this functionality isn't being added to isLeap() instead of having a separate function? Having 2 separate functions that have so much overlap may confuse users.

If a separate function is justified, it should be named isUtcLeap() as acronyms should written in camelCase (see #2582).

Disclaimer: I have not looked at this implementation or discussion, so please excuse any ignorance.

[RyanClementsHax]

I could add an option object to is leap to toggle this feature but it seemed more idiomatic to make a second function to keep in line with the api of JavaScript's date object. This is also why I named the function the way I did. For more details see #3294.

I'm not opposed to changing these details btw. I don’t feel very strongly about them.

[iuioiua]

I see, I see. In that case, a separate function is reasonable.

Again though, the function name would have to be isUtcLeap() with the acronym being in camelCase 👍🏾

[RyanClementsHax]

I've changed the function to be named isUtcLeap

[kt3k]

LGTM