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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(deps)!: remove moment dep and usage #535

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

Conversation

agilgur5
Copy link
Member

@agilgur5 agilgur5 commented Feb 12, 2024

Related to the goals of argoproj/argo-workflows#12059, just removing / replacing a large dep entirely.
Follow-up to argoproj/argo-workflows#12097 and #534 here.
Needed for argoproj/argo-workflows#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 from v2/utils as it is unused

    • it is unused internally (it is only exported here), unused by CD, unused by Workflows
      • it's also unused by Rollouts, which has two internal copies of this function
  • change Ticker component to use plain Date instead of moment

    • downstream projects should get a type error on this which should make the switch straightforward
  • change Duration component to use existing formatDuration function which doesn't use moment

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 (argoproj/argo-workflows#12611 for instance) and before it gets entirely forgotten 馃槙

@agilgur5 agilgur5 added type/dependencies Pull requests that update a dependency file javascript Pull requests that update Javascript code labels Feb 12, 2024

This comment was marked as resolved.

@github-actions github-actions bot added the problem/stale This has not had a response in some time label Apr 13, 2024
@agilgur5 agilgur5 removed the problem/stale This has not had a response in some time label Apr 13, 2024
@agilgur5 agilgur5 force-pushed the refactor-deps-remove-moment branch from 6679442 to 6062986 Compare May 12, 2024 03:06
`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

- remove `formatTimestamp` function from `v2/utils` as it is unused
  - it is unused [internally](https://github.com/search?q=repo%3Aargoproj%2Fargo-ui+formattimestamp&type=code) (it is only exported here), unused by [CD](https://github.com/search?q=repo%3Aargoproj%2Fargo-cd%20formattimestamp&type=code), unused by [Workflows](https://github.com/search?q=repo%3Aargoproj%2Fargo-workflows+formattimestamp&type=code)
    - it's also and unused by Rollouts, which has [two](https://github.com/argoproj/argo-rollouts/blob/c688dd89a7f883c8c004b4024b390104b5be5770/ui/src/app/shared/utils/utils.tsx#L17) [internal](https://github.com/argoproj/argo-rollouts/blob/c688dd89a7f883c8c004b4024b390104b5be5770/ui/src/app/components/rollout/revision.tsx#L21) copies of this function

- change `Ticker` component to use plain `Date` instead of `moment`
  - downstream projects should get a type error on this which should make the switch straightforward

- change `Duration` component to use existing `formatDuration` function which doesn't use `moment`
  - 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](https://github.com/argoproj/argo-cd/blob/9ecc5aec2a4dba485eb8b5b0ab75fff8927b3418/ui/src/app/applications/components/application-deployment-history/application-deployment-history.tsx#L1) [places](https://github.com/argoproj/argo-cd/blob/9ecc5aec2a4dba485eb8b5b0ab75fff8927b3418/ui/src/app/applications/components/application-operation-state/application-operation-state.tsx#L1) and Rollouts uses this in [one place](https://github.com/argoproj/argo-rollouts/blob/c688dd89a7f883c8c004b4024b390104b5be5770/ui/src/app/components/pods/pods.tsx#L3)
    - Workflows does not use this, it has [its own `DurationPanel` component](https://github.com/argoproj/argo-workflows/blob/1dbc856e51967feb58066a4087a8679b08b87be3/ui/src/app/shared/components/duration-panel.tsx#L7) and its own [internal copy](https://github.com/argoproj/argo-workflows/blob/1dbc856e51967feb58066a4087a8679b08b87be3/ui/src/app/shared/duration.ts#L11) of the `formatDuration` function as well
    - none of these use the `allowNewLines` prop, so it is safe to remove

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5 agilgur5 force-pushed the refactor-deps-remove-moment branch from 6062986 to 53189d0 Compare May 12, 2024 03:08
@agilgur5 agilgur5 marked this pull request as ready for review May 12, 2024 03:09
@agilgur5
Copy link
Member Author

agilgur5 commented May 12, 2024

Rebased on top of / fixed merged conflicts with #534 now that it's been merged and marked this ready for review!

@crenshaw-dev
Copy link
Contributor

Looks like the new duration component is behaving weirdly.

Before:

image

After:

image

@agilgur5
Copy link
Member Author

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

@agilgur5
Copy link
Member Author

agilgur5 commented May 12, 2024

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")

src/components/duration.tsx Outdated Show resolved Hide resolved
ms -> s

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5 agilgur5 force-pushed the refactor-deps-remove-moment branch from fd135f7 to 05d91cc Compare May 12, 2024 04:57
@agilgur5
Copy link
Member Author

Should be fixed now

Copy link
Member

@isubasinghe isubasinghe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks okay but unable to run storybook locally, deps need updating

@agilgur5
Copy link
Member Author

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

@crenshaw-dev
Copy link
Contributor

Still getting somewhat strange behavior.

Before:

image

After:

image

@agilgur5
Copy link
Member Author

agilgur5 commented Jun 5, 2024

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 馃

@agilgur5
Copy link
Member Author

agilgur5 commented Jun 5, 2024

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.

- apparently `durationMs` was incorrectly named as it actually took seconds, not milliseconds
- for backward compat, leave `durationMs`, but also introduce `durationS`
  - and add JSDoc to indicate this to users of the component

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5
Copy link
Member Author

agilgur5 commented Jun 6, 2024

I guess I'll leave the buggy behavior for backward compat and add a JSDoc comment about this.

Added JSDoc indication that looks like:
Screenshot 2024-06-05 at 7 54 16鈥疨M

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 in 4d and 4m respectively.

I made it 2 significant figures now, so it will be 4d5h and 4m9s now.

@crenshaw-dev
Copy link
Contributor

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)

@agilgur5 agilgur5 changed the title refactor(deps): remove moment dep and usage refactor(deps)!: remove moment dep and usage Jun 7, 2024
@agilgur5
Copy link
Member Author

agilgur5 commented Jun 7, 2024

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Pull requests that update Javascript code type/dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants