-
Notifications
You must be signed in to change notification settings - Fork 28
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
Update as of November 2023 #296
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Introduce a new spec type, Time Zone Methods Record, which stores a time zone object's getOffsetNanosecondsFor and getPossibleInstantsFor methods once they have been observably looked up. The record is passed around into abstract operations instead of the time zone object itself. Most API operations that do time zone calculations need to look up both getOffsetNanosecondsFor and getPossibleInstantsFor unconditionally. Only in the case where we convert a plain time to an exact time, it's possible we only need to look up getPossibleInstantsFor, or neither (if the offset is supplied.) This is a large commit but most of it is mechanical replacement of Temporal.TimeZone variables with Time Zone Record variables. UPSTREAM_COMMIT=fce7ec8a1fc603861f81d4a557f345af304a9d90
Introduce a new spec type, Calendar Methods Record, which stores a calendar object's methods once they have been observably looked up. The record is passed around into abstract operations instead of the calendar object itself. The mechanism doesn't currently allow for storing all Temporal.Calendar methods, only the ones that are explicitly called internally in other places than the Zoned and Plain types' methods of the same name, as part of a different API call. Some methods not included in the record are only called by the same-named methods on other Temporal types (example, Calendar.p.dayOfWeek is only called by ZonedDateTime.p.dayOfWeek, PlainDateTime.p.dayOfWeek, and PlainDate.p.dayOfWeek.) Other methods not included in the record are potentially called through lookups in PrepareTemporalFields, so it's not possible to pre-fetch them without making polyfilling difficult. (Example, Calendar.p.monthCode is called in `plainYearMonth.toPlainDate(...)` by looking up the `plainYearMonth.monthCode` property in PrepareTemporalFields. Shortcutting that lookup would mean that PlainDate.p.monthCode could never be polyfilled.) This is a large commit but most of it is mechanical replacement of Temporal.Calendar variables with Calendar Methods Record variables. UPSTREAM_COMMIT=57ec4591ec780db0249181e8ac2beff55fe8b2c7
This is an addendum to part 3 (#2671) that removes a ZonedDateTime to PlainDateTime conversion in one place where a fast path doesn't need the PlainDateTime. This is observable in the case where you add or subtract two Temporal.Durations that don't have any units higher than hours. Credit to Anba for spotting this. Closes: #2696 UPSTREAM_COMMIT=dce8cf42d30ca59ad6a41105d0a01096bfa659ef
UPSTREAM_COMMIT=49b3b77d5da73b7dfff629195428a827221941cc
justingrant
approved these changes
Feb 27, 2025
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.
Cool! Thanks for moving the ball fwd.
MethodName: TimeZonePrototypeKeys; | ||
}; | ||
|
||
// We switch off type checking when accessing the cached methods on this object. |
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.
Agree, no need to worry about making this work if you're gonna remove them soon.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This one is very few commits but a lot of changed lines. It corresponds pretty much to tc39/proposal-temporal#2519
I'll be the first to admit that the Typescript annotations in
methodrecord.ts
are not very good, and there's a lot of@ts-expect-error
needed to get it to compile. However, note that method records were removed from the proposal in June 2024. So I didn't want to spend a lot of time getting the types just right, since it'll be deleted in a forthcoming PR anyway.If there are any easy-to-implement suggestions to improve
methodrecord.ts
I'm happy to incorporate them, but if it's complicated I'd prefer to just leave it. The sloppy code will be gone soon enough!