-
Notifications
You must be signed in to change notification settings - Fork 781
refactor: replace moment with dayjs #1856
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
base: main
Are you sure you want to change the base?
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 OverviewGreptile SummaryThis PR replaces the unmaintained The main behavioral risk is in Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as Plugin view (plugin.view.js)
participant Dayjs as dayjs + plugins
UI->>Dayjs: extend(relativeTime)
UI->>Dayjs: extend(updateLocale)
UI->>UI: render Plugin page
UI->>UI: formatUpdatedDate(packageUpdatedAt)
UI->>Dayjs: updateLocale("en", relativeTime overrides)
UI->>Dayjs: dayjs(dateString)
Dayjs-->>UI: Dayjs instance
UI->>Dayjs: isValid()
Dayjs-->>UI: boolean
UI->>Dayjs: fromNow(true)
Dayjs-->>UI: relative string
UI-->>UI: display "(updatedText)" in Version component
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 files reviewed, 2 comments
src/pages/plugin/plugin.view.js
Outdated
| @@ -68,12 +73,12 @@ export default (props) => { | |||
| }, | |||
| }); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P3] dayjs.updateLocale is run on every formatUpdatedDate call
dayjs.updateLocale("en", …) mutates the global locale config; doing this inside formatUpdatedDate() means it executes on every render/call and affects all Day.js usage in the app. Consider moving the updateLocale call to module init (next to dayjs.extend(...)) or guarding it so it only runs once.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/pages/plugin/plugin.view.js
Line: 55:74
Comment:
[P3] `dayjs.updateLocale` is run on every `formatUpdatedDate` call
`dayjs.updateLocale("en", …)` mutates the global locale config; doing this inside `formatUpdatedDate()` means it executes on every render/call and affects all Day.js usage in the app. Consider moving the `updateLocale` call to module init (next to `dayjs.extend(...)`) or guarding it so it only runs once.
How can I resolve this? If you propose a fix, please make it concise.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: |
momentis unmaintained, so i replace it withdayjs.