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

[FEATURE] Using fecha in place of moment #3533

Closed
salzhrani opened this issue Oct 31, 2016 · 13 comments
Closed

[FEATURE] Using fecha in place of moment #3533

salzhrani opened this issue Oct 31, 2016 · 13 comments

Comments

@salzhrani
Copy link

While moment is an amazing and robust library, it seems like an overkill. I propose using fecha it is much smaller than moment and can be extended to add missing functionality. If you think it is beneficial I can submit a PR for review.

@etimberg
Copy link
Member

etimberg commented Nov 1, 2016

hmm, I'm a little wary of changing this. What are the advantages of using fecha over moment?

If we are to use this, we first need to move the time scale to be based on the linear scale. ie, we use a parser to transform the dates into unix millisecond timestamps then simply pick good size steps and then convert back to date strings.

@etimberg
Copy link
Member

etimberg commented Nov 1, 2016

@salzhrani I should add that I did try playing around with changing the time scale to be based on the linear scale. Code is in https://github.com/chartjs/Chart.js/tree/time-scale-improvements

CC @chartjs/maintainers to get your thoughts

@panzarino
Copy link
Contributor

This library does seem to be a lot smaller than moment and could resolve some issues that people are having using moment with Chart.js. If we could implement it, it could save some size but that seems to be the only main advantage.

@salzhrani
Copy link
Author

It is mainly for size, some projects have js payload budget and this could help with that.

@simonbrunel
Copy link
Member

@zachpanz88 it could solve issues for people already using moment, but then issues will be for people already using fetcha. So if we switch to another library, we should consider #2906 but also ensure backward compatibility (I don't know if we expose moment specific options / formatting). Ideally, we should provide a way for users to choose whatever time lib they want.

@avlcodemonkey
Copy link

@salzhrani, it's kind of hacked together, but I'm using time scales with fecha. I've got a fork of fecha at https://gitlab.com/avlcodemonkey/fecha that adds in the date manipulation features needed to make it work. And I've got a branch at https://github.com/avlcodemonkey/Chart.js that implements it.

@etimberg
Copy link
Member

I think the time scale might need a rethink. Instead of using the date manipulation features, we should base it on a linear scale and set the division size appropriately. We'd use fecha/moment to parse the dates into millisecond timesteps then the scale would internally use those timesteps before formatting the output back into nice strings

@etimberg
Copy link
Member

Since this issue was opened we've changed the time scale internals to depend less on moment. Right now moment is primarily used for the date parsing and formatting. This may make this issue much easier to implement safely

@benmccann
Copy link
Contributor

It's also been proposed that we use date-fns in #4303. date-fns looks to be much more popular. Is there any reason we should consider fecha instead of date-fns?

@avlcodemonkey
Copy link

To me, it's about library size. Fecha is a very small library that can replace a very large one for chartjs purposes. If it's possible to use a small subset of date-fns via ES6, I'm open to that approach. date-fns is clearly more popular. I would prefer to avoid including the entirety of date-fns though.

@benmccann
Copy link
Contributor

Fecha does not seem like a realistic option. It has no date manipulation APIs. There's a lot of functionality that we currently have that could not reasonably be written with Fecha. E.g. it has no ability to calculate the timestamp for the beginning of an hour, day, year, etc.

@salzhrani
Copy link
Author

When I opened the issue about a year ago, date-fns didn't exist. The motivation was to reduce the lib size and not necessarily advocate fecha.

@benmccann
Copy link
Contributor

benmccann commented Oct 7, 2017

Ok. I'm going to close this issue in favor of tracking it with the date-fns issue. date-fns supports tree shaking, so it's much easier to use just the relevant subset of it that we need compared to 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

6 participants