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

Migrate to date-fns or luxon? #61

Closed
dmtrKovalenko opened this issue Nov 19, 2017 · 28 comments
Closed

Migrate to date-fns or luxon? #61

dmtrKovalenko opened this issue Nov 19, 2017 · 28 comments
Assignees
Labels

Comments

@dmtrKovalenko
Copy link
Member

We have moved moment to the peer deps, and now I am considering about replacing moment at all with date-fns. 😎

So this just issue to consider if there are many people that using moment with this project?
How many people using date-fns?

I am just need to realize which way would be more convenient. ❓

@sakulstra
Copy link
Contributor

sakulstra commented Nov 19, 2017

We're using moment basically for timezone calculations with moment-timezone atm., still i'd like to completely migrate away from moment due to size implications. Besides missing time-zone support there are also a lot less languages supported in date-fns as in moment (https://github.com/moment/moment/tree/develop/locale vs https://github.com/date-fns/date-fns/blob/master/docs/i18n.md).

My take on this would be to wait till date-fns 2 reaches at least beta and then look into refactoring. A lot of things should be easy to refactor - the one thing which may require breaking changes is then handling of locales though. If i get it correctly date-fns is stateless so you have to pass the locale to every function which is locale aware.
In order to make migration easier once we switch to date-fns we could eventually introduce a locale prop, even know when still using moment.

@mduijn
Copy link

mduijn commented Nov 20, 2017

At its current size moment is an all but dead library.
We are currently using moment but will make the switch soon.
The timezones are currently holding us back (date-fns/date-fns#180)

@dmtrKovalenko dmtrKovalenko changed the title Migrate to date-fns? Migrate to date-fns or luxon? Nov 27, 2017
@dmtrKovalenko
Copy link
Member Author

@sakulstra @mduijn What did you think about luxon?

@sakulstra
Copy link
Contributor

not sure, tbh. I heard of it the first time ~5days ago and didn't use/test it at all.
I generally like their approach to use Intl features provided by the browser - but to me it seems like this is not sufficient in most cases so it complicates our lives as long as polyfills and browsersupport aren't there: https://moment.github.io/luxon/docs/manual/why.html

Eventually it would also make sense to reevaluate if we could just get away with the native Date and rip out the 3th party date libraries. Then users could just decide themselves if/what library they use. Personally i would just let it like it is for now and rethink later in a few months.

@ingro
Copy link

ingro commented Nov 29, 2017

Honestly I'm using only date-fns for newer projects, but luxon could also be a worthy candidate, moment is just too big... But I'm with @sakulstra on this, the best way will be to work with the native Date by default and let the user use whatever they want for their needs of localization/parsing/format dates, should they needs to.

Recently react-day-picker did this by removing direct moment dependency and providing some default functions that still use moment but are entirely optional.

@cherniavskii
Copy link
Member

+1 for migrating off of moment, it's too heavy :/

@jwwisgerhof
Copy link

+1 for migrating to date-fns. This is the main reason we do not use this date picker right now

@dmtrKovalenko
Copy link
Member Author

@cherniavskii @alitaheri
I have an idea how we can support that. We can use utils, that was created for support jalali calendar system to fully replace dependency of moment in the project.
We can create material-ui-pickers-datefns-utils, material-ui-pickers-luxon-utils, material-ui-pickers-vanilla-utils to add ability choose the library. What do you think about that?

@alitaheri
Copy link
Member

@dmtrKovalenko That sounds great 👍 👍 Just keep in mind, not everything has been moved to utils, which is easy to solve 😁

Also, we should come up with a solution for interoperability. I think we should accept only ISO strings to make sure every implementation can know what to do.

@dmtrKovalenko
Copy link
Member Author

@alitaheri Maybe there is a way to allow passing any, to add ability pass library-specific date object?

@alitaheri
Copy link
Member

@dmtrKovalenko Oh... right, you're right 👍 👍

@cherniavskii
Copy link
Member

@dmtrKovalenko good idea!
What about default behavior? Or utils prop will be required?

@dmtrKovalenko
Copy link
Member Author

@cherniavskii I think it should be required, we can do utils with vanilla date by default, but it can take some unused code, if somebody doesnt using tree shaking, I think it should throw some error if no utils passed.

@cherniavskii
Copy link
Member

So creating a single picker will require to import appropriate utils and pass them to picker?

@dmtrKovalenko
Copy link
Member Author

Why not, we can also pass the vanilla utils by default, I think everybody using shared components to use pickers, so it would not be a problem? Or would be?
So we need to decide - add some additional maybe unnecessary code (with vanilla utils) or force to pass utils each time using pickers.

@alitaheri
Copy link
Member

@dmtrKovalenko I would go with forcing utils to be specified. It's not a big deal. and can some a lot of people a lot of hassle, reduce the lib size and a lot more.

We're in beta, so breaking changes are expected anyway. 👍 For externalizing utils and forcing others to require the utils of their choice.

@cherniavskii
Copy link
Member

cherniavskii commented Dec 23, 2017

@dmtrKovalenko I just wanted to make sure I understood your idea right ;)
But
Maybe there's a way to init utils once - like moment.locale() works?

@jephtta
Copy link

jephtta commented Dec 29, 2017

+1 for abandoning moment. Native date is better in the long run, but if a library has to be used, then definitely date-fns. We use it in place of moment wherever we can.

@danieljuhl
Copy link
Contributor

danieljuhl commented Jan 11, 2018

I would vote for Intl.js to be as close to native as possible - if a dep. should be used, then one as small as posible. This was also the case for the built-in datepicker in [email protected].

@TheRusskiy
Copy link

count me in for date-fns

@syntheticsgr
Copy link

+1 for date-fns

@EliasJorgensen
Copy link

Would really love to see date-fns!
Moment is waaaaay too huge, i want to keep it out of my build if at all possible.

@KeKs0r
Copy link

KeKs0r commented Feb 5, 2018

I am using date-fns as well in my project. This picker is the only reason to add moment in my project. Would love to move away from that dependency.

@dmytro-ulianov
Copy link

+1 for date-fns

@dmtrKovalenko
Copy link
Member Author

@ALL guys, we have added that in v1.0.0-rc.1

@Ponjimon
Copy link
Contributor

Ponjimon commented Apr 10, 2018

This has been closed, but I want to ask nevertheless.
Is there still interest in support for Luxon?
If so, I made a fork of this repo and built support for it including tests.

https://github.com/lookapanda/material-ui-pickers/blob/luxon/lib/src/utils/luxon-utils.js
https://github.com/lookapanda/material-ui-pickers/blob/luxon/lib/__tests__/utils/luxon-utils.test.js

If there is interest, I could do a PR.

@cherniavskii
Copy link
Member

@lookapanda wow, that's cool, I think we can add Luxon support :)
@dmtrKovalenko what do you think?

@dmtrKovalenko
Copy link
Member Author

@lookapanda Way to go!
Thats just awesome, I have think about adding support in the next release. Please make a PR 😊

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

No branches or pull requests