[HOLD for payment 2023-11-22] [Feature request] Replace the moment library with some lightweight date utility library

[mountiny]

 Problem

Coming from this Slack thread and Callstack proposal.

The moment library is a powerful library but has performance disadvantages that comes from its OOP design. This design makes it fail working with tree-shaking, so it’s bundle size it’s huge (especially with internalisation or timezone support enabled) - this impacts mostly the web version of Expensify app.

Moreover, the moment library is now a “legacy project in maintenance mode” so it is not actively developed anymore.

More about above here: https://momentjs.com/docs/#/-project-status/

Solution

There are some other powerful yet lightweight date libraries like day.js (the most lightweight) or date-fns (supports tree-shaking) that could successfully replace the moment library in our app. My personal preference is date-fns because of its support for tree-shaking and wider popularity among community, but using day.js may be easier from the Expensify codebase perspective because of its API that is quite similar to the moment one (e.g. parsing date strings using constructor or day/month/year getters etc. - please check the link I’ve posted below for more details).

Context/Examples/Screenshots/Notes: More about the subject, containing the comparison of the most popular moment replacements can be found here: https://github.com/you-dont-need/You-Dont-Need-Momentjs/blob/master/README.md

• [ ] Add an eslint rule once we are done which will prohibit adding the library back

[melvin-bot[bot]]

Triggered auto assignment to @anmurali (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.

[mountiny]

No need to action here now Anu, Callstack will work on this

[burczu]

Hey! It's Bartek from Callstack - expert contributor group - I'll be working on this one.

[mountiny]

Thanks, please, first post a plan of how to exectute this migration before you go ahead.

[mountiny]

@burczu would you be bale to share a plan for this project and some ETA, thanks!

[burczu]

Hey @mountiny! Sorry for being silent in this issue - I'm 100% involved in the Secure Logins issue (migration to use accountID's instead of emails in personal details) right now and had no time for anything else. I'll do my best to share some plan for this issue today or on Monday.

[burczu]

Ok, @mountiny my plan is simple actually:

1. Identify all the places where we use the moment library
2. Check if that places are covered by unit tests
3. Cover all the places that are not covered with unit tests to, later, make sure that we didn't break anything while doing the refactor
4. Replace all the usages of the moment lib with date-fns library (I think it is the best choice because of its popularity in the community)
5. Check if tests still passing

In terms of ETA, it depends on how much tests I will need to add as it's the most time consuming task in this plan. I would say more once I check the current coverage.

What do you think?

[mountiny]

@burczu thanks for the update, I like including the unit tests, the plan sounds great to me.

[hannojg]

Interested in seeing this one as well, as I am looking at the minute at performance issues with momentjs code (which I'd like to replace by date-fns).

[mountiny]

This will continue after the secure logins bugs are fixed

[burczu]

I've been working on the first step of my plan today, and identified all the places where we use the moment library in the app. Here are my findings:

Places where moment is used and how:

1. components/NewDatePicker/index.js
   1. formatting
2. components/AutoUpdateTime.js
   1. moment used indirectly - the DateUtils utility is used
3. components/CalendarPicker/index.js
   1. extensive usage of the moment library (formatting, setting etc.) - this file needs to be 100% covered by tests
4. libs/DateUtils.js
   1. extensive usage of the moment library - this file needs to be 100% covered by tests
   2. methods that returns moment object and used outside:
      1. getLocalMomentFromDateTime method, returns moment object, used in:
         1. components/AutoUpdateTime.js - used to get timezone name (string)
         2. components/ReportActionItem/ChronosOOOListAction.js - formating
         3. components/pages/home/report/ParticipantLocalTime.js - formatting
5. libs/EmojiUtils.js
   1. only used for getting timestamp
6. libs/Navigation/AppNavigator/AuthScreens.js
   1. only used for getting current timezone
7. libs/ReportActionsUtils.js
   1. only used for getting local time from given date
8. libs/ValidationUtils.js
   1. extensive usage of the moment library - this file needs to be 100% covered by tests
   2. no methods that returns moment object and used outside
9. libs/actions/App.js
   1. only used for getting current timezone
10. libs/actions/User.js
    1. only used to compare two dates
11. libs/fileDownload/FileUtils.js
    1. formatting
12. pages/EnablePayments/AdditonalDetailsStep.js
    1. only used for date manipulation (substract)
13. pages/ReimbursementAccount/IdentityForm.js
    1. only used for date manipulation (substract)
14. pages/settings/Profile/PersonalDetails/DateOfBirthPage.js
    1. only used for date manipulation (substract and getting a year)
15. pages/settings/Profile/TimezoneInitialPage.js
    1. only used for getting current timezone
16. pages/settings/Profile/TimezoneSelectPage.js
    1. only used for getting current timezone
17. pages/wallet/WalletStatementPage.js
    1. only used for date manipulation (format and getting year, month etc.)

Tomorrow, I think I'll start working on writing unit tests for places I've marked in bold (CalendarPicker component, DateUtils and ValidationUtils) - I think these files should be 100% covered.

[melvin-bot[bot]]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

[mountiny]

Nice, CalendarPicker will be refactored soon not sure if it would be important, but something to keep an eye on

[burczu]

Not overdue: I made some progress in writing tests I've planned, but I had to switch to another issue for now. I think I'll be able to get back to this one next week.

[anmurali]

@burczu - do you have an update?

[burczu]

@anmurali I'm sorry, I had no time for this issue last week - hopefully this week will look better

[burczu]

Update: I've done some progress today, but I'll be OOO starting on Monday till July 25th - once I'll be back, I think I'll have the time to 100% focus on this issue because my other issues will be taken over by other Callstackers.

[mountiny]

Bartek has been ooo this week

[waterim]

Update: Covered main files with tests, updated DateUtils and ValidationUtils with date-fns.

[mountiny]

@waterim Thats great! Would you be able to open a draft PR? it would also be better to dived this up to couple of smaller PRs to avoid having a large PR which will be super hard to review.

[waterim]

@mountiny Sure! Will create a PR after fixing timezone issues I have now after switching from moment. I think 1-2 hours

[waterim]

@mountiny Sorry about the delay, got stuck in one test with rewriting from moment()

[waterim]

@mountiny Here is adding a draft PR Still some moment in tests and it will be deleted asap, but utils were rewrote fully. I have one question regarding these "a few seconds ago" from moment default moment(date).fromNow() and "less than a minute ago" from date-fns default formatDistanceToNow(new Date(date), {addSuffix: true}); Question is: Do I need to adjust date-fns to return exactly the same messages as a default function from moment did?

[mountiny]

@waterim Could we do that in some config or it would require a patch? if we can do this in some config, than yes, but I dont thing this is work making a patch

[waterim]

@mountiny We can add some custom function instead of date-fns formatDistanceToNow to achieve exact same strings as moment did. But Im not sure that the difference in a default behaviour is crucial, like "a minute ago" and "1 minute ago"

Date-fns is not supporting some custom configs for formatDistanceToNow function unfortunately. All it has: addSuffix, includeSeconds and locale

[mountiny]

@waterim where is this used in the app?

[waterim]

@mountiny Just here And after passing to LocaleContext.Provider

[waterim]

Hello @mountiny! My upate: removed fully moment from utils functions and tests, resolved all comments on my PR. Now after I started testing the app I found that there are a lot of places in the code where we are using Moment Object functions which on top of the utils functions. Something like this: currentUserLocalTime = DateUtils.getLocalDateFromDatetime(props.preferredLocale, null, props.timezone.selected) And after we are using: currentUserLocalTime.zoneAbbr() where zoneAbbr() is a Moment function. Now Im going file by file, line by line to change all these Moment Object functions to date-fns functions.

[mountiny]

@waterim Could we make sure we can split this up to smaller PRs so before we continue working further I think it would be great to get the PR which sets up the date-fns in the app and adds all the tests in place. Then we can start to slowly migrate over all the usages of Moment and eventually deprecate it and add some ESLint rule which will prohibit people of adding it back

[waterim]

@mountiny yes, sure, agree that it will be easier and safer. Will redo my PR tomorrow to have only tests and new library

[mountiny]

@waterim appreciate it 🙇

[waterim]

@mountiny Hello Vit, can you please assign me to this task?

And here is my PR just with tests and new library added, and without any date-fns inside of the code.

[mountiny]

@waterim Thank you 🙇 assigned and merged, I think we are good to work on the other prs.

[waterim]

@mountiny great, thank you) For the next PR I think we can go with rewriting ValidationUtils to date-fns and all files which are using it

[mountiny]

@waterim lets do that

[mountiny]

Another one merged 🚀

[waterim]

@mountiny nice to hear 🙃

[waterim]

@mountiny My update: Moving all usage of moment functions outside of the utils to the DateUtils. I mean all formattings, using of the moment.zoneAbbr() etc. Doing that to have all in one place, for the future it can help with rewriting or changing code related to timezones and dates.

[waterim]

Hey @mountiny, here is a PR regarding DateUtils and timezone fixes

[mountiny]

Added c+ to the issue and PR

[mountiny]

Merged, great job everyone, lets hope there wont be any issues with it

@waterim @burczu should we consider addig some rule for preventing people re-introducing moment

[situchan]

Regression: https://github.com/Expensify/App/issues/25650

[waterim]

@mountiny Here is a PR with tz.guess and polyfills, please test it too :)

[melvin-bot[bot]]

This issue has not been updated in over 15 days. @burczu, @anmurali, @allroundexperts, @mountiny, @waterim eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[mountiny]

@waterim is still working on this one and there is couple of prs left to get us over the finish line and remove moment from the app

[waterim]

@mountiny actually I have an update, I have enough capacity on Friday and today to proceed and will create PR today/tomorrow with new changes

There are a lot of hidden usages and all the time app is crashing after even a small change, but step by step is working fine

[situchan]

Heads-up: PR which linked this issue caused regression - https://github.com/Expensify/App/issues/28315 (old: https://github.com/Expensify/App/issues/26719) It was fixed in https://github.com/Expensify/App/pull/26902 but reverted in https://github.com/Expensify/App/pull/26248.

[waterim]

@mountiny PR is open, last PR will be with datepicker and moment will be removed FULLY from the project!

[waterim]

After my last changes, 2 years of history will disappear into oblivion

[waterim]

For the last changes will be:

1. To remove moment from github actions
2. To remove moment from package.json
3. To remove moment/locale from App.js