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

Do not require moment.js #4303

Closed
hiendv opened this issue May 29, 2017 · 56 comments
Closed

Do not require moment.js #4303

hiendv opened this issue May 29, 2017 · 56 comments
Assignees
Milestone

Comments

@hiendv
Copy link

hiendv commented May 29, 2017

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

screenshot from 2017-05-29 17-55-24
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

  • Chart.js version: 2.5.0
  • Browser name and version: Chrome v59.0.3071.29
@benmccann
Copy link
Contributor

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

@simonbrunel
Copy link
Member

@benmccann not sure we need a new ticket, I'm already on reorganizing the - weird - require() strategy in order to migrate to ES6 modules and finally switch to Rollup instead of Browserify. Tree shaking would be great for external dependencies, but we don't want to get rid of our own methods that are not referenced in the main repo (e.g. deprecations or methods used by external modules).

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 Chart.bundle.js?

@benmccann
Copy link
Contributor

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.

@simonbrunel
Copy link
Member

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 Chart(.min)?.js could detect if a compatible date/time lib is already loaded by the user and use it. But also allow users to write custom adapter for their preferred date/time lib.

@etimberg
Copy link
Member

etimberg commented Jul 4, 2017

Funny that #4461 got filed about this very topic today

@hiendv
Copy link
Author

hiendv commented Jul 5, 2017

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.

@benmccann
Copy link
Contributor

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.

@hiendv
Copy link
Author

hiendv commented Jul 5, 2017

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.
If users want to manipulate the labels, they should use their own library.

@simonbrunel
Copy link
Member

simonbrunel commented Jul 5, 2017

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 date-fns :)

Edit: though, I totally agree that moment.js may not be the best dependency for Chart.js.

@simonbrunel
Copy link
Member

For example: #3533

@hiendv
Copy link
Author

hiendv commented Jul 5, 2017

I understand the benefit of adapters here. There is a small difference between injecting a date/time library and my proposed approach.
If we offer adapters, at some point in the future, chart.js will end up with a dozen of adapters and the problem of bundle size may not be resolved as people will keep injecting the whole date/time library into chart.js.
So instead of a library, we use standalone functions, e.g. format(), parse() (which are provided by date-fns or any library). We don't have to care about those libraries anymore.

@hiendv
Copy link
Author

hiendv commented Jul 5, 2017

So why do I keep insisting on date-fns? Because of tree-shaking / function separation and it has the similar APIs with moment.js (the current date/time lib of chart.js)

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

@simonbrunel
Copy link
Member

date-fns looks great indeed, let's have a look at it after we switch to ES6 module. We will also need to make sure it doesn't break external code.

@hiendv
Copy link
Author

hiendv commented Jul 6, 2017

Hope it arrives soon. I will have my own fork of chart.js-no-moment.js for now.

@simonbrunel
Copy link
Member

Sounds good, can you share your repo/branch once ready, I'm curious to see what really means switching to another date/time lib :)

@hiendv
Copy link
Author

hiendv commented Jul 6, 2017

Yep. Looking forward to your feedbacks when it's done.

@merlinstardust
Copy link

@hiendv can you publish your fork with no moment? Or comment on how to do that?

@merlinstardust
Copy link

To help with the switch, I've created a gist of every instance of moment within Chart.js

https://gist.github.com/merlinpatt/9925bbac94b0c91f1430c2de1569bb26

@beeing
Copy link

beeing commented Oct 13, 2017

Just wondering whether this is still needed as momentjs is now moving to ES6

@benmccann
Copy link
Contributor

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

@benmccann benmccann self-assigned this Jun 4, 2018
@benmccann
Copy link
Contributor

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

praveen-garg added a commit to foglamp/foglamp-gui that referenced this issue Sep 6, 2018
…merge of this will happen; once chartjs/Chart.js#4303 has plugin based solution; let's drive this again
@geirsagberg
Copy link

@dmtrKovalenko (creator of material-ui-pickers) has recently split out his date abstraction library: https://github.com/dmtrKovalenko/date-io.
Maybe this is something to consider?

@benmccann
Copy link
Contributor

@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

@ciaoben
Copy link

ciaoben commented Nov 20, 2018

Sorry but It is not clear to me, do exist a way to use chart without moment?

@geirsagberg
Copy link

@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.

@lsn793
Copy link

lsn793 commented Dec 5, 2018

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 ?

@mojoaxel
Copy link
Contributor

mojoaxel commented Jan 9, 2019

It looks like dayjs could also be a good alternative.

@simonbrunel
Copy link
Member

Update: we are taking another direction than switching to Luxon, please take a look at #5960.

@hiendv
Copy link
Author

hiendv commented Jan 30, 2019

Thank you guys so much :D

@simonbrunel
Copy link
Member

@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 :)

@hiendv
Copy link
Author

hiendv commented Jan 31, 2019

@simonbrunel I would love to help with the date-fns one too ;)

@april
Copy link

april commented Feb 6, 2019

@ciaoben, @lsn793:

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.

@SKGGitHub
Copy link

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
Copy link

image

HI All,

Getting above error while importing from react-chartjs-2. Any suggestion please on how to solve it.

@mesqueeb
Copy link

@SKGGitHub please keep this on topic and open a new issue for other issues.

@egeersoz
Copy link

egeersoz commented Apr 2, 2020

@ciaoben, @lsn793:

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.

I can confirm that this works. I think it's quite an elegant solution too!

@chenilim
Copy link

chenilim commented Jan 9, 2021

For WebPack 5, I had to use this:

plugins: [
  new webpack.IgnorePlugin({
    resourceRegExp: /^\.\/locale$/,
    contextRegExp: /moment$/
  })
]

From webpack docs.

@juanbrujo
Copy link

I'm using just donut chart, no need of moment at all, so removed it completely:

new webpack.IgnorePlugin({
   resourceRegExp: /moment$/,
}),

1mb less of dependencies 👍

@sa02045
Copy link

sa02045 commented Dec 7, 2023

vite (rollup)

build: {
      rollupOptions: {
       external: ['moment'],
    },
},

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