Skip to content

DRAFT: Change NudgeToCalendarUnit to use relative date when comparing durations #3172

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

DRAFT: Change NudgeToCalendarUnit to use relative date when comparing durations #3172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 8 additions & 27 deletions polyfill/lib/duration.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,34 +416,15 @@ export class Duration {

const largestUnit1 = ES.DefaultTemporalLargestUnit(one);
const largestUnit2 = ES.DefaultTemporalLargestUnit(two);
const duration1 = ES.ToInternalDurationRecord(one);
const duration2 = ES.ToInternalDurationRecord(two);

if (
zonedRelativeTo &&
(ES.TemporalUnitCategory(largestUnit1) === 'date' || ES.TemporalUnitCategory(largestUnit2) === 'date')
) {
const timeZone = GetSlot(zonedRelativeTo, TIME_ZONE);
const calendar = GetSlot(zonedRelativeTo, CALENDAR);
const epochNs = GetSlot(zonedRelativeTo, EPOCHNANOSECONDS);

const after1 = ES.AddZonedDateTime(epochNs, timeZone, calendar, duration1);
const after2 = ES.AddZonedDateTime(epochNs, timeZone, calendar, duration2);
return ES.ComparisonResult(after1.minus(after2).toJSNumber());
}

let d1 = duration1.date.days;
let d2 = duration2.date.days;
if (ES.IsCalendarUnit(largestUnit1) || ES.IsCalendarUnit(largestUnit2)) {
if (!plainRelativeTo) {
throw new RangeErrorCtor('A starting point is required for years, months, or weeks comparison');
}
d1 = ES.DateDurationDays(duration1.date, plainRelativeTo);
d2 = ES.DateDurationDays(duration2.date, plainRelativeTo);
}
const timeDuration1 = duration1.time.add24HourDays(d1);
const timeDuration2 = duration2.time.add24HourDays(d2);
return timeDuration1.cmp(timeDuration2);
return ES.CompareDurations(
ES.ToInternalDurationRecord(one),
ES.ToInternalDurationRecord(two),
zonedRelativeTo,
plainRelativeTo,
largestUnit1,
largestUnit2
);
}
}

Expand Down
63 changes: 60 additions & 3 deletions polyfill/lib/ecmascript.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3169,6 +3169,34 @@ export function DifferenceZonedDateTime(ns1, ns2, timeZone, calendar, largestUni
return CombineDateAndTimeDuration(dateDifference, timeDuration);
}

export function CompareDurations(duration1, duration2, zonedRelativeTo, plainRelativeTo, largestUnit1, largestUnit2) {
if (
zonedRelativeTo &&
(TemporalUnitCategory(largestUnit1) === 'date' || TemporalUnitCategory(largestUnit2) === 'date')
) {
const timeZone = GetSlot(zonedRelativeTo, TIME_ZONE);
const calendar = GetSlot(zonedRelativeTo, CALENDAR);
const epochNs = GetSlot(zonedRelativeTo, EPOCHNANOSECONDS);

const after1 = AddZonedDateTime(epochNs, timeZone, calendar, duration1);
const after2 = AddZonedDateTime(epochNs, timeZone, calendar, duration2);
return ComparisonResult(after1.minus(after2).toJSNumber());
}

let d1 = duration1.date.days;
let d2 = duration2.date.days;
if (IsCalendarUnit(largestUnit1) || IsCalendarUnit(largestUnit2)) {
if (!plainRelativeTo) {
throw new RangeErrorCtor('A starting point is required for years, months, or weeks comparison');
}
d1 = DateDurationDays(duration1.date, plainRelativeTo);
d2 = DateDurationDays(duration2.date, plainRelativeTo);
}
const timeDuration1 = duration1.time.add24HourDays(d1);
const timeDuration2 = duration2.time.add24HourDays(d2);
return timeDuration1.cmp(timeDuration2);
}

// Epoch-nanosecond bounding technique where the start/end of the calendar-unit
// interval are converted to epoch-nanosecond times and destEpochNs is nudged to
// either one.
Expand All @@ -3190,13 +3218,29 @@ function NudgeToCalendarUnit(
// Create a duration with smallestUnit trunc'd towards zero
// Create a separate duration that incorporates roundingIncrement
let r1, r2, startDuration, endDuration;
var didExpandCalendarUnit = false;
const compare = (d1, d2) => CompareDurations(d1, d2, undefined, ToTemporalDate(isoDateTime.isoDate), unit, unit);
var cmpResult = 0;
switch (unit) {
case 'year': {
const years = RoundNumberToIncrement(duration.date.years, increment, 'trunc');
r1 = years;
r2 = years + increment * sign;
startDuration = { years: r1, months: 0, weeks: 0, days: 0 };
endDuration = { ...startDuration, years: r2 };
cmpResult = compare(CombineDateAndTimeDuration(endDuration, TimeDuration.ZERO), duration);
if ((sign > 0 && cmpResult != 1) || (sign < 0 && cmpResult != -1)) {
didExpandCalendarUnit = true;
r1 = r2;
r2 = years + increment * 2 * sign;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the ×2 necessary here and in the months case below? Previously r1 and r2 would be exactly one rounding increment apart.

endDuration = { ...endDuration, years: r2 };
cmpResult = compare(CombineDateAndTimeDuration(endDuration, TimeDuration.ZERO), duration);
assert(
(sign > 0 && cmpResult == 1) || (sign < 0 && cmpResult == -1),
"nudgeToCalendarUnit: couldn't find larger duration"
);
startDuration = { ...startDuration, years: r1 };
}
break;
}
case 'month': {
Expand All @@ -3205,6 +3249,19 @@ function NudgeToCalendarUnit(
r2 = months + increment * sign;
startDuration = AdjustDateDurationRecord(duration.date, 0, 0, r1);
endDuration = AdjustDateDurationRecord(duration.date, 0, 0, r2);
cmpResult = compare(CombineDateAndTimeDuration(endDuration, TimeDuration.ZERO), duration);
if ((sign > 0 && cmpResult != 1) || (sign < 0 && cmpResult != -1)) {
didExpandCalendarUnit = true;
r1 = r2;
r2 = months + increment * 2 * sign;
endDuration = AdjustDateDurationRecord(duration.date, 0, 0, r2);
cmpResult = compare(CombineDateAndTimeDuration(endDuration, TimeDuration.ZERO), duration);
assert(
(sign > 0 && cmpResult == 1) || (sign < 0 && cmpResult == -1),
"nudgeToCalendarUnit: couldn't find larger duration"
);
startDuration = AdjustDateDurationRecord(duration.date, 0, 0, r1);
}
break;
}
case 'week': {
Expand Down Expand Up @@ -3240,7 +3297,7 @@ function NudgeToCalendarUnit(
// If the start of the bound is the same as the "origin" (aka relativeTo),
// use the origin's epoch-nanoseconds as-is instead of relying on isoDateTime,
// which then gets zoned and converted back to epoch-nanoseconds,
// which looses precision and creates a distorted bounding window.
// which loses precision and creates a distorted bounding window.
startEpochNs = originEpochNs;
} else {
const start = CalendarDateAdd(calendar, isoDateTime.isoDate, startDuration, 'constrain');
Expand Down Expand Up @@ -3278,8 +3335,8 @@ function NudgeToCalendarUnit(
assert(MathAbs(r1) <= MathAbs(total) && MathAbs(total) <= MathAbs(r2), 'r1 ≤ total ≤ r2');

// Determine whether expanded or contracted
const didExpandCalendarUnit = roundedUnit === MathAbs(r2);
duration = { date: didExpandCalendarUnit ? endDuration : startDuration, time: TimeDuration.ZERO };
didExpandCalendarUnit |= roundedUnit === MathAbs(r2);
duration = { date: roundedUnit == MathAbs(r2) ? endDuration : startDuration, time: TimeDuration.ZERO };

const nudgeResult = {
duration,
Expand Down