-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Do not require moment.js #4303
Comments
I think lack of time zone support would be the main obstacle But this brings up another good point. It seems like it'd be helpful for chartjs to support tree shaking. If I'm only using one chart type then why should I have to include the code for all chart types? I believe moving to ES6 modules would fix that. Perhaps we should have a separate issue to track that |
@benmccann not sure we need a new ticket, I'm already on reorganizing the - weird - I remember a few tickets that suggested other date/time libraries: an idea would be to have our own time/date wrapper and provide a few adapters to popular libs such as moment or date-fns. The adapter could be automatically selected based on some global detection or enforced by the user. And maybe we should deprecate the |
Oh, that's awesome that you're already working on a migration to ES6 modules. Btw, I wasn't suggesting been would tree shake our own modules. Rather that if we use ES6 modules then end users can tree shake out chartjs modules that they don't need. |
Agree, that would be convenient, though I think it's for "advanced" users that want to build the lib by their own. For others, it would be great if |
Funny that #4461 got filed about this very topic today |
Does chart.js use moment timezone? How does a chart library require timezone support? Date/time library adapters are expensive, IMO. Anyway, tree-shaking is kinda inconsistent across bundlers. E.g. Webpack cannot prune unused pure imports, rollup can. |
Chart.js doesn't use time zone itself, but It looks like some people use the client-side time zone support that moment provides to format/parse chart.js labels. E.g.: https://stackoverflow.com/questions/40891462/chart-js-display-control-timescale-time-zone/41715066#41715066 It would be cheaper from a library size perspective to do any timezone handling on the server side, so I'd be fine advising people to do that instead. |
By expensive I mean, the burden of building adapters which supports a variety of date/time libraries should not be on the Chart.js end. |
I wasn't thinking writing tons of adapters, but at least one (moment.js?) and accept contributions from users who want to interface with another lib. The idea behind this would be to keep the Chart.js code independent of any third party date/time lib and allow users to make their project depends on whatever lib they want. Because right now, switching to another lib may be a breaking change for external charts/plugins and I would prefer not to have to change again for a better library if someone else think that finally Chart.js shouldn't use
|
For example: #3533 |
I understand the benefit of adapters here. There is a small difference between injecting a date/time library and my proposed approach. |
So why do I keep insisting on import { format, parse } from 'date-fns/esm'
// No extra bundle garbage
import format from 'date-fns/format'
import parse from 'date-fns/parse'
// This also works |
|
Hope it arrives soon. I will have my own fork of chart.js-no-moment.js for now. |
Sounds good, can you share your repo/branch once ready, I'm curious to see what really means switching to another date/time lib :) |
Yep. Looking forward to your feedbacks when it's done. |
@hiendv can you publish your fork with no moment? Or comment on how to do that? |
To help with the switch, I've created a gist of every instance of moment within Chart.js https://gist.github.com/merlinpatt/9925bbac94b0c91f1430c2de1569bb26 |
Just wondering whether this is still needed as momentjs is now moving to ES6 |
Can you share an issue or pull request that describes moment's move to es6? I wasn't aware of it and am not familiar with the details |
I've sent a PR to add support for Luxon: #5522. This is a big improvement over Moment and a much easier switch to make than date-fns because the API is so similar Luxon is much smaller than Moment and adds timezone support, which will allow us to fix existing timezone bugs and add new functionality. I've also been working to help make Luxon even smaller than it currently is. There's a 2.0 branch of Luxon where some work has started to help allow additional tree shaking (though it will never be as shakable as date-fns) date-fns is not well maintained and does not support timezones, so I'm not sure I want to add a dependency on it. I sent a PR to add timezone support to it months ago and there's been basically no movement. It seems there's a positive reaction to the approach I was taking, but just a lack of maintainer time to review any PRs |
…merge of this will happen; once chartjs/Chart.js#4303 has plugin based solution; let's drive this again
@dmtrKovalenko (creator of material-ui-pickers) has recently split out his date abstraction library: https://github.com/dmtrKovalenko/date-io. |
@geirsagberg that looks fantastic! We will absolutely consider it. I have a few questions about it that I left in the project's issue tracker to understand how / if we can use it |
Sorry but It is not clear to me, do exist a way to use chart without moment? |
@ciaoben Not at the moment (heh). Using a similar approach to the repo I linked earlier should make it possible, but I imagine it will require quite a bit of effort. |
Is there any quick workaround ? I do not use graphs with date, so I really don't need moments and I would like to reduce size of module. How can we found quick workaround ? |
It looks like dayjs could also be a good alternative. |
Update: we are taking another direction than switching to Luxon, please take a look at #5960. |
Thank you guys so much :D |
@hiendv we are working on the Luxon adapter and I guess @kurkle is investigating the date-fns one. I would love these 2 adapters to be ready for 2.8, so any help testing recent changes in master (#5960, #5978) with these adapters would be really appreciated :) |
@simonbrunel I would love to help with the date-fns one too ;) |
While we wait for the much anticipated 2.8 (or 3.0?) release, one quick hack you can do is exclude the moment locales if you use webpack: plugins: [
new webpack.IgnorePlugin(/^\.\/locale$/, /moment$/)
] At least for me (as someone who doesn't use time charts), it cut the moment bundle size by about 73%, saving me roughly 180KB. My apologies if this isn't 100% germane to the discussion, just something I haven't seen on the various moment.js threads, and I thought folks might benefit. |
Hi Team, I had started using react-chartjs-2 for charting needs in my react app. Although i used the suggested npm command to install react-chartjs-2 and chart.js; I am facing a build error of like right after adding import statement from react-chartjs-2. Error is something like : requirejs compile (requirejs task) : Error ENONENT: No such file or directory open '...........temp\moment.js. Then I have done npm install moment --save as well but still error persisted. Any suggestion please. |
@SKGGitHub please keep this on topic and open a new issue for other issues. |
I can confirm that this works. I think it's quite an elegant solution too! |
For WebPack 5, I had to use this:
From webpack docs. |
I'm using just donut chart, no need of moment at all, so removed it completely:
1mb less of dependencies 👍 |
vite (rollup) build: {
rollupOptions: {
external: ['moment'],
},
}, |
Expected Behavior
Chart.js requires moment.js but only uses some of its functions. The moment.js size is too large for production. I want to reduce the bundle size by using a date-time library supporting tree-shaking
Current Behavior
I have a problem with the bundle size of moment.js in production. Moment.js does not support tree-shaking so its size is kinda overwhelming.
Possible Solution
Using date-fns instead of moment.js. Date-fns offers similar APIs and support tree-shaking. There are some comparisons between date-fns and moment.js
Context
Chart.js is the only library requiring moment.js in my application. I did some searches and date-fns seems to be a decent choice.
Environment
The text was updated successfully, but these errors were encountered: