refactor: replace moment with dayjs#1856
refactor: replace moment with dayjs#1856UnschooledGamer merged 19 commits intoAcode-Foundation:mainfrom
Conversation
Summary of ChangesHello @hyperz111, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the project's date handling by replacing the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Greptile SummaryReplaces the unmaintained Key changes:
Note: Previous review comments mentioned that Confidence Score: 4/5
Important Files Changed
Flowchartflowchart TD
A[Module Load] --> B[Import dayjs ESM]
B --> C[Extend relativeTime plugin]
C --> D[Extend utc plugin]
D --> E[Extend updateLocale plugin]
E --> F[Configure custom relativeTime locale]
F --> G[Module Ready]
H[formatUpdatedDate called] --> I{dateString exists?}
I -->|No| J[Return null]
I -->|Yes| K[dayjs.utc parse dateString]
K --> L{Valid date?}
L -->|No| J
L -->|Yes| M[Call fromNow]
M --> N[Return relative time string]
style F fill:#e1f5e1
style K fill:#e1f5e1
style M fill:#e1f5e1
Last reviewed commit: e87700f |
Additional Comments (1)
In Also, A minimal fix is to use Prompt To Fix With AIThis is a comment left during a code review.
Path: src/pages/plugin/plugin.view.js
Line: 52:80
Comment:
[P1] `fromNow(true)` drops the “ago/in …” suffix, changing UI text
In `formatUpdatedDate`, `moment.utc(dateString).fromNow()` previously returned strings including the suffix (e.g., `"5m ago"`). With Day.js you’re currently doing `updateTime.fromNow(true)`, which returns *only* the distance (e.g., `"5m"`) and will never append `past`/`future` strings (so the `past: "%s ago"` config won’t apply). This changes the displayed `packageUpdatedAt` text.
Also, `moment.utc(dateString)` preserved UTC semantics; `dayjs(dateString)` will parse in local time unless the input includes a timezone offset. If `package_updated_at` is UTC and doesn’t include an offset, this can skew the relative time.
A minimal fix is to use `fromNow()` (no arg) and, if UTC is required, `dayjs.utc(dateString)` with the `utc` plugin.
How can I resolve this? If you propose a fix, please make it concise. |
There was a problem hiding this comment.
Code Review
This pull request successfully replaces the unmaintained moment library with dayjs. The dependency changes in package.json and package-lock.json are correct. However, I've identified a critical issue in src/pages/plugin/plugin.view.js where replacing moment.utc() with dayjs() could lead to incorrect date parsing due to timezone differences. I've also pointed out an opportunity to improve performance and fix a minor display bug by moving the dayjs locale configuration out of a frequently called function. Overall, a good refactoring with a couple of important adjustments needed.
|
While Moment.js is unmaintained it's API is pretty much good considerably. The Moment.js is a Pretty Much Done on its base Feature set. Also, Moment.js explained this better in their docs here -> https://momentjs.com/docs/#/-project-status/ |
|
I get the |
|
I don't think there is need for this change. |
I get this build size difference. Before: After: |
@bajrangCoder, I think you should read https://momentjs.com/docs/#/-project-status/ before making a decision, also considering the package of moment.js (~4 mb) and dayjs being <1 mb |
|
@bajrangCoder, what about this? |
momentis unmaintained, so i replace it withdayjs.