feat: Added extensions for Temporal #15
Conversation
| const asyncResults = this.map((e, i, a) => | ||
| maybePromiseThen(predicate(e, i, a), (result) => result || Promise.reject()), | ||
| ); | ||
| return Promise.any(asyncResults).catch(() => false); |
There was a problem hiding this comment.
Was this .catch(() => false) the prior behavior? Seems pretty surprising to me for that a .some call would suppress exceptions.
There was a problem hiding this comment.
Yeah this was the prior implementation 🤷 I also was 🤨 at this, but didn't feel like changing it was within the scope here.
| } | ||
| } | ||
|
|
||
| Temporal.Interval = class<T extends Temporal.PlainDate | Temporal.ZonedDateTime> { |
There was a problem hiding this comment.
Would it be valid to ask for a Interval<DateTime>? That seems like a valid ask, initially/intuitively, except such an interval could not have an eachMonth method?
This makes me think that this should be like CalendarInterval to denote it's for calendar-aware types, and keep Interval available for future / eventual Interval<DateTime> use case?
There was a problem hiding this comment.
why couldn't it have an eachMonth method? I could see zoned date times eventually wanting something like .eachHour / .eachSecond / .eachMinute that PlainDate can't support, but we could restrict those with this: ... typeguards.
Do you mean PlainTime?
There was a problem hiding this comment.
Sorry, I was thinking of Interval<Instant>.
we could restrict those with this: ... typeguards
True, but seems much more boring-in-a-good-way to just have ZonedDateTimeInterval, and in general a more traditional Interval (only contains) -> CalendarInterval -> ZonedDateTimeInterval type hierarchy that progressively adds in more specific methods.
I really enjoyed using this Java library back in the day:
https://timeandmoney.sourceforge.net/
Ah yeah, still have a fork of it around:
(...and ^ has the concept of a super-generic Interval with more progressive subtypes)
There was a problem hiding this comment.
Not sure why you'd want to work with Instant from within business logic code. Instant is basically a "seconds since epoch" timestamp. It primarily exists in Temporal so you can interact with legacy js Date. The moment you start using it you open yourself up to making all sorts of mistakes that the zoned/plain types protect you from.
There was a problem hiding this comment.
Seems like kind of a deflection; Instant is a type in temporal, and Interval<Instant> seems like not an inconceivable ask.
I feel it's pretty objective that Interval-no-calendar-awareness -> CalendarInterval-calendar-awareness is an improvement/more correct than putting calendar-awareness on Interval and just saying "it should never be used for anything else".
There was a problem hiding this comment.
But I don't think that extends to "...and also instant should never be used".
I disagree. There is no benefit to using them as anything more than a transitory type. There are only downsides and footguns. I feel like you might just not have really looked deeply into the API? You really can't do very much with Instant other than convert them into other types. The few methods that do exist are very explicit they only work on units of hours, minutes, seconds, or smaller. All the same methods and properties are also available on ZonedDateTime (including the raw numeric timestamp), so there is really zero reason to ever use them. In the thousands of changes I made to graphql-service, I found zero uses for them.
Does renaming
IntervaltoCalendarInterval, and indefinitely tabling/deferring Interval sound okay?
I just still don't really see the benefit? It just seems like a longer name. Even if I was going to implement per type Intervals, I would want to nest them under the owning type's namespace, eg Temporal.PlainDate.Interval
There was a problem hiding this comment.
The benefit is future-proofing and clarity; you've not made a Temporal.Interval that supports all temporal types; you've made a CalendarInterval for calendared types.
Which is great, I really like it, but having used "just interval" types in the past, I would be surprised that a "generic interval" could do "calendar things".
There was a problem hiding this comment.
Future proofing for what? The spec has taken years to get to stage 3, a bunch of new types or dramatic changes to existing ones aren't going to suddenly emerge. Again, I feel like you are basing this on your experience with prior APIs and not on how Temporal is actually constructed (eg, see the object model). The only two types that aren't "calendered" are PlainTime and Instant. I've yet to find a use for either (beyond Date conversion), and have already expressed why I think use of Instant should not be encouraged. Maybe PlainTime is actually useful (some sort of scheduling?), but I don't think we need to go out of the way to future proof for it (ie, we could just have ClockInterval or something if we really had to). Additionally, Temporal already has interfaces (ie, Duration) that are shared between every date/time type and the parts of those interfaces each type can't use are simply ignored/not provided.
There was a problem hiding this comment.
Future proofing for a Interval<Instant>.
And, again, clarity: seeing eachMonth on a seems-generic/not-obviously-calendared type, in a library (temporal) specifically dedicated to strictly delineating calendared/non-calendared types, would, for me, violate least-surprise.
I.e. it would make my brain slow down and go "what? why does a non-calendared type support this? ... oh right, it is actually calendared".
I anticipate eventually making a breaking change to fix this, but 🤷 I'll approve.
There was a problem hiding this comment.
Future proofing for a
Interval<Instant>
Why? Can you give any concrete example where that would actually be uniquely useful (ie, couldn't just be done with ZonedDateTime) and not a footgun?
I.e. it would make my brain slow down and go "what? why does a non-calendared type support this? ... oh right, it is actually calendared".
Did your brain slow down and go "what?" when you saw that Duration has years / months / days /weeks properties then? Should it be renamed to CalendarDuration? It seems idiomatic of the spec to have a single interface shared by all types that simply isn't fully implemented for all of them.
I anticipate eventually making a breaking change to fix this, but 🤷 I'll approve.
How much money would you like to bet and what is the time frame? 😃
| // Convert to a plain date then add our business days | ||
| const result = this.toPlainDate().addBusinessDays(amount, options); | ||
| // Restore our time zone and wall clock time (ignore DST adjustments) | ||
| return result.toZonedDateTime({ timeZone: this.getTimeZone(), plainTime: this.toPlainTime() }); |
There was a problem hiding this comment.
I was initially worried seeing multiple add|removeBusinessDay files in the PR, but neat that we're just reusing the functionality. 👍
There was a problem hiding this comment.
We exclusively use this on PlainDate in graphql-service anyways, so this is really just here for completeness.
… toInstant to toTemporalInstant to match the spec
|
🎉 This PR is included in version 1.8.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This PR includes all the additions to move
graphql-serviceover fromdate-fns/luxonto purelyTemporal. Most of the tests are ported from the equivalentdate-fnsfunctions. Implementations / jsdoc have also been converted fromdate-fnswhere applicable.addBusinessDaysanddifferenceInBusinessDaysimplementations taken from prior version in this repo.Other changes: