Migrate `Temporal` to its own crate. #3461

nekevss · 2023-11-11T17:35:46Z

This is the initial rough draft on separating Temporal into an engine agnostic crate. There's a lot in this branch approach-wise that I think is an improvement on the current implementation, and at the very least we may be able to work in to the current implementation in smaller pieces.

The biggest point of concern that is a potential blocker for this PR is the safety involving custom user calendars implementation (CustomRuntimeCalendar in boa_engine/src/builtins/temporal/calendar/object.rs). Currently, the implementation has CustomRuntimeCalendar hold a *mut Context to provide access to Boa's context when the user calendar is called by the temporal library.

Edit:

@jedel1043's suggestion of using &mut dyn Any and downcasting seems to work, so I'll work on cleaning this up and getting this in a more mergeable state! 😄

Feel free to leave any feedback whenever!

Also, as this is a more agnostic library, should we continue with the crate continue with the naming approach of the other crates (e.g. boa_temporal) or should the name be more of a generic name (e.g. temporal_rs).

Edit 2: Linking #1804 to this PR

github-actions · 2023-11-11T17:44:12Z

Test262 conformance changes

Test result	main count	PR count	difference
Total	95,609	95,609	0
Passed	76,526	76,526	0
Ignored	18,132	18,132	0
Failed	951	951	0
Panics	0	0	0
Conformance	80.04%	80.04%	0.00%

codecov · 2023-11-11T17:55:47Z

Codecov Report

Attention: 1429 lines in your changes are missing coverage. Please review.

Comparison is base (a8aad0b) 44.59% compared to head (1b2ae98) 45.11%.
Report is 12 commits behind head on main.

Files	Patch %	Lines
...oa_engine/src/builtins/temporal/calendar/object.rs	16.57%	292 Missing ⚠️
boa_temporal/src/fields.rs	0.00%	237 Missing ⚠️
boa_engine/src/builtins/temporal/calendar/mod.rs	29.08%	139 Missing ⚠️
boa_temporal/src/options.rs	0.00%	138 Missing ⚠️
boa_temporal/src/calendar.rs	12.80%	109 Missing ⚠️
boa_engine/src/builtins/temporal/duration/mod.rs	16.10%	99 Missing ⚠️
boa_temporal/src/calendar/iso.rs	13.41%	71 Missing ⚠️
boa_temporal/src/utils.rs	40.38%	62 Missing ⚠️
boa_temporal/src/iso.rs	41.34%	61 Missing ⚠️
boa_engine/src/builtins/temporal/fields.rs	0.00%	53 Missing ⚠️
... and 13 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3461      +/-   ##
==========================================
+ Coverage   44.59%   45.11%   +0.52%     
==========================================
  Files         487      496       +9     
  Lines       50601    50610       +9     
==========================================
+ Hits        22563    22833     +270     
+ Misses      28038    27777     -261

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

boa_engine/src/builtins/temporal/calendar/object.rs

nekevss · 2023-11-14T02:23:38Z

Marking this as ready for review as I think this is probably a good starting point to continue from for the crate. Date, Duration, DateTime, and the ISO fields have been migrated. The calendar protocol has also been moved over and can support a safe user defined "CustomRuntimeCalendar" implementation, which at the very least makes me a lot more comfortable with moving forward with this.

General thoughts:
There are definitely some points that can be improved on with future PRs (or this one if someone wants it added) or came up while migrating this.

The CalendarFields and Calendar.prototype.fields implementation is not complete in this migration and I need to reason about it more, at least spec-wise. My initial thought is that there needs to be an update to the CalendarProtocol or a new super trait entirely which felt outside of the scope for this PR specifically.
It would probably be worthwhile to migrate the ISO8601 parsing and nodes over to this crate as well, but it seemed like that could be completed in another PR as well if it was deemed as needed.
RoundingMode was copied over but not removed from builtins/options.rs due to RoundingMode being used in intl as well, I wasn't entirely sure what the preferred approach would be as far as, intl using the temporal crate's RoundingMode or keep them separate.
As in the above edit, I wasn't entirely clear what name/version should be put on this crate. I went with boa_temporal and the workspace version for consistency across the project. But as this crate would be rather nascent, I'm almost wondering if it should have a separate version (e.g. 0.0.1) that would better communicate the library's current state.

jedel1043 · 2023-11-21T01:47:01Z

I think it would be good to discuss with the whole team if we want to separate this crate from Boa or not, so I'll add it to tomorrow's discussion topics.

nekevss · 2023-11-21T01:57:35Z

Agreed 😄 I just wanted the branch to be up to date and ready if the decision was to move forward with it.

jedel1043 · 2023-11-29T19:00:54Z

@raskad @HalidOdat will try to review this in the next few days.

HalidOdat

Great work on this @nekevss!

There are some small things like docs, and the like but, we can leave those for future PRs :)

raskad · 2023-12-03T23:15:20Z

@jedel1043 I think Github will not let us merge this until you have approved the PR because you requested changes.

jedel1043 · 2023-12-03T23:19:59Z

boa_temporal/src/error.rs

+#[derive(Debug, Clone)]
+pub struct TemporalError {
+    kind: ErrorKind,
+    msg: Box<str>,
+}


Note for the future: Since Temporal overall has pretty specific errors, I think it would be good to check if we could avoid allocations and just return a plain:

enum TemporalError { Error1(Error1), Error2(Error2), Error3(Error3) }

And offer a TemporalError::get_ecma_error_type -> ErrorKind method.

That could be a good optimization to look into.

jedel1043 · 2023-12-03T23:23:38Z

boa_temporal/src/fields.rs

+    }
+}
+
+fn month_code_to_integer(mc: TinyStr4) -> TemporalResult<i32> {


Could probably use TinyAsciiStr<4> instead, since this type has an ominous warning:

These are temporary compatability reexports that will be removed in a future version.

jedel1043

Overall a really needed crateization! Just have a concern about the "context" feature, but after that's solved we can merge.

jedel1043 · 2023-12-03T23:27:05Z

boa_temporal/src/date.rs

+    ///
+    /// Temporal Equivalent: 3.5.13 `AddDate ( calendar, plainDate, duration [ , options [ , dateAdd ] ] )`
+    #[inline]
+    #[cfg(feature = "context")]


I don't think adding a feature that gates a parameter is the greatest idea. If two separate crates import boa_temporal but one of them uses the context feature and the other doesn't, it would break the build for the one that doesn't (cargo unifies features). A better idea would be to remove the feature and add a contextual_add_to_date method that has the optional parameter, and add_to_date shouldn't take any context parameter.

jedel1043 · 2023-12-03T23:45:24Z

I think Github will not let us merge this until you have approved the PR because you requested changes.

Yeah, I have a comment that definitely should be resolved before merging this.

jedel1043

Nice work!

jedel1043 requested changes Nov 11, 2023

View reviewed changes

boa_engine/src/builtins/temporal/calendar/object.rs Outdated Show resolved Hide resolved

nekevss force-pushed the temporal-crate-migration branch from b202355 to 87f86eb Compare November 12, 2023 03:40

nekevss requested a review from jedel1043 November 12, 2023 22:04

nekevss changed the title ~~WIP - Migrate Temporal to its own crate.~~ Migrate Temporal to its own crate. Nov 14, 2023

nekevss marked this pull request as ready for review November 14, 2023 01:43

nekevss requested a review from a team November 14, 2023 02:23

nekevss added the Internal Category for changelog label Nov 14, 2023

nekevss added this to the v0.18.0 milestone Nov 14, 2023

nekevss added 5 commits November 20, 2023 19:32

Update round and total method

f3af329

Begin Temporal crate migration

5e6e261

Add dyn Any context and some small changes

730cf8f

General completion and clean up work of migration

64bbfb8

Finish up clean up and linting of draft

4f576a9

nekevss force-pushed the temporal-crate-migration branch 2 times, most recently from 568206d to ac545bf Compare November 21, 2023 01:43

nekevss force-pushed the temporal-crate-migration branch from ac545bf to c251927 Compare November 21, 2023 01:55

Post-rebase update and a couple changes

08d8e02

nekevss force-pushed the temporal-crate-migration branch from c251927 to 08d8e02 Compare November 21, 2023 02:05

nekevss added 2 commits November 25, 2023 21:34

Rename and some cleanup

82aef33

Remove migrated record file

928ea22

jedel1043 added the waiting-on-review Waiting on reviews from the maintainers label Nov 29, 2023

jedel1043 assigned HalidOdat and raskad Nov 29, 2023

HalidOdat approved these changes Nov 30, 2023

View reviewed changes

nekevss mentioned this pull request Dec 3, 2023

Initial implementation of ZonedDateTime, TimeZone, and Instant #3495

Closed

nekevss requested review from raskad and a team December 3, 2023 22:39

raskad approved these changes Dec 3, 2023

View reviewed changes

jedel1043 reviewed Dec 3, 2023

View reviewed changes

jedel1043 requested changes Dec 3, 2023

View reviewed changes

Apply Review

1b2ae98

nekevss requested a review from jedel1043 December 4, 2023 01:57

jedel1043 approved these changes Dec 4, 2023

View reviewed changes

jedel1043 added this pull request to the merge queue Dec 4, 2023

Merged via the queue into main with commit e51e628 Dec 4, 2023
14 checks passed

jedel1043 deleted the temporal-crate-migration branch December 4, 2023 03:20

nekevss mentioned this pull request Jan 30, 2024

Tracking issue for the Temporal builtin #1804

Open

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate `Temporal` to its own crate. #3461

Migrate `Temporal` to its own crate. #3461

nekevss commented Nov 11, 2023 •

edited

Loading

github-actions bot commented Nov 11, 2023 •

edited

Loading

codecov bot commented Nov 11, 2023 •

edited

Loading

nekevss commented Nov 14, 2023 •

edited

Loading

jedel1043 commented Nov 21, 2023

nekevss commented Nov 21, 2023

jedel1043 commented Nov 29, 2023

HalidOdat left a comment •

edited

Loading

raskad commented Dec 3, 2023

jedel1043 Dec 3, 2023 •

edited

Loading

nekevss Dec 4, 2023

jedel1043 Dec 3, 2023

jedel1043 left a comment •

edited

Loading

jedel1043 Dec 3, 2023

jedel1043 commented Dec 3, 2023

jedel1043 left a comment

Migrate Temporal to its own crate. #3461

Migrate Temporal to its own crate. #3461

Conversation

nekevss commented Nov 11, 2023 • edited Loading

github-actions bot commented Nov 11, 2023 • edited Loading

Test262 conformance changes

codecov bot commented Nov 11, 2023 • edited Loading

Codecov Report

nekevss commented Nov 14, 2023 • edited Loading

jedel1043 commented Nov 21, 2023

nekevss commented Nov 21, 2023

jedel1043 commented Nov 29, 2023

HalidOdat left a comment • edited Loading

Choose a reason for hiding this comment

raskad commented Dec 3, 2023

jedel1043 Dec 3, 2023 • edited Loading

Choose a reason for hiding this comment

nekevss Dec 4, 2023

Choose a reason for hiding this comment

jedel1043 Dec 3, 2023

Choose a reason for hiding this comment

jedel1043 left a comment • edited Loading

Choose a reason for hiding this comment

jedel1043 Dec 3, 2023

Choose a reason for hiding this comment

jedel1043 commented Dec 3, 2023

jedel1043 left a comment

Choose a reason for hiding this comment

Migrate `Temporal` to its own crate. #3461

Migrate `Temporal` to its own crate. #3461

nekevss commented Nov 11, 2023 •

edited

Loading

github-actions bot commented Nov 11, 2023 •

edited

Loading

codecov bot commented Nov 11, 2023 •

edited

Loading

nekevss commented Nov 14, 2023 •

edited

Loading

HalidOdat left a comment •

edited

Loading

jedel1043 Dec 3, 2023 •

edited

Loading

jedel1043 left a comment •

edited

Loading