Skip to content

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.

3 participants