Switch from moment to luxon #469
Conversation
| * format coming from the Durable extension) | ||
| * @param input the formatted string | ||
| */ | ||
| public static durationFromString(input: string): Duration { |
There was a problem hiding this comment.
This was necessary because .NET serializes TimeSpan objects in this weird, non-ISO standard format: [-][d.]hh:mm:ss[.fffffff]. See here for more details. This is the format of the durations the SDK receives in its payload from the Extension. moment.js seemed to handle this fine out of the gate, but I couldn't find a luxon method that either accepts this format, or even allows specifying a custom format. The same is also true for dayjs, the other contender for the replacement. I know it's not ideal, but I also added unit tests to make sure this logic is sound, and step-by-step comments. If anyone has better suggestions, I'm happy to hear them. There is a GH issue here: moment/luxon#296 on luxon to add a Duration.fromFormat() method, which would help us in this case, but until that's closed, this is the best I could come up with :)
There was a problem hiding this comment.
Can we punt this PR until after preview? I believe we discussed this during triage and determined the durable package doesn't export any moment types in its public API (right?), so it's more of an internal engineering change as opposed to something that will directly benefit the user.
Even after preview, I think the fact that we have to manually implement durationFromString is reason enough to stay on moment. It's not like the package is dead - it's still in maintenance mode.
There was a problem hiding this comment.
The Durable SDK does list moment as one of its dependencies, and it does export moment types in its public types, as the type of some private fields, which are included in the auto-generated .d.ts files by default.
There was a problem hiding this comment.
@hossam-nasr: just to make sure I understand the context. If we're moving to manually generated types, would we be able to avoid exporting moment?
There was a problem hiding this comment.
@davidmrdavid Yes, I believe so. But it will stay in our dependencies, even if it's not in our public types (and therefore changing it in the future won't be a breaking change). We can def still do this after preview, but I thought we agreed during triage to just do it now if the fix is easy enough.
There was a problem hiding this comment.
it's not in our public types (and therefore changing it in the future won't be a breaking change)
Given this, I think we should stay on moment until Luxon or dayjs add support for the format we need
| @@ -154,6 +154,63 @@ describe("Utils", () => { | |||
| }).to.not.throw(); | |||
| }); | |||
| }); | |||
|
|
|||
| describe("durationFromString()", () => { | |||
There was a problem hiding this comment.
These unit tests were mostly derived from the examples on the .NET serialization docs here: https://learn.microsoft.com/en-us/dotnet/api/system.timespan.tostring?view=net-7.0#system-timespan-tostring
| @@ -46,7 +46,7 @@ | |||
| "axios": "^0.21.1", | |||
| "debug": "~2.6.9", | |||
| "lodash": "^4.17.15", | |||
| "moment": "^2.29.2", | |||
| "luxon": "^3.2.1", | |||
There was a problem hiding this comment.
Unless there's a compelling reason to use Luxon, I think we should use dayjs. It's way more popular than Luxon and it specifically calls itself an "alternative to Moment.js"
There was a problem hiding this comment.
I'm not sure why Luxon was picked as the alternative instead of dayjs. @davidmrdavid thoughts?
I will say, Luxon was also created by the creators of moment.js, and using Luxon for this migration, I've liked how more strict and type-safe it feels compared to moment.js, something that dayjs doesn't do from what I've seen.
There was a problem hiding this comment.
I think Luxon was suggested by a third party user who notified us of moment.js now longer actively developed. I have no strong feelings about the use of library so whatever works for you both, works for me.
| * format coming from the Durable extension) | ||
| * @param input the formatted string | ||
| */ | ||
| public static durationFromString(input: string): Duration { |
There was a problem hiding this comment.
it's not in our public types (and therefore changing it in the future won't be a breaking change)
Given this, I think we should stay on moment until Luxon or dayjs add support for the format we need
Closing this PR for now |
As in title, resolves #276