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

Discussion: Optional moment.js #1059

Conversation

prayerslayer
Copy link
Contributor

@prayerslayer prayerslayer commented Oct 7, 2017

I got react-datepicker to run with date-fns behind, but it has some implications. This PR is not intended to be merged as-is, but should serve as a starting point for discussion on how to proceed.

Quick refresher: date-fns is a project that provides a collection of date-related functions. In contrast to momentjs it deals with JS Dates directly. However it's not as powerful as moment.

The first thing is timezones. Moment has superior timezone support (moment().utc().utcOffset(6) and such), date-fns none at all, because JS Dates are incapable of truly representing another timezone than what your system is currently in. Of course you can set the time as if you were in UTC, but date.toString() will always show the original timezone. I made amateurish efforts to work around this, but came to the conclusion that the utcOffset just shouldn't do anything when datepicker is backed by date-fns. Safety first.

The second thing is localization. Moment ships with lots of locales by default, date-fns does not. I worked around this by making users provide the locale (strings may or may not work with date-fns, depending on which we include by default):

<Datepicker locale={require('date-fns/locale/fr')} />

A third, but rathor minor issue is that Moment ships with more date formats than date-fns. E.g. L doesn't work without some adjusting code in date_utils/date-fns, which I didn't do so far. Not sure if that should be an issue of react-datepicker in the first place.

However I feel that there's merit in cleaning this PR up, revisit some tests and possibly distributing a separate build for date-fns. Please look over it and let's discuss what can be improved.

@@ -0,0 +1,7 @@
if (process.env.DATE_BACKEND === 'date-fns') {
Copy link
Contributor Author

@prayerslayer prayerslayer Oct 7, 2017

Choose a reason for hiding this comment

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

To be fair, I have not checked if that works as intended with webpack. What you'd want is that the code in the other branch is not added to the bundle.

@prayerslayer
Copy link
Contributor Author

Hey, did you have the chance to glance over this? I know it's much to digest, but I'd like to know if there's intent on your side to ship this, or if you think it's too much effort for too little gain :)

@martijnrusschen
Copy link
Member

Interesting idea. I think we should drop anything that's not supported by date-fns by default. So lets see if we can get a version ready that just does the basics with Date-fns without breaking the moment.js implementation. The date-fns implementation will be less feature rich but we can document the differences.

@martijnrusschen
Copy link
Member

Closing in favor of #1527.

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.

2 participants