Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Date library dependency in v8 #980

Closed
gpbl opened this issue Jan 27, 2020 · 15 comments
Closed

Date library dependency in v8 #980

gpbl opened this issue Jan 27, 2020 · 15 comments
Labels
help wanted Extra attention is needed

Comments

@gpbl
Copy link
Owner

gpbl commented Jan 27, 2020

DayPicker relies on some date manipulation functions to work. Until v8 I've been maintaining a custom set of utilities to keep the bundle size small.

I underestimated the difficulties working with dates and other abstractions required to support localization, so I decided to switch to date-fns.

This choice is raising some valid concerns in the community, which I would like to collect here.

References:

@gpbl gpbl mentioned this issue Jan 27, 2020
@gpbl gpbl added Priority: Medium Proposal Request for comments labels Jan 27, 2020
@bengry
Copy link

bengry commented Jan 27, 2020

@gpbl do you know what functions v8 will need? I mean the logical operators it needs (e.g. adding N days to a given day). This list can then be mapped to library functions (e.g. addDays in the aforementioned example for date-fns or moment(date).add(N, 'days') for moment), then we can see how much of a bundle size increase we're getting from date-fns (or any other tree-shakeable library), given that only these functions are included in the bundle. My guess is that it'll be pretty small when using a tree-shakeable library.

Other options I can think of:

  • Decide and use some date library with its own implementation for Date (e.g. moment, Luxon, dayjs), then expose this as the public API from react-day-picker.
    Personally I don't like this, since it ties down apps to a specific date library, or requires them to include a library that they don't need other than for react-day-picker and convert between the chosen library's type and their own usage in the rest of the app.
  • Use date-io, as mentioned here by @ denisborovikov.
    Again, personally I don't think this is a good direction, as I explained here and here. Basically, while it doesn't limit the app's to any given library, and now react-day-picker can work with anything that has an adapter for date-io, it removes tree-shakeability, and as mentioned above - I think the bundle size increase when using date-fns will be pretty small.

@gpbl
Copy link
Owner Author

gpbl commented Jan 27, 2020

I think the main issue here are localization-based calculations (eg first day of the week). I’d go with date-fns and study the problem with a lower priority. There are many options out there! I really want to push date-fns, it is good for the ecosystem.

Different approaches can be tried after the release. But i’d better discuss real use cases like yours first...

@bengry
Copy link

bengry commented Jan 27, 2020

What do you mean by "real use cases" in this context?

I also think date-fns is a safe and good choice, especially since it uses native Dates. The only concern is that if you drop it later then it'll be a breaking change. Not too big probably, but one nonetheless.

@dmtrKovalenko
Copy link

dmtrKovalenko commented Feb 4, 2020

Interestingly, I am the author of date-io.
Just today we discussed with some other libraries authors about splitting date-io into tree-shakable functions for calculations and a class for formatting and everything that is related to the localization, like days of weeks month and so on.

The main problem of date-fns is that if you will include just 2 functions format and parse. Which are required in case of any datepicker you will something around 30kb, just like here :)
All the other date-fns functions are much-much smaller
Screenshot 2020-01-22 at 22 24 21

P.S. I am not trying to force you to use date-io :D

@bengry
Copy link

bengry commented Feb 5, 2020

@dmtrKovalenko in the case where date-io is tree-shakeable, it would make more sense to use it in this project, but you're still left with the issue of an additional dependency "for nothing" (in the case eyes of the library itself). Also, I'd like to mention that react-date-picker does not have to include the format and parse functions of date-fns. At the end of the day, the default display in the Calendar are pretty "raw", and formatting can be done by the application itself currently when using the renderDay prop, and the same goes for other ones like it. This way, locale is a non-issue for the library until faced with a non-default rendering, which even then may be a non-issue if you're using the browser's locale, or have support for the Intl API in your supported browsers (or polyfilled it anyway).

Note that I haven't referenced the <DayPickerInput /> component though, as it may need these functions, or functions like them. The short answer there is that even in that case you could just pass them as [optional] props. The longer answer is that many use-cases of this library don't even use <DatPickerInput /> in the first place, and it was supposed to be removed in v8 originally AFAIK.

@dmtrKovalenko
Copy link

The main problem is how we can share them to be replaceable by user and get rid of object with functions. 🤷‍♂️ I don’t know

@bengry
Copy link

bengry commented Feb 5, 2020

You can always supply them via props, I don't see anything wrong with that.

<DayPicker renderDay={date => <div>{date.getDate()}</div>} ...	/>

If you want to use the default renderer you could implement a pattern similar to what office-ui-fabric-react uses:

<DayPicker renderDay={(date, defaultRender) => defaultRender(date.getDate())} ...	/>

where defaultRender accepts a ReactChild (essentially JSX | string | number) to render using the default render method that would've been used directly if renderDay wouldn't have been supplied. This lets you format the Date however you want, including localization.

@dmtrKovalenko I don't see any reason for an object with functions here, but I'm happy to learn why you think one is needed in the first place :).

@dmtrKovalenko
Copy link

dmtrKovalenko commented Feb 5, 2020

The main idea of the date-io is that user can import the adapter, one time pass it as a prop and any component will work out of the box

import DateFnsAdapter from 'yourLib/date-fns'

<DatePicker adapter={new DateFnsAdapter({ locale: ruLocale })} /> 
// it doesn't matter this can be a fabric function 
<DatePicker adapter={makeDateFnsAdapter({ locale: ruLocale })} /> 

But how we can populate it as a tree shakable function? Only one way I can imagine -- each consumer of date-io will need to reexport needed functions as objects

import { addDays, format, parse } from '@date-io/date-fns'

export default { addDays, format, parse } 

And then in the code use them exactly like date-fns functions. But I don't think that it worth it, personally I don't like date-fns in the javascript environment :) (In ReasonML - it's the best)

So I think that the best way to handle it would be to split out the date-io into several independent classes or functions for Formatting, Parsing and Calculations.

@bengry
Copy link

bengry commented Feb 5, 2020

You can always utilize React's Context in this case:

const dateTimeFormat = new Intl.DateTimeFormat();
<DayPickerFormattingProvider format={dateTimeFormat.format)>
  <DayPicker /> {/* this can be nested anywhere inside the Provider, so you could even wrap your whole App with a Provider and forget about it if you want to */}
</DayPickerFormattingProvider>

I personally don't like the usage of Context for this very much. I'd much rather have a <DayPicker />-wrapper component within the app's project that just gives that adapter/formatter/whatever else to the library's <DayPicker /> and then use only that component from that point forward. I think this is what a lot of other apps are doing anyway, to customize the library's component in the way they need it, in a way that will then be consistent throughout their app.

So I still don't see a reason for using date-io in this case.

@dmtrKovalenko
Copy link

dmtrKovalenko commented Feb 5, 2020

In case I am using Luxon for formatting and timezone management - I don't want to see 44kb of date-fns in my bundle. I`d better include 1.6kb of date-io adapter.
Moreover, if my application is multi-locale this size will be only bigger

@bengry
Copy link

bengry commented Feb 5, 2020

@dmtrKovalenko how is passing in props/using React Context not solving this issue? This way there's either no usage of date-fns in react-day-picker, or a very minimal one (just for addDays and the like, no formatting or anything like that).

@dmtrKovalenko
Copy link

I think we are talking about different things :)
If you want we can continue discussion in twitter or somewhere.
Let’s not generate white noise in the issue

@gpbl gpbl added this to the v8.0.0 milestone Apr 13, 2020
@gpbl gpbl removed this from the v8.0.0-beta milestone Apr 27, 2020
@gpbl gpbl added help wanted Extra attention is needed and removed Priority: Medium Proposal Request for comments labels Apr 27, 2020
@gpbl gpbl changed the title (v8): Date library dependency discussion Date library dependency in v8 Apr 27, 2020
@gpbl gpbl pinned this issue Apr 27, 2020
@hornetDC
Copy link

hornetDC commented Oct 15, 2020

Is it possible to somehow use v8 without date-fns? We are using dayjs in our project, v7's formatDate and parseDate props are very convenient. I don't want to bring extra dependency.

@dmtrKovalenko
Copy link

@vvav3 Only if maintainers will choose the date-library-independent way of date library.

P.S date-io can be an option :D

@scalisi
Copy link

scalisi commented Oct 15, 2020

We are using dayjs in our project, v7's formatDate and parseDate props are very convenient. I don't want to bring extra dependency.

We also use Day.js with React Day Picker, and we chose them both in part because of their small size and lack of dependencies.

@gpbl gpbl closed this as completed Feb 10, 2021
Repository owner locked and limited conversation to collaborators Feb 10, 2021
@gpbl gpbl unpinned this issue Feb 10, 2021
@gpbl gpbl moved this to Done in DayPicker Plans Aug 26, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
help wanted Extra attention is needed
Projects
Status: Done
Development

No branches or pull requests

5 participants