-
Notifications
You must be signed in to change notification settings - Fork 32
Rebase through early October 2024 #312
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
Conversation
In #2758 we kept the code paths for Duration.p.round() and Duration.p.total() the same. However, it is actually simpler to separate them in most places, except for NudgeToCalendarUnit. - Splits TotalRelativeDuration out of RoundRelativeDuration - Splits TotalTimeDuration out of RoundTimeDuration - Splits DifferenceZonedDateTimeWithTotal out of DifferenceZonedDateTimeWithRounding - Splits DifferencePlainDateTimeWithTotal out of DifferencePlainDateTimeWithRounding - Removes [[Total]] field from Nudge Result Record - Changes NudgeToCalendarUnit to return a record of { [[NudgeResult]], [[Total]] } Closes: #2947 UPSTREAM_COMMIT=ae6edf0262fed5adb35157e82c268254dff69c24
Fixes #2865 UPSTREAM_COMMIT=2c1e94e835b44286953d46fd5b3aeb3934234980
Move ISO Date Record creation out of the two branches of the algorithm. UPSTREAM_COMMIT=deff8b11db190c5ee5be5e22133db3b682532211
While investigating #2985 I noticed that ISODateToEpochDays was defined differently between the spec text and the reference code. Use 0-based months as in the spec text, to avoid confusion when comparing the two. Also I noticed that it can now be implemented more efficiently with GetUTCEpochMilliseconds. UPSTREAM_COMMIT=981ab0f374840bf7ae29e93b0f76641052aa7d16
We must avoid calling GetUTCEpochNanoseconds with dates that are too large because it is ill-defined in ECMA-262 what is supposed to happen. (See tc39/ecma262#1087). We had a condition in ToTemporalInstant that was supposed to prevent this, but was incorrect because subtracting the UTC offset could push the date out of range. Note that this is editorial, because IsValidEpochNanoseconds would throw anyway in this case even if GetUTCEpochNanoseconds was fully defined for large inputs. See: #2985 UPSTREAM_COMMIT=a2a16bccac9d79b2b7d2d24f8f8ab51ddbd49d3c
Similarly to the previous commit, we must avoid calling GetUTCEpochNanoseconds with dates that are too large because it is ill-defined in ECMA-262 what is supposed to happen. (See tc39/ecma262#1087). Previously to PR #2925, that could not happen because GetPossibleInstantsFor took a PlainDateTime object, but now it takes an ISO Date-Time Record. Note that this is editorial, because IsValidEpochNanoseconds would throw anyway in this case even if GetUTCEpochNanoseconds was fully defined for large inputs. UPSTREAM_COMMIT=80dd0718544f1dae6fe61ee2be81609544cb7319
Similarly to the previous commit, we must avoid calling GetUTCEpochNanoseconds with dates that are too large because it is ill-defined in ECMA-262 what is supposed to happen. (See tc39/ecma262#1087). Previously to PR #2925, that could not happen because we used a PlainDateTime object in InterpretISODateTimeOffset, but now we use an ISO Date-Time Record. Note that this is editorial, because IsValidEpochNanoseconds would throw anyway in this case even if GetUTCEpochNanoseconds was fully defined for large inputs. UPSTREAM_COMMIT=5d5582897686697862824f551645e03d1bb38e05
UPSTREAM_COMMIT=086d5f700f7851cba60d45b6ac33edf940c50e85
This is a shorthand operation, so use it instead of writing out all the fields with CreateISODateRecord. UPSTREAM_COMMIT=1b9ecf23492961fb69f8fd27b45bf4b99e140b09
In most cases, this means calling ISODateTimeToDateRecord on the ISO Date-Time Record. However, in the case of CombineISODateAndTimeRecord, I think it'd be more readable to just permit ISO Date-Time Records there, as we already did with the _time_ argument. Closes: #2990 UPSTREAM_COMMIT=a73f2482ae23b7f43d658fd63eb7df935c7e47d2
This is the only place it's called from. This refactor is part of consolidating the many calendar AOs. See: #2948 UPSTREAM_COMMIT=ae86a91b4c900cc9a3124874189620883359270e
They are identical except for clamping the day value, so if we pass 1 as the day value we get the same effect. The only caller was in CalendarYearMonthFromFields. See: #2948 UPSTREAM_COMMIT=444ec937cca777c2225132df6ff5d860da66bb72
This is the only place it's called from. This refactor is part of consolidating the many calendar AOs. See: #2948 UPSTREAM_COMMIT=0406eb1342197aa51385e9dde8d657ac577c5a63
Move the ISO 8601 code from CalendarDateFromFields and CalendarYearMonthFromFields into CalendarDateToISO. At this point it just consists of RegulateISODate anyway. To achieve this, we also drop the requirement that CalendarDateToISO checks its return value is in the representable range, instead moving that to CalendarDateFromFields and CalendarYearMonthFromFields since it is different in both cases. See: #2948 Closes: #2998 UPSTREAM_COMMIT=f3997ebab3e5ecfd018167337f14d48a924fdb55
Having two separate operations for this was already confusing just because of the names. Combine them into an ISO 8601 part and an implementation- defined part, as in CalendarDateToISO. See: #2948 UPSTREAM_COMMIT=2eb79f1dbd6837a136457e81d0c827d6d7464502
We had two versions of this code, one in the calendarImpl object and one in the helper object. Simply use the one in the helper object. UPSTREAM_COMMIT=435876ef6918d178d8bb436725eff51b4822edca
Move the ISO 8601-specific code from CalendarEra, CalendarYear, etc., into CalendarISOToDate. See: #2948 UPSTREAM_COMMIT=a35d4767925ee91708276c26d507a88138ca659b
Move the ISO 8601 code from ISOResolveMonth into CalendarResolveFields. See: #2948 UPSTREAM_COMMIT=8f903388cb23b2635201ead261d15e99c429e14b
These were all one line now, and should just call CalendarISOToDate. See: #2948 UPSTREAM_COMMIT=9202f1f4ffc436e0948097092cff6c1ec37ba530
Add a comment on where the formula (probably) came from, and give the variables more self-documenting names. UPSTREAM_COMMIT=8424055541b191589aacf4c97a33444a87ccb6d0
Split out some things into local variables for readability. UPSTREAM_COMMIT=8a790146e505254fbbeb93b1d5647aa562fff63f
…tion More hacks to deal with avoiding the circular import between calendar.mjs and ecmascript.mjs. UPSTREAM_COMMIT=b0e9b3a35313c6128b1fa122f12b6312999f7902
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. Nice work with the compex TS.
I had a few optional suggestions and questions.
day: number; | ||
dayOfWeek: number; | ||
dayOfYear: number; | ||
weekOfYear: { week: number; year: number } | { week: undefined; year: undefined }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to make sure I understand the intent: this type is either an object with both week and year present, or an object with week
and year
properties that are undefined
?
If that's correct, why not just use undefined
(not an object) for that latter case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I'll leave it as is so that the types match the upstream code for the time being, and I'll add this to my to-do list of minor fixes once the rebase is finished.
|
||
type ResolveFieldsReturn<Type extends ISODateToFieldsType> = Resolve< | ||
CalendarFieldsRecord & { | ||
year: Type extends 'date' ? number : never; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, this seems correct. Another option would be to put the entire object types into the conditional type (one which would have year and one which wouldn't), but think how you do it here is good because any access of year
should result in a compiler error.
isoToDate< | ||
Request extends Partial<Record<keyof CalendarDateRecord, true>>, | ||
T extends { | ||
[Field in keyof CalendarDateRecord]: Request extends { [K in Field]: true } ? CalendarDateRecord[Field] : never; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also looks correct, although it's complex enough that I can't be 100% sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure either, but I'm confident enough to merge it.
// The caller wants the offset to always win ('use') OR the caller is OK | ||
// with the offset winning ('prefer' or 'reject') as long as it's valid | ||
// for this timezone and date/time. | ||
if (offsetBehaviour === 'exact' || offsetOpt === 'use') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forget: did we agree to standardize on British or American spellings in our code? I have no preference, other than we should match one standard throughout if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TC39 uses British spelling in the specifications, so that's what I've been using.
@@ -3391,8 +3421,8 @@ export function UnbalanceDateDurationRelative(dateDuration: DateDuration, plainR | |||
// balance years, months, and weeks down to days | |||
const isoDate = TemporalObjectToISODateRecord(plainRelativeTo); | |||
const later = CalendarDateAdd(GetSlot(plainRelativeTo, CALENDAR), isoDate, yearsMonthsWeeksDuration, 'constrain'); | |||
const epochDaysEarlier = ISODateToEpochDays(isoDate.year, isoDate.month, isoDate.day); | |||
const epochDaysLater = ISODateToEpochDays(later.year, later.month, later.day); | |||
const epochDaysEarlier = ISODateToEpochDays(isoDate.year, isoDate.month - 1, isoDate.day); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why these -1s are here, so I'll assume they're correct!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to match the spec text: https://tc39.es/proposal-temporal/#sec-isodatetoepochdays
} | ||
get inLeapYear(): Return['inLeapYear'] { | ||
if (!ES.IsTemporalDateTime(this)) throw new TypeErrorCtor('invalid receiver'); | ||
const isoDate = ES.TemporalObjectToISODateRecord(this); | ||
return ES.CalendarInLeapYear(GetSlot(this, CALENDAR), isoDate); | ||
return ES.calendarImplForObj(this).isoToDate(isoDate, { inLeapYear: true }).inLeapYear; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a lot of repetitive code here. Could you explain a bit about this pattern? If only one scalar is needed, I wonder if there's a simpler way to get it than this { fieldName: true }
parameter followed by a property getter. Also the same pattern is repeated in every method, could it be DRY-ed? Unless I'm missing something, could all these 2-line repeated calls be coalesced into a single shared function with a signature like this?
ES.getCalendarProperty(this, 'inLeapYear')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a couple of other places where we fetch multiple scalars at once (e.g. ISODateToFields
, calendarDateWeekOfYear
.) But I like the suggestion to reduce the repetition in the many public property getters. I've added it to my to-do list of cleanups after the rebase is done.
Thanks for the review! |
Includes a refactor of calendar operations.