Port commits from proposal repo (2024-02-25 tranche) #275
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
justingrant
force-pushed
the
port-commits-2024-02-26
branch
from
075aff1
to
3298e85
Compare
ptomato
approved these changes
Feb 26, 2024
lib/ecmascript.ts
Outdated
Comment on lines
1566
to
1593
| let year: number, | ||
| month: number, | ||
| day: number, | ||
| hour: number, | ||
| minute: number, | ||
| second: number, | ||
| millisecond: number, | ||
| microsecond: number, | ||
| nanosecond: number, | ||
| calendar; | ||
| let year, month, day, hour, minute, second, millisecond, microsecond, nanosecond, calendar; |
There was a problem hiding this comment.
Is it intentional that the type annotations disappeared here?
There was a problem hiding this comment.
This one was interesting. It was not intentional (was applied by a rebase) but interestingly TS doesn't complain because it's smart enough to deduce the type of an untyped let. So.... we could choose to leave it this way, or we could add types not just here, but to all untyped let's. I have a commit ready that will do just that, if we want to. The downside of adding all those types is that we'll have a lot more rebase conflicts in the future, which means extra work (likely for me!). Without, AFAICT, any additional type safety.
But I'm also happy to add all the type annotations if we think it's worthwhile.
@12wrigja what's your opinion?
There was a problem hiding this comment.
James is out on vacation, so to be conservative I just restored the previous type annotations here that rebasing removed. We can decide when James is back whether we want to add or remove type annotations from lets, given that TS is now pretty good at figuring out the correct type of an untyped let as it changes throughout a method.
lib/ecmascript.ts
Outdated
Comment on lines
1844
to
1870
| let year: number, | ||
| month: number, | ||
| day: number, | ||
| hour: number, | ||
| minute: number, | ||
| second: number, | ||
| millisecond: number, | ||
| microsecond: number, | ||
| nanosecond: number, | ||
| timeZone, | ||
| offset: string | undefined, | ||
| calendar: string | Temporal.CalendarProtocol | undefined; | ||
| let disambiguation: NonNullable<Temporal.ToInstantOptions['disambiguation']>; | ||
| let offsetOpt: NonNullable<Temporal.OffsetDisambiguationOptions['offset']>; | ||
| let year, month, day, hour, minute, second, millisecond, microsecond, nanosecond, timeZone, offset, calendar; | ||
| const resolvedOptions = SnapshotOwnProperties(GetOptionsObject(options), null); | ||
| let disambiguation, offsetOpt; |
There was a problem hiding this comment.
Let's discuss this here: #275 (comment)
There was a problem hiding this comment.
James is out on vacation, so to be conservative I just restored the previous type annotations here that rebasing removed. We can decide when James is back whether we want to add or remove type annotations from lets, given that TS is now pretty good at figuring out the correct type of an untyped let as it changes throughout a method.
12wrigja
approved these changes
Feb 29, 2024
This commit changes the ESLint and Prettier configuration to ignore test262. This is different from how upstream handles things. UPSTREAM_COMMIT=bc7639cb0acf5b92eddfa562d5f8082cb4b29204
It should be an error to return duplicate fields in a Calendar's field() method, thus we throw if we encounter one in PrepareTemporalFields. There are two cases (PlainMonthDay.prototype.toPlainDate and PlainYearMonth.prototype.toPlainDate) where, after checking on the return values of field() in previous invocations of PrepareTemporalFields, we want to deduplicate the field name list in a later call to PrepareTemporalFields after concatenating them, since we sort them in PrepareTemporalFields. For this reason, we add a duplicateBehavior parameter to PrepareTemporalFields. This allows us to remove the MergeLists abstract operation, now replaced by a simple list concatenation and deduplication in PrepareTemporalFields with duplicateBehavior set to ignore. It should also be an error to return field names 'constructor' or '__proto__' in a Calendar's field() method, and we throw if that happens. Fixes #2532, 2576. UPSTREAM_COMMIT=ca1e1c7c309f93dc6ebecadad5ab8c1ecd29fbe8
UPSTREAM_COMMIT=8b807320788c79769724d7be27bdd0a880129c0f
This editorial commit removes redundant spec text dealing with time formatting: * Adds a new AO FormatTimeString, and calls it in TemporalTimeToString, TemporalDateTimeToString, and TimeString (the legacy date AO in 262). * Removes FormatSecondsStringPart and replaces it with a new AO FormatFractionalSeconds, and call it in FormatTimeString and TemporalDurationToString. The text of this new AO is aligned with similar text in GetOffsetStringFor in #2607. * Replaces sub-second formatting text in TemporalDurationToString with a call to FormatFractionalSeconds. * Aligns polyfill code to these spec changes. * Adjusts polyfill code in a few places to better match the spec. Note that this commit doesn't touch spec text for formatting time zone offsets because there are several in-flight PRs dealing with offsets and I wanted to keep this PR merge-conflict-free. But once those PRs land and the dust settles, then I'll send another editorial PR to DRY offset string formatting too, by using FormatTimeString to replace bespoke formatting text for offsets. UPSTREAM_COMMIT=c429eed6a3e3d31d8f050ba692ea11a9150a62a2
This commit stops converting non-string inputs to strings when parsing ISO strings or property bag fields. When numbers are coerced to strings, the result is sometimes a valid ISO string subset but it often doesn't behave as expected. The result is often brittle, yielding "data driven exceptions" that we've tried to avoid. Numbers can also be ambiguous, e.g. is a Number passed for `offset` mean hours like an ISO string, minutes like Date.p.getTimezoneOffset, or msecs like Date.p.getTime(). This commit also removes coercing objects to strings in the same contexts (like TimeZone and Calendar constructors) because it's unclear that there are use cases for this coercion. UPSTREAM_COMMIT=772379413df5b364b79d41457798c5680da47d31
UPSTREAM_COMMIT=9253db8069db07c4094208dc284913e4b45bafba
At implementers' request to reduce the storage requirements of Temporal.TimeZone from 49+ bits to 12-13 bits, this commit requires that the [[OffsetNanoseconds]] internal slot of Temporal.TimeZone is limited to minute precision. Sub-minute precision is still allowed for custom time zone objects and built-in named time zones. In other words, this commit changes storage requirements but not internal calculation requirements. This commit is fairly narrow: * Changes |TimeZoneUTCOffsetName| production to restrict allowed offset syntax for parsing. * Changes FormatOffsetTimeZoneIdentifier AO to format minute strings only. * Moves sub-minute offset formatting from FormatOffsetTimeZoneIdentifier to instead be inlined in GetOffsetStringFor, which is now the only place where sub-minute offsets are formatted. Fixes #2593. UPSTREAM_COMMIT=70bc509f7e8079e3e7966dc321d857723a7ac802
UPSTREAM_COMMIT=861aba9d97335b99f4f2a4df9f6713a425c3657b
Now that we've limited TimeZone's [[OffsetNanoseconds]] internal slot to minute precision, this commit refactors TimeZone to clarify that only minutes are allowed in that slot and related abstract operations. Changes: * Renames TimeZone's [[OffsetNanoseconds]] internal slot to [[OffsetMinutes]] * Changes ParseTimeZoneIdentifier to return an [[OffsetMinutes]] field instead of an [[OffsetNanoseconds]] field. * Changes FormatOffsetTimeZoneIdentifier to expect a minutes argument. The goal of this change is to avoid the complexity and potential confusion from a slot and AOs that deal with "nanoseconds" values that nonetheless are restricted to minutes. UPSTREAM_COMMIT=683448732ce4c0dbcf9a74aceea845cd1c3a2411
Simplify offset formatting by using FormatTimeString instead of bespoke formatting logic. This commit completes the time-formatting refactor that UPSTREAM_COMMIT=9eb39e1d61c08c8bf8f3e0694412d4dbf3f4fbc9
UPSTREAM_COMMIT=e5577276a33ceb69bf26d472b98fa456187890cc
UPSTREAM_COMMIT=1dc2bbbd8a4ebf11b3e98e2bba21e0fb9e9bcc79
To make it easier to find actual Test262 gaps, this PR removes unreachable and unused code in ISO parsing. It also removes offset normalization code from ParseISODateTime, to better mach the spec that only normalizes downstream from ISO string parsing. Doing this exposed a test bug that's fixed in the second commit of tc39/test262#3877. UPSTREAM_COMMIT=212fffcd2df3d5ac6dbcc6abfb628d696c5de146
Recently the code for formatting UTC offsets with nanosecond precision was inlined into GetOffsetStringFor because that was the only place it was used. However, we need to use it in more than one place after the user code audit of #2519 because we'll be pre-calculating the UTC offset in cases where it's calculated more than once. Specifically, in ZonedDateTime.p.getISOFields(), so change that to use the new FormatUTCOffsetNanoseconds operation. UPSTREAM_COMMIT=c895534de684e1b74a6ea92e0545e6b17b259cd2
The polyfill's regexes were still allowing sub-minute offsets for ISO strings, even though in the spec an ISO string is syntactically invalid if it has a sub-minute annotation. This commit fixes this polyfill bug. Tests for Instant are in tc39/test262#3893. At some point in the future, we should probably add similar tests for all Temporal types that accept ISO strings. UPSTREAM_COMMIT=12740d19992f58c2a487168f8e90750ef1559ac6
* Update polyfill to match the refactored time zone parsing in the spec. * Rename `tzName` => `tzAnnotation` in the output of ParseISODateTime, to clarify that this field could be a name or an offset, and to avoid confusion with the `tzName` field that's returned from the new ParseTemporalTimeZoneString which really is an IANA name and is never an offset. * Many updates to validStrings.mjs tests to accommodate the changes above. UPSTREAM_COMMIT=2a5a15ba4ddb96c53fa1d51879075af00fb07d90
As part of this PR, I found some Test262 gaps and bugs. This commit catches up to include those new tests. UPSTREAM_COMMIT=e84c82fad3dbf2ffe2eb534a448239329adbd040
UPSTREAM_COMMIT=38f3f0f369ef59dfc041cd62ec11c566714b131e
Found by codecov: ParseTemporalInstantString already throws if neither `z` nor `offset` are present, so there's no need to check again after ParseTemporalInstantString returns. UPSTREAM_COMMIT=49ba35e39ed7fc27b7c49750142913c1fa6b75c8
Align RoundDuration checking of `relativeTo` to spec. All callers of this AO guarantee that `relativeTo` is one of `undefined`, a ZDT instance, or a PlainDate instance. UPSTREAM_COMMIT=27e045263d66e1c8bec6ec01ab6bb500555ee7df
If we convert an exact time into a PlainDateTime, we're already calculating the time zone's UTC offset for that exact time. In the case where we need the UTC offset for another purpose in the same method, we should not call the time zone method again to recalculate it. Instead, we call the getOffsetNanosecondsFor method once, and pass the result to the GetPlainDateTimeFor abstract operation. This affects the following methods, removing an observable property Get and observable Call to getOffsetNanosecondsFor: - Temporal.Instant.prototype.toString - Temporal.ZonedDateTime.prototype.toString - Temporal.ZonedDateTime.prototype.round - Temporal.ZonedDateTime.prototype.getISOFields UPSTREAM_COMMIT=b7a30a11afe25d7868b74ffe9008de86e7f1890d
Time units are no longer included in the array passed as the argument of Calendar.p.fields(). (And as long as we're doing this, we may as well limit fields() so that it doesn't accept time units. They are never used.) This doesn't eliminate any user-visible calls by itself, but is a prerequisite for eliminating the visible Gets of time unit properties on the receiver of PlainDateTime.p.with() and ZonedDateTime.p.with(). UPSTREAM_COMMIT=32cbd55dbf64838a6113d460705f674c3c4bc67d
In PlainDateTime.p.with() and ZonedDateTime.p.with(), avoid calling the property getters to obtain the values for the time units, since they do not need to go through the calendar; we can unobservably get the same values from the receiver's internal slots. In ZonedDateTime.p.with(), additionally we don't need to call the getter for the `offset` property. Since we have the offset nanoseconds already, we can do what the getter does and format an offset string with FormatTimeZoneOffsetString. UPSTREAM_COMMIT=e8e4501d27d463ffbe25994cb6a46689c9a3c9d2
…with() Instead of calling PrepareTemporalFields on the receiver in ZonedDateTime.p.with() to get the values of the date units, first create a PlainDateTime from the ZonedDateTime's exact time and time zone, and call PrepareTemporalFields on that. This still calls the corresponding calendar method for each field, but avoids calling the time zone's getOffsetNanosecondsFor() method redundantly for each field. UPSTREAM_COMMIT=ec00d92822b6ffaef233417f0a83e7cd0a47f7e6
Following the precedent set in #2447, if we're going to pass the options object to a calendar method we should make a copy of it. Also flatten the 'options' property once it's read and converted to a string in InterpretTemporalDateTimeFields, so that it doesn't have to be observably converted to a string again in Calendar.p.dateFromFields(). In PlainDateTime.from, delay validation of the options until after validation of the ISO string, for consistency with ZonedDateTime.from and in accordance with our general principle of validating arguments in order. This affects the following APIs, which are all callers of InterpretTemporalDateTimeFields: - Temporal.PlainDateTime.from() - Temporal.PlainDateTime.prototype.with() - Temporal.ZonedDateTime.from() - Temporal.ZonedDateTime.prototype.with() It does not affect ToRelativeTemporalObject, even though that also calls InterpretTemporalDateTimeFields, because it does not take an options object from userland. UPSTREAM_COMMIT=c775f411df08b245f754ca63ea725f170e62349c
…m,p.with}
Also following the precedent set in #2447, this preserves consistency with
analogous from/with operations in {Plain,Zoned}DateTime that need to read
the overflow option twice in order to deal with time and date units
separately.
UPSTREAM_COMMIT=d92a1bee1c3ea4ae6d672ff6959b3b73b32fc7fb
UPSTREAM_COMMIT=04f9544d02c46a4d5e2351d098e67b785769d75e
justingrant
force-pushed
the
port-commits-2024-02-26
branch
2 times, most recently
from
0702bfa
to
f0b175a
Compare
UPSTREAM_COMMIT=9378492cfba7bff3926679133b5b709bde1a1150
Fixes #2649. See #issuecomment-1684907910 for an explanation of why it's unreachable. UPSTREAM_COMMIT=1275241afdcb116c439d6cdea7dd0f98aadc76d7
UPSTREAM_COMMIT=4c10c80f4f19d1f980185d8662bd0b149d69d5e9
UPSTREAM_COMMIT=e09bf4f7774cd8ae94dbe243b7356fc03589218f
UPSTREAM_COMMIT=4345a8cbe90bd140321d31549a0382d3cbf2546b
justingrant
force-pushed
the
port-commits-2024-02-26
branch
2 times, most recently
from
d55f393
to
2dbce3b
Compare
* Change unnecessary `let` to `const`. * Add type annotations that were removed while rebasing, and add them for newly-added methods.
justingrant
force-pushed
the
port-commits-2024-02-26
branch
from
2e5e735
to
100ac63
Compare
|
Ah shoot, I merged this PR without remembering to autosquash the three fixup commits. Sorry guys! |
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 PR catches us up to 4345a8cbe90bd140321d31549a0382d3cbf2546b in the tc39/proposal-temporal repo.
Only 173 more commits to go. (!!!) Thankfully only a fraction of those are polyfill commits, so hopefully it won't be too heavy of a lift. Probably only 3-4 more PRs like this one.