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

Allow using either moment or luxon in pluggable fashion #5522

Closed
wants to merge 1 commit into from

Conversation

benmccann
Copy link
Contributor

@benmccann benmccann commented May 26, 2018

This PR only allows choosing between Luxon and Moment. The motivation for supporting Luxon is to reduce application size (#4303), add timezone support (#5186), and add support for multiple languages (#5664). Moment is retained for backwards compatibility. Luxon is smaller than Moment especially when using internationalization and time zones. It also improves upon Moment in several other ways such as by having an immutable API.

I do not think it would make sense to allow switching between additional date libraries in the long-term. Supporting switching between Moment/Luxon is not bad since the APIs are so similar, but it would be quite a bit of overhead in the code to support other date libraries as well.

date-fns is the other library that people have been asking us to support. date-fns is tiny and if we use it together with rollup then it introduces very little additional size. So even for users who are using Luxon or Moment in their own application there would not be much additional code size from chartjs using date-fns. I imagine that creating an adapter functionality to allow users to choose between Moment/Luxon/date-fns would introduce possibly as much additional size as just using date-fns directly and would be far more complex. However, date-fns wouldn't make sense to support in my mind until 3.0 when we can drop support for Moment, so that we don't have to create a really complex adapter. Also, more importantly, time zone support is still pending for date-fns which is a blocker and would need to be resolved before we could consider supporting it

With this PR, Moment will still be included in the bundled file. If you're including Chart.js via npm then moment will not be included automatically, but you will get a warning in the case that it is not included. Even if you include Luxon you will still get this warning. The warning can only be tied to a single dependency, so unfortunately we can't print the warning only in the case that neither Luxon nor Moment is included. Printing the warning always seemed perhaps the safer way for backwards compatibility

I updated the installation instructions, but I have not yet updated the README because that change would be deployed immediately. I will update the README when we're releasing Chart.js 2.8.0.

@benmccann benmccann force-pushed the optional-moment branch 2 times, most recently from de41588 to 5b6118a Compare May 26, 2018 06:10
@benmccann benmccann force-pushed the optional-moment branch 11 times, most recently from bbe30a3 to 06efd98 Compare June 4, 2018 04:37
@benmccann benmccann changed the title Setup moment to be pluggable Allow using either moment or luxon in pluggable fashion Jun 4, 2018
@benmccann benmccann force-pushed the optional-moment branch 4 times, most recently from 7b0d5ef to 1a1b5b4 Compare June 10, 2018 19:16
@benmccann benmccann force-pushed the optional-moment branch 2 times, most recently from c1107b1 to 9674ace Compare June 16, 2018 17:45
@Allsimon
Copy link

any ETA for this ?

@larizzatg
Copy link

It will be really nice if there was a way to don't use them if we don't need the time chart functions

@koresar
Copy link

koresar commented Sep 12, 2018

Any chance to process this PR @etimberg @simonbrunel? Currently moment.js takes ~50% of my website.

image

@larizzatg
Copy link

Waiting for the PR 🔢

@jazoom
Copy link

jazoom commented Oct 15, 2018

Is this still happening?

@koresar
Copy link

koresar commented Oct 15, 2018

Hey, people.

Let's reach out to the maintainers via Twitter. They might not seeing any of our begging.
Here is what I did: https://twitter.com/kore_sar/status/1051706638367830016
Need one more volunteer to do the same.

I believe this awesome project just needs more core maintainers with merge and publish access.

Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

I think some more isolation from moment/luxon would help here. We're still using some things that are specific to those libraries such as .startOf().

src/scales/scale.time.js Show resolved Hide resolved
@NoMaillard
Copy link

Wouldn't it be great if this could also include the integration of dayjs, it's super lightweight, compatible with the moment API, has i18n, timezone support

@pokonski
Copy link

This would be fantastic to have as I switched from Moment to Luxon, but some dependencies still depend on Moment, like Chart.js

@benmccann
Copy link
Contributor Author

I like that dayjs is smaller, but it looks like it's missing to quite a few features
It looks like it may not have date parsing: iamkun/dayjs#289
It doesn't have timezones: iamkun/dayjs#46
It's a bit limited for locales, which may be an issue for some of our users: iamkun/dayjs#171
There's also lots of open issues like subtracting days (iamkun/dayjs#384) and lots of unreviewed PRs that would make me nervous depending on it

@benmccann benmccann force-pushed the optional-moment branch 10 times, most recently from 76356ae to 63c1389 Compare December 21, 2018 03:34
@simonbrunel
Copy link
Member

Sorry for the long wait but I don't think this PR is going in the right direction, mainly because it locks us with an additional lib: Luxon (other reasons detailed in #5960 for those who are brave enough to read). That's why we explored different other approaches and came with another proposal in #5960 (feedback are more than welcome).

@benmccann
Copy link
Contributor Author

Closing this for now in favor of #5960. Thanks @simonbrunel for all your feedback on the proposal and working to come up with a good solution

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

Successfully merging this pull request may close these issues.

9 participants