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

move moment dependencies to date-fns-tz #53

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

Conversation

mcshaz
Copy link

@mcshaz mcshaz commented Jan 11, 2023

I wanted to move the library away from moment and moment based tools in order to decrease bundle size. There are likely significant improvements in processing speed as well.

All tests pass, although I have not worked on documentation as yet pending your thoughts.

If using date-fns-tz (date-functions-timezone), the native Intl API is used with hopefully significant decrease in bundle size. It would have to be polyfilled in browsers without support - as IE11 is now unsupported by Microsoft for over 6 months (and has current usage < 0.6%), this should have little impact. Android will be affected and a good polyfill (which I will including in the documentation if you were interested in merging these changes) is maintained by Format.JS.

The hijri-calendar folder uses a small method in the script to create the hijri-calendar JavaScript object. This will not polyfill well as polyfills only tend to use the gregorian calendar. This should not be a problem as this script it is never run client side.

The new script differed slightly on a few dates produced in - for example the hijri date "3/1/1389 AH" (M/D/YYYY) had corresponded to the 17th May 1969 with the moment-hijri library but is now the 18th May 1969. I have tested these dates on Node and several major browsers and they seem to agree with the new version (18th May 1969). Given the effort ICU JIRA (the engine backing Intl API in most modern browsers) have gone to working with the Saudi ministry, I would suspect the newer implementation produces the more correct result, but I am not able to read Arabic and the canononical source is http://www.ummulqura.org.sa/

Appologies about the multiple commits - these should be squashed.

@F1nnM
Copy link

F1nnM commented Mar 20, 2023

I would like to bump this, is there any possibility that this transition will happen anytime soon?

@mcshaz
Copy link
Author

mcshaz commented Mar 20, 2023

@F1nnM - thanks for your interest. The changes are all working when the 3 libraries (date-holidays, date-holidays-parser and caldate) are used together - I can send you the setup as workspaces so you can build distributable files. It is under half the size and all tests pass (plus a few additional tests). I believe @commenthol is wanting to compare to another pull request to using Luxon instead of date-fns-tz. Ultimately given the inability to tree shake Luxon and the relatively few functions required, date-fns-tz will be a smaller bundle.

I added a few opinionated changes I would also like to hear back about from @commenthol. Primarily this involves moving caldate to a few named exports rather than a single default export. This has the advantage that a) number of utility functions for which code is duplicated in date-holidays-parser have a single source. b) allows an architecture whereby minimal change is required in years to come when the ECMAScript Temporal spec has been rolled out (in consuming this new spec and also removing dependency on any timezone library).

@F1nnM
Copy link

F1nnM commented Mar 21, 2023

Oh, that would be great. Can you attach them here or send them to me under ######?

@mcshaz
Copy link
Author

mcshaz commented Mar 22, 2023

@F1nnM I have put the workspace structure under a repo at https://github.com/mcshaz/date-holidays-workspace/ with the readme giving instructions on which branch to install in which folder

@F1nnM
Copy link

F1nnM commented Mar 24, 2023

It took some tweaking and hacky workarounds, but I got it to work in the end. Thank you!

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.

2 participants