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

Remove options parameter from Duration.p.add/subtract #2825

Closed
ptomato opened this issue Apr 18, 2024 · 3 comments · Fixed by #2841
Closed

Remove options parameter from Duration.p.add/subtract #2825

ptomato opened this issue Apr 18, 2024 · 3 comments · Fixed by #2841
Assignees
Labels
normative Would be a normative change to the proposal
Milestone

Comments

@ptomato
Copy link
Collaborator

ptomato commented Apr 18, 2024

Originally emerged as a solution to #2811. I'm collecting the consensus from 2024-04-18 in a new issue and closing all the outdated ones.

In #2811 we determined that Duration.p.add/subtract with a ZonedDateTime relativeTo parameter can result in a DST adjustment that can be surprising and in some cases might not be what callers want. We analyzed the use cases for adding/subtracting durations and figured that adding/subtracting durations with years, months, and weeks relative to a starting point is relatively rare, and maybe better done explicitly:

// Before
duration1.add(duration2, { relativeTo })

// After
relativeTo.until(relativeTo.add(duration1).add(duration2))

This is also an opportunity to reduce the size of the proposal and the implementation complexity.

In terms of observable behaviour:

  • Remove the options parameter from Temporal.Duration.prototype.add and Temporal.Duration.prototype.subtract
  • Everything else remains the same:
    • Additions and subtractions that previously succeeded without relativeTo, still succeed, with the same results
    • Additions and subtractions that previously threw if there was no relativeTo, now just throw unconditionally

Of course, there are a few other things that need to be done:

@khawarizmus
Copy link
Contributor

What is the implication of this decision on calculating rrule occurrences based on RFC 5545?

I have implemented an rrule calculator strictly adhering to RFC 5545 using Temporal. and this issue draw my attention that i am not using relativeTo. Should that be the case?

in my implementation i have this lines of code:

// expand or limit occurrence from start
// ...
// Calculate the next date to work with according to the frequency
start = start.add(this._increment(start))

// where increment looks like this
// returns the increment to be used when generating the recurrences
  private _increment(start: StartDateTime): Temporal.Duration {
    const interval = this._props.interval ?? 1
    switch (this._props.freq) {
      case 'YEARLY':
        return Temporal.Duration.from({ years: 1 * interval })
      case 'MONTHLY':
        return Temporal.Duration.from({ months: 1 * interval })
      case 'WEEKLY':
      {
        if (this._startDate.toString() === start.toString()) {
          // For the first occurrence, use the interval directly
          return Temporal.Duration.from({ weeks: interval })
        }
        else {
          if (interval === 1) {
            // No adjustment for week start needed if interval is 1 week
            return Temporal.Duration.from({ days: 7 })
          }
          else {
            let nextStart = start.add({ weeks: interval })
            // Convert _weekStart to Temporal's dayOfWeek format (1 for Monday, 7 for Sunday)
            const adjustedWeekStart = this._weekStart === 0 ? 7 : this._weekStart + 1
            const dayOfWeek = nextStart.dayOfWeek

            // Check if adjustment for weekStart is needed
            if (dayOfWeek !== adjustedWeekStart) {
              const daysToNextWeekStart = (adjustedWeekStart - dayOfWeek + 7) % 7
              const potentialNextStart = nextStart.add({ days: daysToNextWeekStart })

              // Only adjust if the next weekStart is within the current interval
              if (potentialNextStart.since(start, { largestUnit: 'days' }).days <= 7 * interval) {
                nextStart = potentialNextStart
              }
            }

            return nextStart.since(start, { largestUnit: 'days' })
          }
        }
      }
      case 'DAILY':
        return Temporal.Duration.from({ days: 1 * interval })
      case 'HOURLY':
        return Temporal.Duration.from({ hours: 1 * interval })
      case 'MINUTELY':
        return Temporal.Duration.from({ minutes: 1 * interval })
      case 'SECONDLY':
        return Temporal.Duration.from({ seconds: 1 * interval })
    }
  }

@ptomato
Copy link
Collaborator Author

ptomato commented Apr 22, 2024

I don't think you need to use relativeTo there, because you're always adding durations to a datetime (I'm not sure whether it's PlainDate, PlainDateTime, or ZonedDateTime, but it doesn't matter.) The datetime is already the relativeTo point.

@felixfbecker
Copy link

I honestly find the "After" example much clearer to understand than the "Before" 👍🏻
With relativeTo, it wasn't as clear whether relativeTo marks the starting point of duration1, or the end point/start point of duration2. In the "After" example it is explicit.

@ptomato ptomato self-assigned this May 8, 2024
ptomato added a commit that referenced this issue May 15, 2024
Removes the options parameter from Temporal.Duration.prototype.add and
Temporal.Duration.prototype.subtract. Everything else remains the same:
Additions and subtractions that previously succeeded without relativeTo,
still succeed, with the same results. Additions and subtractions that
previously threw if there was no relativeTo, now just throw
unconditionally.

Closes: #2825
ptomato added a commit to ptomato/test262 that referenced this issue May 15, 2024
See tc39/proposal-temporal#2825. This is a mass removal of tests that use
this functionality, in a separate commit for ease of review. Further
adjustments will be made in the following commit.
ptomato added a commit to ptomato/test262 that referenced this issue May 15, 2024
See tc39/proposal-temporal#2825.

Various edits to existing tests so that they make more sense with the
removal of relativeTo.

New tests specifically testing that calendar units cannot be added or
subtracted directly.
ptomato added a commit to ptomato/test262 that referenced this issue May 26, 2024
See tc39/proposal-temporal#2825. This is a mass removal of tests that use
this functionality, in a separate commit for ease of review. Further
adjustments will be made in the following commit.
ptomato added a commit to ptomato/test262 that referenced this issue May 26, 2024
See tc39/proposal-temporal#2825.

Various edits to existing tests so that they make more sense with the
removal of relativeTo.

New test specifically testing that calendar units cannot be added
directly.
ptomato added a commit that referenced this issue May 26, 2024
Removes the options parameter from Temporal.Duration.prototype.add and
Temporal.Duration.prototype.subtract. Everything else remains the same:
Additions and subtractions that previously succeeded without relativeTo,
still succeed, with the same results. Additions and subtractions that
previously threw if there was no relativeTo, now just throw
unconditionally.

Closes: #2825
ptomato added a commit that referenced this issue May 27, 2024
Removes the options parameter from Temporal.Duration.prototype.add and
Temporal.Duration.prototype.subtract. Everything else remains the same:
Additions and subtractions that previously succeeded without relativeTo,
still succeed, with the same results. Additions and subtractions that
previously threw if there was no relativeTo, now just throw
unconditionally.

Closes: #2825
ptomato added a commit that referenced this issue May 27, 2024
Removes the options parameter from Temporal.Duration.prototype.add and
Temporal.Duration.prototype.subtract. Everything else remains the same:
Additions and subtractions that previously succeeded without relativeTo,
still succeed, with the same results. Additions and subtractions that
previously threw if there was no relativeTo, now just throw
unconditionally.

Closes: #2825
Aditi-1400 pushed a commit to Aditi-1400/proposal-temporal that referenced this issue May 31, 2024
Removes the options parameter from Temporal.Duration.prototype.add and
Temporal.Duration.prototype.subtract. Everything else remains the same:
Additions and subtractions that previously succeeded without relativeTo,
still succeed, with the same results. Additions and subtractions that
previously threw if there was no relativeTo, now just throw
unconditionally.

Closes: tc39#2825
Aditi-1400 pushed a commit that referenced this issue May 31, 2024
Removes the options parameter from Temporal.Duration.prototype.add and
Temporal.Duration.prototype.subtract. Everything else remains the same:
Additions and subtractions that previously succeeded without relativeTo,
still succeed, with the same results. Additions and subtractions that
previously threw if there was no relativeTo, now just throw
unconditionally.

Closes: #2825
ptomato added a commit that referenced this issue Jun 5, 2024
Removes the options parameter from Temporal.Duration.prototype.add and
Temporal.Duration.prototype.subtract. Everything else remains the same:
Additions and subtractions that previously succeeded without relativeTo,
still succeed, with the same results. Additions and subtractions that
previously threw if there was no relativeTo, now just throw
unconditionally.

Closes: #2825
ptomato added a commit to ptomato/test262 that referenced this issue Jun 13, 2024
See tc39/proposal-temporal#2825. This is a mass removal of tests that use
this functionality, in a separate commit for ease of review. Further
adjustments will be made in the following commit.
ptomato added a commit to ptomato/test262 that referenced this issue Jun 13, 2024
See tc39/proposal-temporal#2825.

Various edits to existing tests so that they make more sense with the
removal of relativeTo.

New tests specifically testing that calendar units cannot be added or
subtracted directly.
ptomato added a commit to tc39/test262 that referenced this issue Jun 13, 2024
See tc39/proposal-temporal#2825. This is a mass removal of tests that use
this functionality, in a separate commit for ease of review. Further
adjustments will be made in the following commit.
ptomato added a commit to tc39/test262 that referenced this issue Jun 13, 2024
See tc39/proposal-temporal#2825.

Various edits to existing tests so that they make more sense with the
removal of relativeTo.

New tests specifically testing that calendar units cannot be added or
subtracted directly.
ptomato added a commit that referenced this issue Jun 13, 2024
Removes the options parameter from Temporal.Duration.prototype.add and
Temporal.Duration.prototype.subtract. Everything else remains the same:
Additions and subtractions that previously succeeded without relativeTo,
still succeed, with the same results. Additions and subtractions that
previously threw if there was no relativeTo, now just throw
unconditionally.

Closes: #2825
ptomato added a commit that referenced this issue Jun 13, 2024
Removes the options parameter from Temporal.Duration.prototype.add and
Temporal.Duration.prototype.subtract. Everything else remains the same:
Additions and subtractions that previously succeeded without relativeTo,
still succeed, with the same results. Additions and subtractions that
previously threw if there was no relativeTo, now just throw
unconditionally.

Closes: #2825
ptomato added a commit to ptomato/test262 that referenced this issue Jun 13, 2024
See tc39/proposal-temporal#2825. This is a mass removal of tests that use
this functionality, in a separate commit for ease of review. Further
adjustments will be made in the following commit.
ptomato added a commit to ptomato/test262 that referenced this issue Jun 13, 2024
See tc39/proposal-temporal#2825.

Various edits to existing tests so that they make more sense with the
removal of relativeTo.

New test specifically testing that calendar units cannot be added
directly.
ptomato added a commit that referenced this issue Jun 13, 2024
Removes the options parameter from Temporal.Duration.prototype.add and
Temporal.Duration.prototype.subtract. Everything else remains the same:
Additions and subtractions that previously succeeded without relativeTo,
still succeed, with the same results. Additions and subtractions that
previously threw if there was no relativeTo, now just throw
unconditionally.

Closes: #2825
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
normative Would be a normative change to the proposal
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants