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

Remove moment dependency to reduce bundle sizes #2

Closed

Conversation

OscarBarrett
Copy link

@OscarBarrett OscarBarrett commented Mar 20, 2018

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:

  • ✅ Chrome: Supported since 24
  • ✅ Firefox: Supported since 52
  • ✅ Safari 10+: Supported
  • ❓Edge: ?
  • ❌ IE11: Only accepts 'UTC' and complains for anything else
  • ❌ IE10: Ignored completely (uses local system time)

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

@commenthol
Copy link
Owner

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
Copy link
Author

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. 👍

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

Successfully merging this pull request may close these issues.

None yet

2 participants