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

Interval API #205

Closed
felixfbecker opened this issue Oct 27, 2019 · 9 comments
Closed

Interval API #205

felixfbecker opened this issue Oct 27, 2019 · 9 comments
Labels
feedback v2? Candidate for being moved to a Temporal v2 proposal

Comments

@felixfbecker
Copy link

felixfbecker commented Oct 27, 2019

I couldn't see this being discussed elsewhere, but has an interval API been thought about? Many use cases require passing around "start" and "end" points in time, and many calculations you want to do with Duration only work if you anchor it with a start or end date, like checking if two intervals (e.g. events) overlap, or if a point in time falls into an interval. ISO6801 defines intervals as a concept and their string representation.

The API could look roughly like this:

class Interval {
  static fromString(isoInterval: string): Interval
  
  // These are getters for convenience - only two of these actually need to be stored
  readonly start: Instant
  readonly end: Instant
  readonly duration: Duration
  
  // Provide two of the above to construct an interval
  constructor(values: IntervalLike)

  contains(value: Instant | Interval): boolean
  equals(interval: Interval): boolean
  overlap(interval: Interval): Interval | null
  
  with(changes: IntervalLike): Interval

  // Outputs ISO6801 Interval string
  toString(startTimeZone: TimeZone, endTimeZone: TimeZone): string
}

On a side note: I have a feeling considering these APIs that compose the objects already in the proposal is very important for making decisions such as whether to go with separate ZonedDateTime and the semantics of Duration. For example, without timezone information, we need to provide it to toString(), and duration can only be expressed in hours, not in days or larger (because depending on timezone there could be DST shifts within the interval which means not every day will be 24h long).

@ryzokuken ryzokuken added behavior Relating to behavior defined in the proposal non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved labels Oct 28, 2019
@ryzokuken ryzokuken added this to To do in Finalize spec text via automation Oct 28, 2019
@ryzokuken ryzokuken added this to To do in Finalize polyfill via automation Oct 28, 2019
@ryzokuken ryzokuken added this to the Stage 3 milestone Oct 28, 2019
@pipobscure
Copy link
Collaborator

This has actually been talked about. The conclusion was that we do not want to do this at this time, though there is nothing hindering a follow-on proposal.

Here are some of the reasons:

What is an Interval:

  • < Date : Date >
  • < Time : Time >
  • < DateTime : DateTime >
  • < Absolute : Absolute >

are just the ones that come to mind immediately. So in fact an Interval isn't a single thing, it's any one of several things. In short which one it should be is very much a business logic decision.

In addition there is nothing that prevents anyone from creating any of those Interval types.

This goes with the decision not to include the combination types for Absolute/TimeZone and DateTime/TimeZone either.

Finalize spec text automation moved this from To do to Done Oct 28, 2019
Finalize polyfill automation moved this from To do to Done Oct 28, 2019
@boeckMt
Copy link

boeckMt commented Jul 23, 2020

Is this something which is really not wanted?
Most of my time working with Dates, I have something like a date range/Interval with a start and an end point.
Then I need to check whether a Date/Time/Interval is in this range or calculate single steps between the two points given by a duration.

Something similar like luxon Interval would really be helpful!

@ptomato
Copy link
Collaborator

ptomato commented Jul 30, 2020

Thanks for the feedback! So if I understand correctly, this is the business logic that you would need:

  • A box of two orderable Temporal types (let's say Temporal.Absolute for now) representing a start and end point of an interval
  • A method for checking whether another Temporal.Absolute lies within the interval
  • A method for checking whether another interval lies within the interval
  • A method that iterates through Temporal.Absolutes starting at the start point and incrementing by a certain Temporal.Duration

The reason for not doing it so far is as stated above, there would be three (possibly four: Temporal.Date, Temporal.DateTime, Temporal.Absolute, maybe Temporal.YearMonth) interval types needed which would clutter the interface, whereas there's only one Luxon interval type because it deals with legacy Date. We'd expect that business logic would use at most one of the interval types.

See also #682 for discussion of the last item.

That said, I will reopen this issue and put it on our feedback list for consideration.


FWIW, off the top of my head it'd look something like this to implement it in your business logic:

class Interval {
  constructor(start, end) {
    this._start = Temporal.Absolute.from(start);
    this._end = Temporal.Absolute.from(end);
  }
  contains(absolute) {
    // assuming start inclusive, end exclusive?
    return Temporal.Absolute.compare(absolute, this._start) >= 0 &&
      Temporal.Absolute.compare(absolute, this._end) < 0;
  }
  encloses(interval) {
    return Temporal.Absolute.compare(interval._start, this._start) >= 0 &&
      Temporal.Absolute.compare(interval._end, this._end) < 0;
  }
  *iterate(duration) {
    let absolute = this._start;
    while (this.contains(absolute)) {
      yield absolute;
      absolute = absolute.plus(duration);
    }
  }
}

@ptomato ptomato reopened this Jul 30, 2020
Finalize spec text automation moved this from Done to In progress Jul 30, 2020
Finalize polyfill automation moved this from Done to In progress Jul 30, 2020
@ptomato ptomato added feedback and removed behavior Relating to behavior defined in the proposal non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved labels Jul 30, 2020
@boeckMt
Copy link

boeckMt commented Jul 30, 2020

Thanks for putting it on the feedback list!!

Yes my business logic would look something like your described Interval class except i also need sometimes that the end of the Interval is included (Temporal.Absolute.compare(interval._end, this._end) <= 0).

And if i understand the Temporal API correctly Temporal.Absolute.from is always a ISO 8601 string because the documentation described it as thing: any.


Maybe it would be possible to create something like this so the Interval would be related to its construction types...

type TemporalType = Temporal.Absolute | Temporal.DateTime | Temporal.YearMonth | Temporal.Date | Temporal.Time ...;

class Interval {
  constructor(start: TemporalType , end: TemporalType) {
    if(start !instanceof end){
      throw new TypeError(`start is not from the same type as end...`);
    }
    this._start = start;
    this._end = end;
    this._temporalType = start.constructor;
  }
  contains(temporalType: TemporalType, inclusive?: {...}) {
    // check if start or end should be inclusive
    const  containable = temporalType.toTemporalType(); //don't know how to do this right now
    return this._temporalType.compare(containable, this._start) >= 0 &&
      this._temporalType.compare(containable, this._end) < 0;
  }
  encloses(interval: Interval, inclusive?: {...}) {
    // check interval has the same TemporalType ...
    return this._temporalType.compare(interval._start, this._start) >= 0 &&
      this._temporalType.compare(interval._end, this._end) < 0;
  }
  *iterate(duration: Temporal.Duration) {
    let absolute = this._start;
    while (this.contains(absolute)) {
      yield absolute;
      absolute = absolute.plus(duration);
    }
  }
}

@pipobscure
Copy link
Collaborator

Just to add a bit of history. When we closed this issue, we were of a mindset to keep the API as slim as possible. For that reason we wanted to have as little in scope as possible. (No calendars, durations-type, etc).

We have since (with calendars I think it became real) abandoned the idea of going incremental and having the smallest possible API footprint. (While I’m still advocating to keep it small and not go over board).

So there is no fundamental reason why we should not have intervals.


At the same time: I seriously question the value of it. That’s because an interval makes sense both in terms of Absolute as well as DateTime as well as a mixture thereof (with one or the other at the start/end). That leaves 4 valid versions of interval that all function slightly differently. And they do that just to provide something that can be done in an if statements and a simple while loop (as demonstrated by @ptomato above).

So to my mind adding Interval needs a really good justification that goes way beyond just asking for reasons NOT to have it. I’m not sure which level of value-add it needs to provide, but it feels like the level would be quite near to ”cannot be done correctly in user-land”.

@stebogit
Copy link

stebogit commented Sep 8, 2020

I'd personally consider the concept of Interval a fundamental part of the Temporal object, like the DateTime, TimeZone and Duration classes. In fact, I believe it goes hand-in-hand with Duration like TimeZone goes with DateTime.
As side note, also PHP has a DateInterval class.

I think @boeckMt's draft implementation well abstracts the dependency to a specific Temporal type, allowing to apply the concept of interval to different scenarios and user defined detail, passed into the constructor or methods.
However I don't think you can really create an Interval instance with non timezone related types (like YearMonth, MonthDate, Date or Time), since you actually need the timezone for correct comparison or iteration inside the class methods (DST, leap years and such do matter here).

type TemporalType = Temporal.Absolute | Temporal.DateTime;

class Interval {
  constructor(start: TemporalType, end: TemporalType | Temporal.Duration) {
    if (end !instanceof Temporal.Duration && start !instanceof end){
      throw new TypeError(`start is not from the same type as end...`);
    }
    this._start = start;
    this._end = end instanceof Temporal.Duration ? start.plus(end) : end;
  }
  contains(datetime: TemporalType, inclusive?: {...}) {
    // check if start or end should be inclusive
    return containable.compare(this._start) >= 0 && containable.compare(this._end) < 0;
  }
  encloses(interval: Interval, inclusive?: {...}) {
    // check if start or end should be inclusive
    return this._start.compare(interval._start) >= 0 && this._end.compare(interval._end) < 0;
  }
  *iterate(duration: Temporal.Duration) {
    let value = this._start;
    while (this.contains(value)) {
      yield value;
      value = value.plus(duration);
    }
  }
}

@ptomato ptomato removed this from the Stage 3 milestone Sep 17, 2020
@ptomato ptomato added the v2? Candidate for being moved to a Temporal v2 proposal label Jan 13, 2021
@ptomato
Copy link
Collaborator

ptomato commented Feb 12, 2021

This is not present in the revision of Temporal that's currently under review, but let's continue gathering information at js-temporal/proposal-temporal-v2#4.

@ptomato ptomato closed this as completed Feb 12, 2021
Finalize spec text automation moved this from In progress to Done Feb 12, 2021
Finalize polyfill automation moved this from In progress to Done Feb 12, 2021
@sparebytes
Copy link

Using code from @stebogit and @boeckMt, I created the temporal-interval package which implements the Interval class. I expanded upon it, fixed some edge cases, and wrote unit tests.

https://www.npmjs.com/package/temporal-interval

A few other considerations:

  • implemented methods for equals and toDuration
  • provided a way to use this or any other polyfill package if desired
  • JSON.stringify(interval) results in { "start": "...", "end": "..." }
  • interval.toString() results in ${start}--${end}

@sandstrom
Copy link

I was mostly looking for a way of formatting (printing) a date format. In case others are looking for the same thing, there is already an API that does this today, available in most browers.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/DateTimeFormat/formatRange

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback v2? Candidate for being moved to a Temporal v2 proposal
Projects
Development

No branches or pull requests

8 participants