search

boa-dev / boa

Boa is an embeddable and experimental Javascript engine written in Rust. Currently, it has support for some of the language.
MIT License
4.82k stars 385 forks source link

Remove `Temporal.Calendar` and `Temporal.TimeZone` #3890

Closed jedel1043 closed 3 days ago

jedel1043 commented 4 days ago

We will probably miss some changes when the spec is patched with the removals, but we can do a pass through temporal when that time comes.

Also checked all the regressions and they're failing because they try to access the removed Temporal.Calendar in some way or another, so that's fine.

github-actions[bot] commented 4 days ago

Test262 conformance changes

Test result main count PR count difference
Total 50,213 48,212 -2,001
Passed 42,981 42,611 -370
Ignored 1,411 1,413 +2
Failed 5,821 4,188 -1,633
Panics 0 0 0
Conformance 85.60% 88.38% +2.79%
Fixed tests (9): ``` test/built-ins/Temporal/PlainYearMonth/calendar-wrong-type.js (previously Failed) test/built-ins/Temporal/ZonedDateTime/from/argument-propertybag-calendar-number.js (previously Failed) test/built-ins/Temporal/PlainDateTime/calendar-wrong-type.js (previously Failed) test/built-ins/Temporal/PlainDateTime/calendar-string.js (previously Failed) test/built-ins/Temporal/PlainDateTime/constructor-full.js (previously Failed) test/built-ins/Temporal/PlainDate/calendar-undefined.js (previously Failed) test/built-ins/Temporal/PlainDate/calendar-wrong-type.js (previously Failed) test/built-ins/Temporal/PlainDate/basic.js (previously Failed) test/built-ins/Temporal/PlainDate/calendar-string.js (previously Failed) ```
nekevss commented 4 days ago

Hmm, would it be worth while doing this in an intermediate stage of removing the custom functionality and waiting for the PR on the specification for the removal of the entire builtin?

jedel1043 commented 4 days ago

Hmm, would it be worth while doing this in an intermediate stage of removing the custom functionality and waiting for the PR on the specification for the removal of the entire builtin?

That was the plan, but almost everything on the Temporal.Calendar and Temporal.TimeZone builtins depended on things that were removed in temporal_rs, so I had to remove all methods from those.

nekevss commented 4 days ago

The custom functionality was removed from both, but weren't the core methods kept for calendars and time zones?

jedel1043 commented 3 days ago

The custom functionality was removed from both, but weren't the core methods kept for calendars and time zones?

On the spec side? Nope, the plan is to delete both builtins and only be able to access the calendar/timezone by IANA ID or ca value, respectively.

On the temporal_rs side? Yes, we have the core methods still, it was just easier to remove everything instead of having to patch custom versions of those methods with all the custom calendar functionality removed.

nekevss commented 3 days ago

That makes sense. I'm just a little hesitant to preemptively remove the builtins based on the removal issues and plans without confirming with the champions the most recent consensus and timeline for the updates to the specification. Because the builtins are currently in the spec and test suite, and there is a history of things changing (even though that seems to be slowing down).

jedel1043 commented 3 days ago

I mean, there's already a PR to remove those builtins from the tests, and it has been approved already, so... https://github.com/tc39/test262/pull/4119