refactor(moment): Replace Moment.js with DayJs #31310

msyavuz · 2024-12-05T18:22:16Z

SUMMARY

This pr aims to replace Moment.js with Day.js as the former is a legacy project and is in maintenance mode. There were some design decisions made:

Datepicker component in Ant Design v4 accepts Moment object as props. There are couple options to remedy that:

Use a temporary plugin to allow Datepicker v4 to accept Day.js.
Refactor the Datepicker to allow Day.js.
Mark the usages to complete Day.js migration in the Datepicker pr refactor(date picker): Migrate Date Picker to Ant Design 5 #31019

I decided on the last option since other two seemed overkill for a temporary hurdle.

Extensions
Day.js object needed to be extended to support our usages. I centralized that in extendedDayjs to prevent confusion. Alternative to this is to extend where its used.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Run the testing suite.

ADDITIONAL INFORMATION

Has associated issue: Fixes Remove Moment.js in favor of Day.js #31178
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

rusackas · 2024-12-18T16:51:58Z

It sounds like Ant Design v4 is one hangup here. Pinging @geido since we're in the process of upgrading from v4 to v5, one component at a time. Would v5's Datepicker component mitigate this issue? If so, perhaps we can prioritize that, to get this PR through more easily.

msyavuz · 2024-12-18T16:59:19Z

@rusackas This PR and the Datepicker PR kind of depend on each other since Ant design v4 uses Moment and v5 uses Dayjs. In my opinion finishing the Moment to Dayjs migration on datepicker PR is the most straightforward way.

korbit-ai

Review by Korbit AI

Category	Issue
	Incorrect Time Unit Documentation ▹ view
	Missing Day.js Locale Plugin ▹ view
	Incorrect Duration Calculation Method ▹ view
	Redundant Timezone Conversion ▹ view

Files scanned

File Path	Reviewed
superset-frontend/src/components/CachedLabel/TooltipContent.tsx	✅
superset-frontend/src/components/Timer/Timer.stories.tsx	✅
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/utils.ts	✅
superset-frontend/src/components/ListView/Filters/DateRange.tsx	✅
superset-frontend/src/preamble.ts	✅
superset-frontend/plugins/plugin-chart-handlebars/src/components/Handlebars/HandlebarsViewer.tsx	✅
superset-frontend/src/explore/components/controls/DateFilterControl/utils/dateParser.ts	✅
superset-frontend/src/components/LastUpdated/index.tsx	✅
superset-frontend/src/utils/dates.ts	✅
superset-frontend/src/features/allEntities/AllEntitiesTable.tsx	✅
superset-frontend/src/pages/ExecutionLogList/index.tsx	✅
superset-frontend/src/visualizations/TimeTable/SparklineCell.tsx	✅
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/transformProps.ts	✅
superset-frontend/src/features/home/ActivityTable.tsx	✅
superset-frontend/src/dashboard/components/OverwriteConfirm/OverwriteConfirmModal.tsx	✅
superset-frontend/src/features/datasets/AddDataset/EditDataset/UsageTab/index.tsx	✅
superset-frontend/src/pages/AllEntities/index.tsx	✅
superset-frontend/src/explore/components/controls/TimeOffsetControl.tsx	✅
superset-frontend/src/components/TimezoneSelector/index.tsx	✅
superset-frontend/src/features/annotations/AnnotationModal.tsx	✅
superset-frontend/src/pages/AnnotationList/index.tsx	✅
superset-frontend/src/SqlLab/components/QueryTable/index.tsx	✅
superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx	✅
superset-frontend/src/pages/QueryHistoryList/index.tsx	✅
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx	✅
superset-frontend/src/pages/AlertReportList/index.tsx	✅
superset-frontend/webpack.config.js	✅
superset-frontend/src/components/Chart/chartAction.js	✅
superset-frontend/src/dashboard/components/Header/index.jsx	✅
superset-frontend/plugins/legacy-preset-chart-nvd3/src/NVD3Vis.js	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Need a new review? Comment /korbit-review on this PR and I'll review your latest changes.

Korbit Guide: Usage and Customization

Interacting with Korbit

You can manually ask Korbit to review your PR using the /korbit-review command in a comment at the root of your PR.

You can ask Korbit to generate a new PR description using the /korbit-generate-pr-description command in any comment on your PR.

Too many Korbit comments? I can resolve all my comment threads if you use the /korbit-resolve command in any comment on your PR.

Chat with Korbit on issues we post by tagging @korbit-ai in your reply.

Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.

Customizing Korbit

Check out our docs on how you can make Korbit work best for you and your team.

Customize Korbit for your organization through the Korbit Console.

Current Korbit Configuration

General Settings

Setting Value

Review Schedule Automatic excluding drafts

Max Issue Count 10

Automatic PR Descriptions ❌

Issue Categories

Category Enabled

Naming ✅

Database Operations ✅

Documentation ✅

Logging ✅

Error Handling ✅

Systems and Environment ✅

Objects and Data Structures ✅

Readability and Maintainability ✅

Asynchronous Processing ✅

Design Patterns ✅

Third-Party Libraries ✅

Performance ✅

Security ✅

Functionality ✅

Feedback and Support

Tell us what you think of Korbit

Schedule a call with our team

Email us @ support@korbit.ai

Note

Korbit Pro is free for open source projects 🎉

Looking to add Korbit to your team? Get started with a free 2 week trial here

korbit-ai · 2024-12-18T17:11:06Z

superset-frontend/src/utils/dates.ts

+export const now = function (): number {
+  // seconds from EPOCH as a float
+  return dayjs().utc().valueOf();
+};


Incorrect Time Unit Documentation

Tell me more

What is the issue?

The comment is incorrect as valueOf() returns milliseconds since epoch, not seconds

Why this matters

This misleading comment could cause integration issues if other developers rely on the comment's description of the return value being in seconds.

Suggested change

export const now = function (): number {
// milliseconds from EPOCH as a number
return dayjs().utc().valueOf();
};

Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.

korbit-ai · 2024-12-18T17:11:06Z

superset-frontend/src/preamble.ts

@@ -44,7 +44,7 @@ const bootstrapData = getBootstrapData();
 // Configure translation
 if (typeof window !== 'undefined') {
  configure({ languagePack: bootstrapData.common.language_pack });
-  moment.locale(bootstrapData.common.locale);
+  dayjs.locale(bootstrapData.common.locale);


Missing Day.js Locale Plugin

Tell me more

What is the issue?

Day.js locale support requires loading the appropriate locale plugin before setting the locale.

Why this matters

Without loading the locale plugin first, the locale setting will have no effect, causing date formatting to fallback to default English locale regardless of user preferences.

Suggested change

Import and load the Day.js locale plugin before setting the locale:

import dayjs from 'dayjs'; import LocalizedFormat from 'dayjs/plugin/localizedFormat'; dayjs.extend(LocalizedFormat);

Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.

korbit-ai · 2024-12-18T17:11:06Z

superset-frontend/src/utils/dates.ts

+export const fDuration = function (
+  t1: number,
+  t2: number,
+  format = 'HH:mm:ss.SSS',
+): string {
+  const diffSec = t2 - t1;
+  const duration = dayjs(new Date(diffSec));
+  return duration.utc().format(format);
+};


Incorrect Duration Calculation Method

Tell me more

What is the issue?

The fDuration function incorrectly calculates duration by creating a date from the difference in milliseconds instead of using dayjs.duration()

Why this matters

This will produce incorrect duration formatting as it treats the millisecond difference as an epoch timestamp rather than a duration, leading to unexpected results when formatting the time difference.

Suggested change

export const fDuration = function (
t1: number,
t2: number,
format = 'HH:mm:ss.SSS',
): string {
return dayjs.duration(t2 - t1).format(format);
};

Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.

korbit-ai · 2024-12-18T17:11:06Z

superset-frontend/src/components/TimezoneSelector/index.tsx

@@ -82,43 +70,40 @@ export default function TimezoneSelector({
      const getTimezoneName = (name: string) => {
        const offsets = getOffsetKey(name);
        return (
-          (currentDate.tz(name).isDST()
+          (isDST(currentDate.tz(name), name)


Redundant Timezone Conversion

Tell me more

What is the issue?

Double timezone conversion might occur as the date is already converted to the timezone before being passed to isDST, which might also perform a timezone conversion.

Why this matters

Unnecessary timezone conversions can lead to edge case bugs around DST boundaries and increase the chance of timezone-related calculation errors.

Suggested change

Pass the unconverted currentDate to isDST and let it handle the timezone conversion: isDST(currentDate, name)

Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.

geido

Great job! A few nits

...ntend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/transformProps.ts

superset-frontend/src/components/TimezoneSelector/TimezoneSelector.test.tsx

superset-frontend/src/components/TimezoneSelector/index.tsx

superset-frontend/src/features/alerts/AlertReportModal.test.tsx

superset-frontend/src/pages/AnnotationList/index.tsx

geido · 2024-12-19T11:55:30Z

superset-frontend/src/pages/ExecutionLogList/index.tsx

@@ -119,7 +119,7 @@ function ExecutionLog({
            original: { scheduled_dttm: scheduledDttm },
          },
        }: any) =>


Can we now use a better type than any?

For this one i think keeping any is ok still

superset-frontend/src/pages/ExecutionLogList/index.tsx

…ry loading

refactor(moment): Replace Moment.js with DayJs

ba7a325

pull-request-size bot added the size/L label Dec 5, 2024

github-actions bot added the dependencies:npm label Dec 5, 2024

sadpandajoe added the review:draft label Dec 5, 2024

msyavuz added 13 commits December 6, 2024 10:08

fix(package): sync package.json and tests

ded1285

refactor(moment): refactor moment out of components

63d7c02

refactor(dayjs): re-add moment and migrate components

17d0041

refactor(dayjs): migrate some components

8eb99d3

chore(moment): add comment and change moment to dayjs

f556d9c

refactor(dayjs): centralize dayjs extensions

8572277

chore(dayjs): add test for extensions and change package.jsons

fd2f451

fix(type): type from dayjs instead

4a55d06

Merge branch 'master' into msyavuz/momentjs-to-dayjs

9c8eca1

fix(test): only wait for load if its loading

6cbac28

test(AlertReportModal): use events directly instead?

e9fcacd

test(AlertReportModal): fix the test

a035c87

refactor(pluigin-echarts): move to dayjs

af97ee0

github-actions bot added the plugins label Dec 18, 2024

msyavuz added 3 commits December 18, 2024 17:57

chore: remove unused waitfor

5a0862a

feat(moment): remove timezone-support, migrate handlebars to dayjs

a3e10bf

chore(timezone-support): remove usages

638599f

rusackas requested a review from geido December 18, 2024 16:50

refactor(legacy-plugins): migrate to dayjs

854a66a

msyavuz marked this pull request as ready for review December 18, 2024 17:02

dosubot bot added change:frontend Requires changing the frontend frontend:refactor Related to refactoring the frontend labels Dec 18, 2024

korbit-ai bot reviewed Dec 18, 2024

View reviewed changes

michael-s-molina removed the review:draft label Dec 18, 2024

michael-s-molina requested review from villebro and justinpark December 18, 2024 18:22

geido added preset:bounty Issues that have been selected by Preset and have a bounty attached. hold! On hold labels Dec 18, 2024

geido reviewed Dec 19, 2024

View reviewed changes

msyavuz added 4 commits December 19, 2024 17:50

fix(timezoneselector): use map instead of array and remove unneccessa…

972bf08

…ry loading

refactor(alert report modal): remove loading check

58a7355

chore(alert report modal): remove unused import

f60a221

chore(types): add some types instead of any

03ab506

geido approved these changes Dec 23, 2024

View reviewed changes

geido removed the hold! On hold label Dec 23, 2024

geido merged commit a193d79 into apache:master Dec 23, 2024
35 of 38 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(moment): Replace Moment.js with DayJs #31310

refactor(moment): Replace Moment.js with DayJs #31310

msyavuz commented Dec 5, 2024 •

edited

Loading

rusackas commented Dec 18, 2024

msyavuz commented Dec 18, 2024

korbit-ai bot left a comment •

edited

Loading

korbit-ai bot Dec 18, 2024

korbit-ai bot Dec 18, 2024

korbit-ai bot Dec 18, 2024

korbit-ai bot Dec 18, 2024

geido left a comment

geido Dec 19, 2024

msyavuz Dec 23, 2024

Setting	Value
Review Schedule	Automatic excluding drafts
Max Issue Count	10
Automatic PR Descriptions	❌

Category	Enabled
Naming	✅
Database Operations	✅
Documentation	✅
Logging	✅
Error Handling	✅
Systems and Environment	✅
Objects and Data Structures	✅
Readability and Maintainability	✅
Asynchronous Processing	✅
Design Patterns	✅
Third-Party Libraries	✅
Performance	✅
Security	✅
Functionality	✅

refactor(moment): Replace Moment.js with DayJs #31310

refactor(moment): Replace Moment.js with DayJs #31310

Conversation

msyavuz commented Dec 5, 2024 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

rusackas commented Dec 18, 2024

msyavuz commented Dec 18, 2024

korbit-ai bot left a comment • edited Loading

Choose a reason for hiding this comment

Review by Korbit AI

Interacting with Korbit

Customizing Korbit

Current Korbit Configuration

Feedback and Support

Looking to add Korbit to your team? Get started with a free 2 week trial here

korbit-ai bot Dec 18, 2024

Choose a reason for hiding this comment

Incorrect Time Unit Documentation

What is the issue?

Why this matters

Suggested change

Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.

korbit-ai bot Dec 18, 2024

Choose a reason for hiding this comment

Missing Day.js Locale Plugin

What is the issue?

Why this matters

Suggested change

Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.

korbit-ai bot Dec 18, 2024

Choose a reason for hiding this comment

Incorrect Duration Calculation Method

What is the issue?

Why this matters

Suggested change

Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.

korbit-ai bot Dec 18, 2024

Choose a reason for hiding this comment

Redundant Timezone Conversion

What is the issue?

Why this matters

Suggested change

Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.

geido left a comment

Choose a reason for hiding this comment

geido Dec 19, 2024

Choose a reason for hiding this comment

msyavuz Dec 23, 2024

Choose a reason for hiding this comment

msyavuz commented Dec 5, 2024 •

edited

Loading

korbit-ai bot left a comment •

edited

Loading