fix: workaround sequelize types in tests #226
Conversation
|
Is there an issue in the main repo tracking this typing error? If not, can you please open one? (And mention in it the comment you wrote in the code) |
Yes - I changed it to Re making an issue in the main repo - I'll try to find some time to get a repro that doesn't involve all the baggage of this test, but I don't actually use sequelize itself so might take a little while to get to it. |
test/storage/sequelize.test.ts
Outdated
| // TODO [sequelize@>=6.0.0] remove when sequelize fixes the types bug | ||
| // a change in the types of sequelize@next makes `describe` a static function - see https://github.com/sequelize/sequelize/issues/12296 | ||
| // eslint-disable-next-line @typescript-eslint/ban-ts-ignore | ||
| // @ts-ignore | ||
| const describeModel = (model: sequelize.Model) => model.describe(); |
There was a problem hiding this comment.
| // TODO [sequelize@>=6.0.0] remove when sequelize fixes the types bug | |
| // a change in the types of sequelize@next makes `describe` a static function - see https://github.com/sequelize/sequelize/issues/12296 | |
| // eslint-disable-next-line @typescript-eslint/ban-ts-ignore | |
| // @ts-ignore | |
| const describeModel = (model: sequelize.Model) => model.describe(); | |
| const describeModel = (model: typeof sequelize.Model) => model.describe(); |
There was a problem hiding this comment.
This doesn't work because we don't have a typeof Model, we have a Model - this is called with the return value of model.sync() - from sequelize: https://github.com/sequelize/sequelize/blob/b2bccb8aa3faeb6a891fd9f7d3858ebff5b9c32f/types/lib/model.d.ts#L1604-L1608
I'm going to change to any for now. It might be that we are using describe wrong and it's working by chance, but this behaviour is currently going to make every PR fail, so I've changed the todo to TODO [>=3.0.0], i.e. it's on us to fix before we ship v3.
There was a problem hiding this comment.
Hmm, great catch, sorry about that, I am pretty sure the sync type signature is the one wrong then. I will look into it later.
One of the travis jobs runs
npm install sequelize@next, which now breaks us. This gets things green again with a workaround. I've put in a todo that will become a lint error when we're on a stable version of sequelize using expiring-todo-comments, to make sure we remember to clean up the workaround.