Migrate `Temporal` to its own crate. by nekevss · Pull Request #3461 · boa-dev/boa

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 A-Internal Changes that don't modify execution behaviour 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 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 a team and raskad 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

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

Closed

12 tasks

Uh oh!

Conversation

nekevss commented Nov 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test262 conformance changes

Uh oh!

codecov bot commented Nov 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

nekevss commented Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jedel1043 commented Nov 21, 2023

Uh oh!

nekevss commented Nov 21, 2023

Uh oh!

jedel1043 commented Nov 29, 2023

Uh oh!

HalidOdat left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raskad commented Dec 3, 2023

Uh oh!

jedel1043 Dec 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nekevss Dec 4, 2023

Choose a reason for hiding this comment

Uh oh!

jedel1043 Dec 3, 2023

Choose a reason for hiding this comment

Uh oh!

jedel1043 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jedel1043 Dec 3, 2023

Choose a reason for hiding this comment

Uh oh!

jedel1043 commented Dec 3, 2023

Uh oh!

jedel1043 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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