Conversation
|
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. |
eemeli
left a comment
There was a problem hiding this comment.
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 <eemeli@gmail.com>
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. |
eemeli
left a comment
There was a problem hiding this comment.
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.Durationsupport yet, which would require:Intlnamespace to supportIntl.DurationFormatandIntl.DurationFormatOptionsIntl.DurationFormattoscope.memoizeIntlObjectFluentDurationwrapper forTemporal.DurationDURATIONbuiltinHappy to work on that as well, but can also create a separate PR for that.
FluentVariable
I've taken the liberty to move
FluentVariabletotypes.js– that made more sense to me, especially with the expanded list.