Skip to content

feat: Remove moment js #302

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

Merged
merged 7 commits into from
Jun 10, 2025
Merged

feat: Remove moment js #302

merged 7 commits into from
Jun 10, 2025

Conversation

mertkirbuga
Copy link
Contributor

Resolves F-168

  • I have added automatic tests where applicable
  • The PR title is suitable as a release note
  • The PR contains a description of what has been changed
  • The description contains manual test instructions

Changes

All moment js instances is replaced by dayjs instances.

Test

TBD

@mertkirbuga mertkirbuga requested a review from jimmycallin May 28, 2025 09:41
@mertkirbuga mertkirbuga self-assigned this May 28, 2025
@mertkirbuga mertkirbuga requested a review from a team as a code owner May 28, 2025 09:41
@mertkirbuga mertkirbuga added enhancement New feature or request dependencies Pull requests that update a dependency file labels May 28, 2025
Comment on lines 334 to 338
// Ensure that the dayjs object is in local time zone and format
// to timezone naive string.
return {
__type__: "datetime",
value: moment(date).local().format(ENCODE_DATETIME_FORMAT),
value: dayjs.utc(date).local().format(ENCODE_DATETIME_FORMAT),
Copy link
Contributor Author

@mertkirbuga mertkirbuga May 28, 2025

Choose a reason for hiding this comment

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

Should we remove this also? @jimmycallin

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need an alternative to replace this? Because in this case, when serverInformation and timezone is enabled we will return an object { __type__: "datetime", value:date }. Otherwise, we will return the data instead of date.

Copy link
Contributor

Choose a reason for hiding this comment

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

is_timezone_support_enabled is a separate from recently implemented use_workspace_timezone_enabled - it's an old flag that should always be enabled. we can remove this check and only support when it's true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still a bit uncertain about my implementation. Can you check this commit f71e035?

@jimmycallin jimmycallin changed the title feat: Replace moment js with dayjs feat!: Remove moment js May 28, 2025
@mertkirbuga mertkirbuga changed the title feat!: Remove moment js feat: Remove moment js Jun 3, 2025
@mertkirbuga mertkirbuga merged commit d43725b into main Jun 10, 2025
3 checks passed
@mertkirbuga mertkirbuga deleted the replace-momentjs-with-dayjs branch June 10, 2025 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants