Temporal: Remove support for nested property bags from TimeZone/Calendar protocol, and change validation #3813
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
approved these changes
Apr 7, 2023
test/intl402/Temporal/ZonedDateTime/compare/argument-propertybag-timezone-string-datetime.js
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,24 @@ | |||
| // Copyright (C) 2022 Igalia, S.L. All rights reserved. | |||
There was a problem hiding this comment.
I saw a few brand-new tests in this PR that seem unrelated to the nested property bags. That seems fine to me, just wanted to confirm that this was intended.
There was a problem hiding this comment.
Yeah, the story behind this one is that it should've been here but for some reason was missing from PlainYearMonth.compare. (Probably an oversight on my part.) It would've previously had a nested property bag in it, which I would've removed in this PR.
test/built-ins/Temporal/Instant/prototype/toZonedDateTimeISO/timezone-wrong-type.js
Show resolved
Hide resolved
test/built-ins/Temporal/PlainDate/prototype/toString/calendarname-undefined.js
Show resolved
Hide resolved
Ms2ger
approved these changes
Apr 10, 2023
Previously, "nested" time zone property bags were unwrapped up to one
level. That is, this object:
{
timeZone: {
// ...Temporal.TimeZone methods
}
}
would not be considered to implement the TimeZone protocol, but would have
its timeZone property used instead, if it were passed to an API that
required a TimeZone protocol object.
These nested property bags are no longer supported. Discussion:
tc39/proposal-temporal#2104 (comment)
Corresponding normative PR:
tc39/proposal-temporal#2485
Previously, "nested" calendar property bags were unwrapped up to one
level. That is, this object:
{
calendar: {
// ...Temporal.Calendar methods
}
}
would not be considered to implement the Calendar protocol, but would have
its calendar property used instead, if it were passed to an API that
required a Calendar protocol object.
These nested property bags are no longer supported. Discussion:
tc39/proposal-temporal#2104 (comment)
Corresponding normative PR:
tc39/proposal-temporal#2485
Checking whether an object implements the TimeZone protocol is now done by means of HasProperty operations for each of the required methods unless the object already has the TimeZone brand. Discussion: tc39/proposal-temporal#2104 (comment) Corresponding normative PR: tc39/proposal-temporal#2485
Checking whether an object implements the Calendar protocol is now done by means of HasProperty operations for each of the required methods unless the object already has the Calendar brand. Discussion: tc39/proposal-temporal#2104 (comment) Corresponding normative PR: tc39/proposal-temporal#2485
Suggestion from Justin Grant's code review.
|
Thanks for the reviews! |
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.
Normative PR: tc39/proposal-temporal#2485 which reached consensus in the January/February 2023 TC39 meeting.
The required changes to tests involve a lot of search-and-replace so I'd recommend reviewing this PR commit by commit and probably skimming over the boring parts. The good news is that the tests become much simpler overall, reflecting a simplification of the TimeZone and Calendar protocols in Temporal.