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

refactor(deps): remove moment dep and usage #12611

Merged
merged 2 commits into from
Nov 5, 2024

Conversation

agilgur5
Copy link

@agilgur5 agilgur5 commented Feb 3, 2024

Partial fix for #12059, but removing / replacing a large dep entirely.

  • EDIT: turns out it's still used by the old version of chart.js in the deps, so it instead got code-split into the Reports page bundle, see "Notes to Reviewers" below

Follow-up to #12097

Motivation

moment has been deprecated since Sep 2020 and recommends using native Intl or newer libraries that make use of native Intl, such as luxon and day.js

  • moment is also a very large dependency and hence is ripe for pruning and replacement as well

  • shave off ~634KB / 260KB minified / 73KB gzipped from the main bundle by replacing ~15 LoC

Modifications

Verification

  1. make start UI=true

  2. View a specific node of a workflow; workflow-node-info looks correct to me. Screenshot:

    Screenshot 2024-10-28 at 4 15 38 PM

Bundle Analysis

before:

before moment removal - Screenshot 2024-10-28 at 3 23 27 PM

after:

after moment removal - Screenshot 2024-10-28 at 3 19 52 PM

Notes to Reviewers

moment was removed as a dep in this PR from our own usage and from argo-ui's usage (argoproj/argo-ui#535), but apparently it is still depended on by the old version of chart.js in the deps, so it got code-split out to the Reports bundle from #12061.

  • See "Future Work" below for how to improve that

Future Work

  1. Upgrade chart.js and react-chartjs-2 to more modern versions with tree-shaking and no direct dependence on moment

@agilgur5 agilgur5 added type/dependencies PRs and issues specific to updating dependencies area/ui javascript Pull requests that update Javascript dependencies labels Feb 3, 2024
@agilgur5
Copy link
Author

NOTE: This is still a draft right now as to fully remove moment requires upstream changes in argo-ui: argoproj/argo-ui#535

This was finally merged in late August, so this PR can now be finalized

@agilgur5 agilgur5 force-pushed the refactor-deps-remove-moment branch from 34678ed to 4c62f08 Compare October 28, 2024 18:52
@agilgur5
Copy link
Author

Apparently moment is still depended on by the old version of the chart.js dep, so it didn't get entirely removed, but did get code-split from the main bundle at least. See the new "Future Work" section above

@agilgur5 agilgur5 marked this pull request as ready for review October 28, 2024 20:18
Anton Gilgur added 2 commits November 5, 2024 11:07
`moment` has been [deprecated since Sep 2020](https://momentjs.com/docs/#/-project-status/) and recommends using native `Intl` or newer libraries that make use of native `Intl`, such as `luxon` and `dayjs`
  - `moment` is also a very large dependency and hence is ripe for pruning and replacement as well

- replace all usage of `moment` with regular `Date` functions
  - `valueOf()` works the same way for `Date` and `moment`
  - `diff` doesn't exist, but it's a one-liner helper
  - `format` doesn't exist, but we can also use a simple helper for that

- shave off X kb by replacing ~15 LoC

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5 agilgur5 force-pushed the refactor-deps-remove-moment branch from 41d4fa4 to 3c33a21 Compare November 5, 2024 16:09
@agilgur5
Copy link
Author

agilgur5 commented Nov 5, 2024

Fixed conflicts

@tczhao tczhao merged commit 2f3d6a6 into argoproj:main Nov 5, 2024
16 checks passed
@agilgur5 agilgur5 deleted the refactor-deps-remove-moment branch November 6, 2024 05:26
Joibel pushed a commit that referenced this pull request Nov 22, 2024
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
(cherry picked from commit 2f3d6a6)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ui javascript Pull requests that update Javascript dependencies type/dependencies PRs and issues specific to updating dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants