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 dependency on Moment library in exchange for date-fns-tz #8

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mcshaz
Copy link

@mcshaz mcshaz commented Jan 11, 2023

As per the pull request in date-holidays-parser this is in order to reduce bundle size and also keep dependency to an actively maintained library, with the added bonus of faster processing of dates.

@commenthol
Copy link
Owner

Hi @mcshaz,
Thanks for your efforts.
We currently have another PR #6 which tries to replace moment-timezone using luxon.
This PR currently is on hold due to errors when calculating equinox or solstice dates.
Can you please spend some words why to abandon the approach with luxon and use date-fns-tz instead?
It would also be nice to get some data on how much the bundle size is being reduced using your approach.
I am asking, as IMHO replacing core libs usually comes with side effects.

Tests added to describe the problem with the date-fns-tz library when the zoned time is set in a daylight savings jump forward, which therefore doesn't exist. A temporary workaround is added but only for Iran (the only country which changed to DST at their local midnight).
@mcshaz
Copy link
Author

mcshaz commented Jan 12, 2023

Thanks for the questions @commenthol. In answer:

I have set up a workspace (calDate, date-holidays-parser and date-holidays) and everything works in all 3 libs (pnpm -r all goes to completion with no errors). the bundle size with a few changes I have is (unzipped) 566 kb (from 1415 kb) although this also includes removing the async polyfill (included with @babel/polyfill) in exchange for core-JS@3 & babel preset-env (IE 11 support removed).

My older version of NodeJS initially struggled with tests of Fiji, as it recently (and with little rollout period) changed its daylight savings rules, but this was fixed with a Node upgrade.

The reason for picking date-fns-tz were based on the table https://github.com/you-dont-need/You-Dont-Need-Momentjs#brief-comparison. Luxon does not support tree shaking and therefore is 75 kb for the only 2 functions we need (converting zoned time to UTC and vice versa). js-Joda would probably also have met requirements, but it has a much smaller user base. I also read https://medium.com/swlh/best-moment-js-alternatives-5dfa6861a1eb and https://www.skypack.dev/blog/2021/02/the-best-javascript-date-libraries/ - both informative articles, but again 95% of the functionality discussed is not used by the commenthol set of libraries.

note there is a problem with date-fns-tz when the zoned date is set to a non-existant time (as daylight saving jumps the clock forward from 2 to 3 am) the issue is here - marnusw/date-fns-tz#222 but when reviewing the pull request you will see a bit of a hack to overcome this problem for Iran, which no longer has daylight saving, but when it did, moved time forward on midnight. This led to the target zoned date/time being non existant.

As per the comments in the pull request code- the Temporal proposal is stage 3 and will likely replace all of this eventually.

@mcshaz
Copy link
Author

mcshaz commented Jan 12, 2023

@commenthol - I had a couple of thoughts overnight:

  1. the size of the library will be much smaller, but I think I must have overestimated the final size advantage in the post above - it just seems too significant a size reduction. I suspect I have set babel/webpack to analyse and apply polyfills to date-holidays, but not date-holidays-parser and caldate.
  2. Given the functionality of converting to and from zoned/local/UTC dates will eventually be taken over by the ES Temporal spec (once final approval and rollout has occured by the EcmaScript committees), it would actually make sense to have caldate export the 2 zoned time conversion functions, which are subsequently consumed by the tests and non-gregorian calendars in date-holidays-parser. That way you can easily plug in Luxon or other date library, and eventually Temporal. Personally I am not a fan of mixed named and default exports, so I would suggest moving caldate to all named exports if you like this idea.

Edit
I found this from the rollup docs at https://rollupjs.org/guide/en/#default-export, so it is not just me!

It is bad practice to mix default and named exports in the same module, though it is allowed by the specification.

The other option would be to have a subdirectory such as named-exports to export the named CalDate class as well as the zoned time function, as this would not be a breaking change. I am unsure how many repositories depend on caldate other than date-holidays-parser, and how important it is to avoid breaking changes.

Thoughs?

@mcshaz
Copy link
Author

mcshaz commented Jan 15, 2023

I made another branch to explore changes. in this branch there are a few slightly opinionated changes (e.g. I changed to typescript), so I am not putting a PR in as yet, but:

  • final bundle size of date-holidays umd.min.js including polyfills and the bundled timezone functions is 602kb.
  • I added a number more tests which I believe will make it far less likely this library passes all tests but breaks date-holidays-parser
  • albeit a bit of a hack, date-fns-tz can 'cascade' the division into commonJS and ESM - that is the code base imports from date-fns-ts/esm library, but by adding the script "replace-in-files --string 'date-fns-tz/esm/' --replacement 'date-fns-tz/' --no-glob lib/index.cjs the cjs transpile requires the cjs date-fns-tz library instead. I'll keep looking into a cleaner method. Admittedly currently this is not a problem in Luxon as they use the same package.json ".": { import: "..path to esm..", require: "...path to cjs..." } pattern as this library does.
  • As per previous comment, re-exports the thinly wrapped "to and from timezone" functions. By having a single source which date-holidays-parser refers to, it will make transferring to another library trivial (in particular the Temporal spec once the final proposal is accepted and EcmaScript engines have native support). It also allows a single source to fix a quirk when the local timezone doesn't exist due to daylight savings jump forward (which effects Iran, which used to shift at midnight and had a holiday on the daylight savings change over).

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