Skip to content
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

Remove moment as a dependency #71

Open
jimmycallin opened this issue Feb 6, 2023 · 7 comments
Open

Remove moment as a dependency #71

jimmycallin opened this issue Feb 6, 2023 · 7 comments

Comments

@jimmycallin
Copy link
Contributor

The ftrack javascript API today uses moment extensively for date instances. Given that moment has been in maintenance mode for a few years now, and the community has moved on to other date libraries (or none at all), we are to remove it as a dependency from ftrack-javascript in our next major version.

What we are to use for date representation instead is still under investigation, but we will likely end up in either using ISO 8601 string representations, or the native date object.

@ffMathy
Copy link
Contributor

ffMathy commented Sep 15, 2023

Chiming in with an opinion here. I suggest using date objects, not strings.

If date manipulation is needed, I can recommend using date-fns.

@jimmycallin
Copy link
Contributor Author

Chiming in with an opinion here. I suggest using date objects, not strings.

If date manipulation is needed, I can recommend using date-fns.

We went back and forth on this a lot, and landed in ISO strings primarily for wanting the result to be serializable to ease client-server integrations. When we looked at other similar libraries, we noticed that a few (which I cannot find at the moment) had gone in this direction as well for this reason.

@ffMathy
Copy link
Contributor

ffMathy commented Sep 15, 2023

Okay. I guess I'll have to wait and see how it "feels", but as a consumer of the API, I don't enjoy strings a lot. Especially because the format always gets me. I'm always confused if I need to provide ISO strings, or whatever. It's not always clear.

@jimmycallin
Copy link
Contributor Author

Okay. I guess I'll have to wait and see how it "feels", but as a consumer of the API, I don't enjoy strings a lot. Especially because the format always gets me. I'm always confused if I need to provide ISO strings, or whatever. It's not always clear.

Totally understand this, I found a discussion that talks about this in a prisma context:
prisma/prisma#5522

In general ISO strings are very well supported in all date libraries, and easily converted to ISO strings again with date.toISOString().

You can already now try it out in your apps by passing decodeDatesAsIso: true to your query functions. I recommend that you do this ahead of the next major as a soft migration step, to reduce the amount of work necessary once we remove moment, but also for providing any feedback to us that would make us reconsider this decision.

@ffMathy
Copy link
Contributor

ffMathy commented Sep 15, 2023

Yeah that makes sense. But I still think there's value in not having to go look at some documentation first to see what format it's in.

Often I dont trust to just use ISO.

What are the arguments against date objects? Because right now I can only see benefits.

@jimmycallin
Copy link
Contributor Author

There are some benefits we see with using ISO date strings instead of the date objects:

  • It will increase performance of the client by not requiring traversal of the entire object tree to convert date strings to date objects, since we can just pass it on as is.
  • In react server components, all props needs to be serializable. Date objects are thus not supported and need to be converted to strings. By passing ISO strings and having the consumer be responsible to convert to their preferred format, we bypass this issue entirely. https://nextjs.org/docs/app/building-your-application/rendering/composition-patterns#passing-props-from-server-to-client-components-serialization
  • We are eagerly awaiting for the Temporal object, and would want to move away from date objects as soon as possible anyway. This avoids a potential future breaking change of the client where we convert from date objects to temporal.

The main disadvantages we see are:

@ffMathy
Copy link
Contributor

ffMathy commented Sep 18, 2023

Interesting. In the end, it's your decision (and I will of course respect it), but I will just chime in with a couple of thoughts:

It will increase performance of the client by not requiring traversal of the entire object tree to convert date strings to date objects, since we can just pass it on as is.

True, but is this a real bottleneck? I feel like the Ftrack backend and other IO will always be the primary cause of performance issues. If I do this on around 10.000 JSON objects, I still think it's doable to perform in less than 10 milliseconds. And if that's the case, then I think it is safe to assume that the execution time of a query itself on Ftrack will always take longer.

To me, it's kind of like arguing that Rust is faster than Node or whatever. But if you then end up using making a database call anyway, that single millisecond you saved means nothing when the database takes 1 second to execute. Would it make sense to try to make some real-world measurements of this?

If we get strong-typed queries (see #143), we could generate deserialization functions that don't need to do a check, but can always serialize and deserialize at the right time. Many OpenAPI or Swagger implementations do this today. They emit a function for parsing a particular response or request.

In react server components, all props needs to be serializable. Date objects are thus not supported and need to be converted to strings. By passing ISO strings and having the consumer be responsible to convert to their preferred format, we bypass this issue entirely. https://nextjs.org/docs/app/building-your-application/rendering/composition-patterns#passing-props-from-server-to-client-components-serialization

This is a fair point I guess. I have never seen React Server Components been used at all in my entire career yet, and if I've seen somebody else use it, it has definitely only been a fraction. Maybe I should "get out" more. I don't know 😄 But I am typically in touch with what's moving around me, and what's also happening within the organisations I work for.

I could be totally wrong about this since it's just based on my own observation, but I could fear that it's too early to say whether or not React Server Components will take off. There are countless alternatives right now such as how Next.js and Qwik is doing it, and the "winner" might end up being some third option. I will eat my own words if I end up being wrong, but just raising a flag here.

And assuming reality right now is that 95% of users are either web or node users, wouldn't it make sense to optimize for that? Then React Server Component users would have to stringify their dates beforehand (by using .toIsoString or whatever), but it'd affect a much smaller user-base.

We are eagerly awaiting for the Temporal object, and would want to move away from date objects as soon as possible anyway. This avoids a potential future breaking change of the client where we convert from date objects to temporal.

Didn't know about Temporal. Now I'm eagerly waiting too! 😆

Your arguments here are also really good. However, I do not see the issue of providing a little flexibility. Why not support both Date and Temporal when Temporal eventually comes out? Again, if the concern is performance, I'd again argue that it may be premature (see above).

And waiting for Temporal would require us to wait for Node to officially support it, but also for non-supporting Node versions to reach end-of-life, right? At least if you follow the same logic of not wanting to support both.

The typing is less strict, since they are returned as string rather than Date. We could look into typing them as a custom string literal instead: https://gist.github.com/MrChocolatine/367fb2a35d02f6175cc8ccb3d3a20054

Strong-typing the literal string (as we are doing) does not catch common mistakes. For instance, the date "2022-05-06" is valid. That means the 6th of May, 2022. However, in Danish (for instance) that same date is written as "2022-06-05". We'd love if the typings could catch those mistakes. But in the above example, it won't (both are valid).

You could argue that the same would happen for Date objects. However, there, the arguments are named. It's harder to put a month where a day goes, and vice versa.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants