Skip to content

Add Temporal support #636

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

Merged
merged 8 commits into from
Mar 24, 2025
Merged

Add Temporal support #636

merged 8 commits into from
Mar 24, 2025

Conversation

rkh
Copy link
Contributor

@rkh rkh commented Feb 15, 2025

This is an alternative to #635, adding only support for Temporal time objects without implementing a public variable caster API.

It still has some special handling to work fine in environments without Temporal. It also avoids running any such checks at load time, to allow loading a Temporal polyfill after loading fluent-bundle.

Duration

It does not implement Temporal.Duration support yet, which would require:

  • Augmenting built-in TypeScript Intl namespace to support Intl.DurationFormat and Intl.DurationFormatOptions
  • Add signature for Intl.DurationFormat to scope.memoizeIntlObject
  • Add FluentDuration wrapper for Temporal.Duration
  • Optional: Add DURATION builtin

Happy to work on that as well, but can also create a separate PR for that.

FluentVariable

I've taken the liberty to move FluentVariable to types.js – that made more sense to me, especially with the expanded list.

Copy link
Member

@eemeli eemeli left a comment

Choose a reason for hiding this comment

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

Looks very promising!

@rkh rkh requested review from eemeli and Demivan March 2, 2025 09:53
@rkh
Copy link
Contributor Author

rkh commented Mar 2, 2025

Incorporated the feedback and added more tests (different calendars, number conversion).

The main thing I see right now is the type import, which creates a TypeScript-only dependency on temporal-polyfill.

Copy link
Member

@eemeli eemeli left a comment

Choose a reason for hiding this comment

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

I'm a bit concerned about the temporal types getting included in the FluentVariable and FluentDateTime external types, as that's effectively making the temporal-spec package a runtime dependency. Could that be avoided somehow, even at the cost of making the types less exact?

@rkh
Copy link
Contributor Author

rkh commented Mar 3, 2025

I'm a bit concerned about the temporal types getting included in the FluentVariable and FluentDateTime external types, as that's effectively making the temporal-spec package a runtime dependency. Could that be avoided somehow, even at the cost of making the types less exact?

Yes, that's what I meant with my comment as well. Could type them as Temporal-like objects (which is a bit what I was trying to do in the first version). It would make more sense with what the code is like now, as type checks are only done in a single place. I'll see what I can do without copying over the full type spec.

@rkh
Copy link
Contributor Author

rkh commented Mar 3, 2025

FYI, I plan to work on this again later in the week.

@rkh rkh requested a review from eemeli March 7, 2025 08:57
@rkh
Copy link
Contributor Author

rkh commented Mar 7, 2025

Defined a minimal interface for the few fields the code uses from Temporal. The main issue I can see with that one is that TypeScript at least won't catch if an object is passed in that matches the interface but isn't a Temporal object.

But that's probably preferable over a dependency or shipping types for all the various Temporal classes.

This can be dropped as soon as TypeScript comes with Temporal support.

@rkh
Copy link
Contributor Author

rkh commented Mar 20, 2025

It looks like the tests are somehow timezone-dependent. Will check if I can reproduce that. Feedback on the patch itself beyond that more than welcome.

Copy link
Member

@eemeli eemeli left a comment

Choose a reason for hiding this comment

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

This looks good! Sorry for taking so long to reply; I've been on vacation. Once the tests pass, this is good to merge.

@rkh
Copy link
Contributor Author

rkh commented Mar 21, 2025

All good. Hope you had a nice vacation.
We've solved this differently for our app weeks ago, so this isn't a blocker for us.
Tests should be passing now.

@rkh
Copy link
Contributor Author

rkh commented Mar 21, 2025

Can reproduce the CI failure locally (seems to be Node 18 specific, I wonder if the polyfill is broken on that version).

@eemeli
Copy link
Member

eemeli commented Mar 21, 2025

It's almost certainly due to that behaviour of calendar: 'iso8601' requiring ICU 75. It's horribly hacky, but you can get the same effect by formatting the date in sv Swedish where that doesn't work.

@rkh
Copy link
Contributor Author

rkh commented Mar 22, 2025

I did a one line check whether iso8601 is supported or not, and skip the tests if it isn't. Probably out of scope for fluent.js to have a workaround.

@eemeli eemeli merged commit bbcf907 into projectfluent:main Mar 24, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants