-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
refactor: convert utils to typescript #13782
Conversation
7d96c0c
to
0fdb3f3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The majority of my review is about making the types stricker or add hints for TypeScript.
Some of them are based on my understanding of the methods, so a double review is welcome
I've resolved the issue you had with this PR being dependant on the DataTypes PR. Solution was the same as my own TS conversion PR:
If you're ok with it I can push my changes to this branch (edit: done). They include a resolution for the merge conflict + these changes. It compiles now :) |
Working a bit more on this currently I'm changing part of sequelize.define('User', {
firstName: {
type: DataTypes.STRING,
field: 'name' // this is also the name of another attribute but it should work because User.name uses "name2" as a column name!
},
name: {
type: DataTypes.STRING,
field: 'name2'
}
}); |
for future debugging :)
Damnit. I broke TypeScript < 4.4 because they finally allowed And it also broke in TS < 4.1 because I used template string types again but I'll wait until sequelize/meetings#6 (comment) has been decided to change it |
There is something fishy going on in this PR. Some of our tests are randomly failing. I've seen sqlite fail, then postgres in a re-run. |
For future me: https://github.com/sequelize/sequelize/pull/13992/files#diff-3274f1a37032fb0ae4e2823def0007c634e869ae0dfc304ff6a12c36513c3a52R271 this is the code that I need to include in the new utils when resolving the new conflict |
In also some failing tests With mariadb and than something else on my other pr |
So probably every dialect, but at different times. Very weird. I'll keep a list of failing suites here so we can investigate whether they have something in common:
https://github.com/sequelize/sequelize/runs/5152023145?check_suite_focus=true This one I've seen fail in other PRs, so unrelated to this one
https://github.com/sequelize/sequelize/runs/5156282992?check_suite_focus=true
|
The logs are only available for a limited period (I thought 1 week but haven't checked) so download them before they're gone (or fix it before ofc) |
* test: cover isColString with tests * test: cover remaining check util functions * refactor: improve importing of check module Co-authored-by: Sascha Depold <sdepold@ebay.com>
I've copy-pasted the relevant CI logs in my previous comment to make sure we don't lose them I've also resolved the merge conflict @sdepold if you have time, do you mind if I let you take over the remaining of this PR now that the TS part is done? |
|
The MariaDB test has failed in another PR https://github.com/sequelize/sequelize/runs/5320675964?check_suite_focus=true, which means it's also unrelated to this PR Only the postgres one remains |
I have seen the Postgres one as well I believe. At least the 'schema does not exist' in DAO tests. Not sure about this specific test |
Then I think we can merge this PR. I'll resolve the merge conflict Documented here: #14161 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few small remarks/questions before we merge this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
🎉 This PR is included in version 7.0.0-alpha.11 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Co-authored-by: ephys <zoe@ephys.dev> Co-authored-by: Rik Smale <13023439+WikiRik@users.noreply.github.com>
Pull Request Checklist
Please make sure to review and check all of these items:
npm run test
ornpm run test-DIALECT
pass with this change (including linting)?Description Of Change
Conversion of lib/utils to typescript and splitting into smaller chunks.