Skip to content
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

Temporal API removals #4119

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

Temporal API removals #4119

wants to merge 22 commits into from

Conversation

ptomato
Copy link
Contributor

@ptomato ptomato commented Jun 27, 2024

These are the test changes for the remaining API removals which reached TC39 consensus in June 2024: Temporal.Calendar, Temporal.TimeZone, the getCalendar methods, Temporal.ZonedDateTime.prototype.getTimeZone, and the getISOFields methods.

I'm well aware that this PR is thousands of files. I've tried to organize it logically into commits, so I recommend reviewing it commit by commit.

The bulk of the PR is the wholesale removal of 2250 files in the commit "Temporal: Remove tests directly relating to custom calendar and time zones". I would recommend skipping that one; you can verify mechanically that it's just file deletions.

That still leaves 1000 files to review. 200 of those are in "Temporal: Fix tests that unnecessarily create TimeZone or Calendar instances" which is a trivial, nearly mechanical replacement. Of the remaining 800, over half are spread out over the smaller commits, so shouldn't be too hard to review.

The only really tedious review will be of the commit "Temporal: Test adjustments for removing calendar and time zone objects", with 350 files, which consists of all of the various rewrites that we have to do, no way around it, that weren't easily organizable into other categories. However, a lot of these are deletions as well, so hopefully that's not too bad.

I'm open to suggestions for other ways to split this up to make reviews easier.

…nsition

This is being moved to a method on Temporal.ZonedDateTime.prototype. It
will not take a Temporal.Instant argument.

See: tc39/proposal-temporal#2826
Temporarily replace them with getISOFields().calendar/timeZone just to
keep the tests running until we remove Calendar and TimeZone objects
altogether.

See: tc39/proposal-temporal#2826
Some of the tests can be removed altogether since they deal with what
forms of input can be passed to ToTemporalTimeZoneSlotValue. Those are
tested on every method that takes a TimeZone as input.

Other tests are still relevant, but need to move to ZonedDateTime.p.equals
where the various quirks of time zone equality can still be tested. (Some
of these still will be removed in a following commit because they use
time zone objects.)

See: tc39#2826
…zones

These are tests that just won't apply anymore without custom calendars and
time zones.
…stances

In many cases we created a TimeZone or Calendar instance from a built-in
time zone or calendar. These tests can be trivially adapted to just use
the string ID.
…lpers

It's no longer possible to fake built-in time zones using custom objects.
So testing DST shifts will have to use real built-in time zones. Replace
TemporalHelpers.springForwardFallBackTimeZone with America/Vancouver (it
was modelled on the DST transitions in 2000) and
TemporalHelpers.crossDateLineTimeZone with Pacific/Apia (it was modelled
on the 2011 switch to the other side of the international date line.)

These tests have to move to the intl402/ folder since non-Intl-aware
implementations are allowed (but not required) to support any built-in
time zones other than UTC.
…esses

Many tests tested some functionality while asserting that there were no
calls of calendar or time zone methods. We can continue testing the
functionality, but there are no more methods to call, so we can delete
those parts of the tests.
Test the "wrong type of object" with Duration objects where appropriate,
otherwise just use plain objects.
This is no longer necessary if there are no calendar objects.
These are no longer possible without custom objects. Also add an exception
for calendar and timeZone properties in property bag observers so they are
not treated as objects.
…on-UTC time zone

Without custom calendars and time zones there are actually a bunch of
things that we now can't test on implementations that don't have non-ISO
calendars or non-UTC time zones. (Alternatively, we can say that these are
functionalities that those implementations don't have to implement.)
Previously getISOFields() was used to get the exact value of the
[[Calendar]] and [[TimeZone]] internal slots, as well as to get the
reference ISO year for PlainMonthDay and reference ISO day for
PlainYearMonth.

Use calendarId and timeZoneId for the former and toString() for the
latter.
… time zone

As in the previous commit, without custom calendars and time zones, some
functionality cannot be tested on implementations that don't have any
other calendars and time zones than iso8601 and UTC.

Some of the staging tests fall in this category. We take the opportunity
to port these into proper tests, in the intl402/ folder.
This test should trip if an implementation forgets to perform the removals
that reached consensus in June 2024. (Although such an implementation
would technically comply with the specification, if you really need to do
that, please open an issue on the Temporal proposal repo rather than just
skiplisting this test.)
…onstructors

Now that we don't have to deal with strings or objects as input to the
calendar ID or time zone ID parameter of constructors, we accept only the
data that actually goes into the internal slots.
Copy link
Contributor

@justingrant justingrant left a comment

Choose a reason for hiding this comment

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

Wow, amazing. I spot-checked a few files and they looked good. Thanks so much for persevering through this huge effort!

Copy link
Contributor

@Ms2ger Ms2ger left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants