argo-ui
argo-ui copied to clipboard
refactor(deps): remove `moment` dep and usage
Related to the goals of https://github.com/argoproj/argo-workflows/issues/12059, just removing / replacing a large dep entirely. Follow-up to https://github.com/argoproj/argo-workflows/pull/12097 and #534 here. Needed for https://github.com/argoproj/argo-workflows/pull/12611
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
Modifications
-
remove
formatTimestamp
function fromv2/utils
as it is unused- it is unused internally (it is only exported here), unused by CD, unused by Workflows
-
change
Ticker
component to use plainDate
instead ofmoment
- downstream projects should get a type error on this which should make the switch straightforward
-
change
Duration
component to use existingformatDuration
function which doesn't usemoment
- this will look different than the previous version, but the formatting between the two should really be consistent in any case
- other option was to just remove this component entirely, since this repo is not actively maintained
- leaving the component in instead of deleting it gives a more gradual off-ramp
- CD uses this component in two places and Rollouts uses this in one place
- CD may prefer this formatting per #65 / https://github.com/argoproj/argo-cd/issues/4388
- Workflows does not use this, it has its own
DurationPanel
component and its own internal copy of theformatDuration
function as well - none of these use the
allowNewLines
prop, so it is safe to remove
Verification
Builds and tests pass
Notes to Reviewers
I actually wrote this weeks ago in Jan (see the commits), around the same time as my other PRs, but this merge conflicts with some of them, so I didn't create the PR, intending to rebase with those once they were merged. It's already been over a month with no review/merge, so I'm placing this here as as Draft at least so I can reference/link to it from other PRs (https://github.com/argoproj/argo-workflows/pull/12611 for instance) and before it gets entirely forgotten 😕
Stale pull request message
Rebased on top of / fixed merged conflicts with #534 now that it's been merged and marked this ready for review!
Looks like the new duration component is behaving weirdly.
Before:
After:
That was intentional, to match the formatDuration
func, as I wrote in the PR description.
Otherwise, as I wrote there, we could remove the component entirely
OHHH, did you mean the "Active for: 85d"? That does sound buggy and is not equivalent to "02:00 hours"
I didn't notice that initially (playing spot the difference as you didn't specify 😅), I thought you just meant the different format in "Time to deploy: 0s" (which is equivalent to the previous "00:00 min")
Should be fixed now
Looks okay but unable to run storybook locally, deps need updating
See #469 with regard to the currently broken Storybook build 😕
#536 partially fixes that
Still getting somewhat strange behavior.
Before:
After:
Huh well that's more confusing... sigfigs
was the only part I ignored, but that should result in 4d
and 4m
respectively. Floating point arithmetic is also imprecise, but not that far off. I'm actually not sure what's causing the math to be off now 🤔
I did some testing:
const minute = 60;
const hour = minute * 60;
const day = hour * 24;
// spot checks
console.log(formatDuration(20)); // 20s
console.log(formatDuration(day)); // 24h
console.log(formatDuration(day * 4)); // 4d
// CD screenshot checks
const s4m9s = minute * 4 + 9;
console.log(formatDuration(s4m9s)); // 4m
console.log(formatDuration(s4m9s / 1000)); // 0s
const s4d5h = day * 4 + hour * 5;
console.log(formatDuration(s4d5h)); // 4d
console.log(formatDuration(s4d5h / 1000)); // 6m
Those numbers line up exactly -- this means that the above is actually a bug in CD; it's passing seconds to durationMs
, which takes, well, milliseconds.
Or, well, that does also mean that this component was buggy; it previously took seconds despite the durationMs
prop name.
I guess I'll leave the buggy behavior for backward compat and add a JSDoc comment about this.
I guess I'll leave the buggy behavior for backward compat and add a JSDoc comment about this.
Added JSDoc indication that looks like:
Note that the JSDoc @deprecated
does not work for functions, so it is manually annotated.
sigfigs
was the only part I ignored, but that should result in4d
and4m
respectively.
I made it 2
significant figures now, so it will be 4d5h
and 4m9s
now.
Man, so close. I did a sync in the UI and got this:
Something went wrong!
Consider submitting an issue [here](https://github.com/argoproj/argo-cd/issues/new?labels=bug&template=bug_report.md).
Stacktrace:
TypeError: ((operationState.finishedAt && moment__WEBPACK_IMPORTED_MODULE_1__(...)) || time).diff is not a function
at Object.eval [as children] (webpack-internal:///./src/app/applications/components/application-operation-state/application-operation-state.tsx:89:123)
at Ticker.render (webpack-internal:///./node_modules/argo-ui/src/components/ticker.tsx:17:46)
at finishClassComponent (webpack-internal:///./node_modules/react-dom/cjs/react-dom.development.js:11841:35)
at updateClassComponent (webpack-internal:///./node_modules/react-dom/cjs/react-dom.development.js:11809:28)
at beginWork (webpack-internal:///./node_modules/react-dom/cjs/react-dom.development.js:12758:18)
at HTMLUnknownElement.callCallback2 (webpack-internal:///./node_modules/react-dom/cjs/react-dom.development.js:103:18)
at Object.invokeGuardedCallbackDev (webpack-internal:///./node_modules/react-dom/cjs/react-dom.development.js:128:20)
at invokeGuardedCallback (webpack-internal:///./node_modules/react-dom/cjs/react-dom.development.js:159:35)
at beginWork$1 (webpack-internal:///./node_modules/react-dom/cjs/react-dom.development.js:15669:11)
at performUnitOfWork (webpack-internal:///./node_modules/react-dom/cjs/react-dom.development.js:15002:16)
TypeError: ((operationState.finishedAt && moment__WEBPACK_IMPORTED_MODULE_1__(...)) || time).diff is not a function at Object.eval [as children] (webpack-internal:///./src/app/applications/components/application-operation-state/application-operation-state.tsx:89:123) at Ticker.render (webpack-internal:///./node_modules/argo-ui/src/components/ticker.tsx:17:46)
That error is coming from this line in CD.
The error is correct, this PR makes a breaking change to the Ticker
component to use Date
instead of moment
. Unlike the Duration
component, moment
was exposed externally here (Duration
only used it internally), so this must be changed in order to remove moment
.
It'll fail to typecheck as well, as I wrote in my PR description. So that is intentional, CD needs to refactor its usage of the Ticker
component to not rely on passing moment
types to/from it.
Man, so close.
If there's no other issues, I think this is good to merge.
I modified the title here to use the !
for breaking change indication, although this repo does not check for conventional commits and has had various unnoted breaking changes in the past (including by dependabot)
Sure, it's <2 LoC: https://github.com/argoproj/argo-cd/pull/19655, though I do recommend following that up with a moment
removal in CD as well