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.

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

Fix query builder promise typings #1689

Closed
wants to merge 1 commit into from

Conversation

sripberger
Copy link

Fixes for #1688.

Note that this is a breaking change for any dependents written in TypeScript. CatchablePromiseLike can be returned from async functions no problem, but as can be seen in the changes to examples.ts, any non-async function or variable that explicitly defines a Promise type will need to be changed.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.076% when pulling 124b06c on sripberger:fix/promise-typings into e646635 on Vincit:master.

@koskimas
Copy link
Collaborator

This is an insanely breaking change and we cannot merge it before 3.0, which is far in the future.

@sripberger
Copy link
Author

This is an insanely breaking change and we cannot merge it before 3.0, which is far in the future.

Which is totally reasonable. Though it is worth noting that it is kind of broken as-is. I just want to make sure everyone is aware of it so it can be fixed, far in the future if need be.

@koskimas
Copy link
Collaborator

Yeah, it's broken and this is s good change, but you are the first person to bring this up so it doesn't seem that critical.

@sripberger
Copy link
Author

Fair enough. I think what that means is that nobody is using finally on these, which isn't surprising. I don't think I've ever used it anywhere, to be honest.

Feel free to close and reopen when 3.0 is realistic, or whatever else seems appropriate. 👍

@capaj
Copy link
Contributor

capaj commented Jul 1, 2020

This is an insanely breaking change and we cannot merge it before 3.0, which is far in the future.

I don't think I agree with that. Sure, the typings change a bit, but no one is having Model.query().finally() in their code anyway even if the TS typings let them do that now, because this code will never run.
I can't imagine a situation where this could break a TS compilation of a code that's running correctly now,
If I were the maintainer, I'd merge this into 2.3.0, or 2.4.0 releases, but that just me.

@koskimas
Copy link
Collaborator

koskimas commented Aug 22, 2020

@capaj So you don't think anyone has created a function like this:

function getUser(id: number): Promise<User> {
  return User.query().findById(id);
}

I'm pretty sure most projects have.

@koskimas
Copy link
Collaborator

Supporting knex >= 0.95 requires us to make breaking changes, so I'll release a 3.0 soon. This will be fixed at the same time.

@koskimas
Copy link
Collaborator

Cant merge because of conflicts, but I copy pasted the relevant parts and this will be merged soon. Thank you for your contribution!

@koskimas koskimas closed this Jul 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants