-
Notifications
You must be signed in to change notification settings - Fork 79
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
Add Temporal support #636
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very promising!
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. |
There was a problem hiding this 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?
Co-authored-by: Eemeli Aro <[email protected]>
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. |
FYI, I plan to work on this again later in the week. |
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. |
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. |
There was a problem hiding this 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.
All good. Hope you had a nice vacation. |
Can reproduce the CI failure locally (seems to be Node 18 specific, I wonder if the polyfill is broken on that version). |
It's almost certainly due to that behaviour of |
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. |
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:Intl
namespace to supportIntl.DurationFormat
andIntl.DurationFormatOptions
Intl.DurationFormat
toscope.memoizeIntlObject
FluentDuration
wrapper forTemporal.Duration
DURATION
builtinHappy to work on that as well, but can also create a separate PR for that.
FluentVariable
I've taken the liberty to move
FluentVariable
totypes.js
– that made more sense to me, especially with the expanded list.