Skip to content
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.

Sign up for GitHub

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

Jump to bottom

refactor: ♻️ migrate moment.js to dayjs #14400

Merged
merged 12 commits into from May 19, 2022
Merged

refactor: ♻️ migrate moment.js to dayjs #14400

merged 12 commits into from May 19, 2022

Conversation

DraftProducts
Copy link
Contributor

Pull Request Checklist

Please make sure to review and check all of these items:

  • Have you added new tests to prevent regressions?
  • Does yarn test or yarn test-DIALECT pass with this change (including linting)?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you update the typescript typings accordingly (if applicable)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description Of Change

Migrate moment.js to dayjs.
Closes: #13372

@DraftProducts
Copy link
Contributor Author

DraftProducts commented Apr 15, 2022 •
edited

I have 4 failing tests.
Looks like it's not from this PR : (please note that the results are same with sqlite and postgres tests)
image

@WikiRik
Copy link
Member

WikiRik commented Apr 15, 2022

I have 4 failing tests.
Looks like it's not from this PR : (please note that the results are same with sqlite and postgres tests)
image

Yes, these stem from #14390. No need to fix them in this PR

ephys
ephys requested changes Apr 15, 2022
View reviewed changes
Copy link
Member

@ephys ephys left a comment

Choose a reason for hiding this comment

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

Thank you for the PR :)
Overall, looks good, my main comment is that we should not include a built-in support for dayjs objects (i.e. they should not be accepted as values by DataTypes.DATE), to avoid needing to introduce another breaking change when Temporal becomes stable

test/integration/data-types.test.js Show resolved Hide resolved
src/sequelize.js Outdated Show resolved Hide resolved
src/utils/dayjs.ts Show resolved Hide resolved
src/sequelize.d.ts Outdated Show resolved Hide resolved
DraftProducts and others added 3 commits April 15, 2022 15:40
@DraftProducts @ephys
Update documentation about acceptable timezone
1d9331c 
Co-authored-by: Zoé <zoe@ephys.dev>
@DraftProducts @ephys
Update documentation about acceptable timezone
d0201d2 
Co-authored-by: Zoé <zoe@ephys.dev>
@DraftProducts @ephys
perf: ⚡️ cache result of timezone
9192cf4 
Co-authored-by: Zoé <zoe@ephys.dev>
@ephys
Copy link
Member

ephys commented Apr 15, 2022

Looks like there is a failing test when the timeZoneOffset is +00:00. Can you ping me once the tests pass? Or if you need help :)

ephys
ephys reviewed Apr 18, 2022
View reviewed changes
src/data-types.js Outdated Show resolved Hide resolved
@ephys ephys mentioned this pull request Apr 18, 2022
Merged
7 tasks
@DraftProducts
Copy link
Contributor Author

Hi @ephys Have you an idea of where is coming from the last CI fail ? (CI / DB2 (Node 14))
I had a look, but I don't understand where is the link with our changes.

@ephys
Copy link
Member

ephys commented Apr 18, 2022

That DB2 test is randomly failing, you can ignore it. Sorry for having it waste your time

@sdepold
Copy link
Member

sdepold commented May 17, 2022

What's the status of this one?

@DraftProducts
Copy link
Contributor Author

Okay, last changes requested are done.

@DraftProducts DraftProducts requested a review from ephys May 17, 2022 12:51
@DraftProducts
Copy link
Contributor Author

DraftProducts commented May 17, 2022 •
edited

No conflicts any more 👍

sdepold
sdepold reviewed May 18, 2022
View reviewed changes
yarn.lock
@@ -5882,7 +5887,7 @@ moment-timezone@^0.5.15, moment-timezone@^0.5.34:
dependencies:
moment ">= 2.9.0"

"moment@>= 2.9.0", moment@^2.29.1, moment@^2.29.3:
"moment@>= 2.9.0", moment@^2.29.3:
Copy link
Member

Choose a reason for hiding this comment

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

do we still need moment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's still needed, moment is a dependency of snowflake-sdk@1.6.9 and mariadb@2.5.6.

You might wonder what the point of this PR would be if we keep the moment dependency.
The advantage will be that it will no longer be an internal dependency of the project.
It will only be needed if the developer needs the mariadb peer dependency or if the developer is a sequelize contributor because snowflake-sdk is a development dependency.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense :)

sdepold
sdepold reviewed May 18, 2022
View reviewed changes
Copy link
Member

@sdepold sdepold left a comment

Choose a reason for hiding this comment

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

looks fine overall!

@sdepold sdepold merged commit 9395e79 into sequelize:main May 19, 2022
@github-actions
Copy link
Contributor

🎉 This PR is included in version 7.0.0-alpha.13 🎉

The release is available on:

Your semantic-release bot 📦🚀

vanthome pushed a commit to vanthome/sequelize that referenced this pull request Jun 12, 2022
@DraftProducts @vanthome
refactor: ♻️ migrate moment.js to dayjs (sequelize#14400)
dfd7d97
@louh louh mentioned this pull request Jul 18, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fzn0x fzn0x fzn0x left review comments

@sdepold sdepold sdepold approved these changes

@ephys ephys Awaiting requested review from ephys

Assignees
No one assigned
Labels
released on @v7
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Chore: migrate away from moment.js
5 participants
@DraftProducts @WikiRik @ephys @sdepold @fzn0x