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

feat: deprecate accessing DataTypes directly on Sequelize class #14373

Merged
merged 4 commits into from Apr 13, 2022
Merged

feat: deprecate accessing DataTypes directly on Sequelize class #14373

merged 4 commits into from Apr 13, 2022

Conversation

ephys
Copy link
Member

@ephys ephys commented Apr 12, 2022

Pull Request Checklist

  • 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

This PR deprecates accessing DataTypes directly on the Sequelize constructor.
Instead, users should use the DataTypes object.

These DataTypes take up a lot of possible names and should be kept in their own namespace:

// deprecated
Sequelize.STRING
Sequelize.INTEGER
// ...

// not deprecated
DataTypes.STRING
DataTypes.INTEGER
// ...

Sequelize.DataTypes.STRING
Sequelize.DataTypes.INTEGER
// ...

@ephys ephys self-assigned this Apr 12, 2022
@ephys ephys added the type: refactor For issues and PRs. Things that improve the code readability, maintainability, testability, etc. label Apr 12, 2022
@ephys ephys mentioned this pull request Apr 12, 2022
Merged
@WikiRik
Copy link
Member

WikiRik commented Apr 12, 2022

Maybe I missed it with my quick scroll through the changes, but do we test somewhere that Sequelize.DataTypes.STRING is valid?

ephys added a commit to sequelize/website that referenced this pull request Apr 12, 2022
@ephys
document PR sequelize/sequelize#14373
89328c0
@ephys
Copy link
Member Author

ephys commented Apr 12, 2022

Not directly but require('@sequelize/core') returns Sequelize so const { DataTypes } = require('@sequelize/core'); is equal to Sequelize.DataTypes

@WikiRik
Copy link
Member

WikiRik commented Apr 12, 2022

Ah of course, that's true

@ephys
Copy link
Member Author

ephys commented Apr 12, 2022 •
edited

Looks like I missed a few import updates

Edit: I accidentally fixed a DataType typo that needed to be there:

image

@ephys ephys mentioned this pull request Apr 12, 2022
Closed
6 tasks
@ephys ephys requested a review from a team April 13, 2022 08:39
@WikiRik WikiRik merged commit 4bbd4fb into main Apr 13, 2022
@WikiRik WikiRik deleted the ephys/deprecate-datatypes branch April 13, 2022 10:13
@fzn0x
Copy link
Member

fzn0x commented Apr 13, 2022

Nice changes, remove confusion between using Sequelize class workaround or not for DataTypes.

sdepold
sdepold reviewed Apr 13, 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.

Can we mark this as breaking change in the commit message somehow?

Good change overall

@sdepold
Copy link
Member

sdepold commented Apr 13, 2022

Oh. Too slow :(

@sdepold
Copy link
Member

sdepold commented Apr 13, 2022

Aha! There is a pr for this.

@WikiRik
Copy link
Member

WikiRik commented Apr 13, 2022 •
edited

Aha! There is a pr for this.

To document this change? Yeah, this is indeed something that we needed to put in the upgrade guide

EDIT: sequelize/website#80

@WikiRik WikiRik mentioned this pull request Apr 13, 2022
Merged
@github-actions
Copy link
Contributor

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

The release is available on:

Your semantic-release bot 📦🚀

sdepold added a commit to sequelize/website that referenced this pull request Apr 20, 2022
@ephys @sdepold
document deprecations & removals (#80)
15e4219 
* document deprecations & removals

* document PR sequelize/sequelize#14373

Co-authored-by: Sascha Depold <sdepold@users.noreply.github.com>
vanthome pushed a commit to vanthome/sequelize that referenced this pull request Jun 12, 2022
@ephys @WikiRik @vanthome
feat: deprecate accessing DataTypes directly on Sequelize class (sequ…
f894f8c 
…elize#14373)

* feat: deprecate accessing DataTypes directly on Sequelize class

* docs: update datatypes in jsdoc

* test: revert fixing of a datatype that needs to be broken

Co-authored-by: Rik Smale <13023439+WikiRik@users.noreply.github.com>
gaspardmonet added a commit to gaspardmonet/website that referenced this pull request Feb 6, 2024
@gaspardmonet @sdepold
document deprecations & removals (#80)
71440df 
* document deprecations & removals

* document PR sequelize/sequelize#14373

Co-authored-by: Sascha Depold <sdepold@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sdepold sdepold sdepold left review comments

@WikiRik WikiRik WikiRik approved these changes

Assignees

@ephys ephys

Labels
released on @v7 type: refactor For issues and PRs. Things that improve the code readability, maintainability, testability, etc.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ephys @WikiRik @fzn0x @sdepold