[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]

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]

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.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.95-9 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Reopened/Feature: Remove moment from datepicker #29984

If no regressions arise, payment will be issued on 2023-11-13. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @burczu does not require payment (Contractor)
• @allroundexperts does not require payment (Eligible for Manual Requests)
• @waterim does not require payment (Contractor)

[melvin-bot]

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@burczu / @allroundexperts] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
• [@anmurali] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

[mountiny]

Almost there!

[waterim]

Hopefully no regression will appear :)

[mountiny]

Thanks for continued work on this ❤️

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.99-0 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• [NoQA] Feature: Remove moment fully #31071

If no regressions arise, payment will be issued on 2023-11-22. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @burczu does not require payment (Contractor)
• @allroundexperts does not require payment (Eligible for Manual Requests)
• @waterim does not require payment (Contractor)

[melvin-bot]

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@burczu / @allroundexperts] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
• [@anmurali] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

[mountiny]

@allroundexperts could you please summarize what needs to be paid here to you, it was couple of PRs.

No Testrail cases are needed fo this, it was a migration of a package for performance reasons and no new behaviour has been added in this issue

[allroundexperts]

Hi @mountiny!

It's been a long issue but I've tried to summarise the payment below. Please let me know if I missed something.

1. Datetime polyfill, removed moment.tz.guess() #26248 (This caused a regression but it was found after 7 days it was deployed to production) -> Flat review ($500)
2. [NoQA] feat: Remove moment from the project (Except Datepicker) #28175 -> Flat review ($500)
3. Feat: Remove moment from datepicker #29062 -> Had a regression and was reverted and then redone here. The second go had regressions as well. So the penalty doubles. ($125)
4. [NoQA] Feature: Remove moment fully #31071 -> Flat review ($500)

[mountiny]

Sounds good to me, Could you please request $1625 on NewDot and note here, then we can close this issue. Thanks!

[allroundexperts]

All done @mountiny!

[mountiny]

Thanks, we should be all good here and we can close, thanks @waterim and @burczu and @allroundexperts for the effort here

[JmillsExpensify]

$1,625 payment approved based on this comment.