search

commenthol / date-holidays-parser

parser for worldwide holidays
ISC License
46 stars 23 forks source link

Remove moment dependency to reduce bundle sizes #2

Closed OscarBarrett closed 6 years ago

OscarBarrett commented 6 years ago

We've found date-holidays to be a very useful package, with one downside being moment inflating bundle size significantly. This PR removes the moment dependencies (as well as caldate) and replaces it with vanilla JS and date-fns where appropriate.

For our main use case, these dependencies were taking up more than 40% of the size of our vendor bundle (180kB for moment-timezone and 56kB for moment, for a combined 236kB).

date-fns is highly modularised. The usage here is for 42kB for us (it may be less as some of our entrypoints might be pulling other date-fns modules not used in this PR into our vendor bundle).

RE the implementation of toTimezone in internal/utils.js and browser support for the timeZone setting of Date.prototype.toLocaleString:

There is a polyfill here which could be used when necessary. https://github.com/yahoo/date-time-format-timezone

commenthol commented 6 years ago

Thanks for your intention to contribute and that you find the package useful. I'm sorry that I can't accept this PR in this form as it draws the lib unusable with regards to correct times and timezones. date-fns as well as the suggested links on timezones don't match the needs of this lib. Your suggestions address date display only but not the calculations necessary to shift a date with regards to its timezone. This is the reason for the failing tests.

OscarBarrett commented 6 years ago

Understood - it's a pity that there isn't a better alternative for handling timezones. I have looked into addressing the failing tests but certain cases are difficult to handle with toLocaleString. Our current use cases admittedly don't require supporting multiple timezones.

If I find a better solution to handle timezones then I'll submit a new pr. :+1: