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

QueryBuilder promise typings are incorrect. #1688

Closed
sripberger opened this issue Feb 15, 2020 · 0 comments
Closed

QueryBuilder promise typings are incorrect. #1688

sripberger opened this issue Feb 15, 2020 · 0 comments

Comments

@sripberger
Copy link

Using extends Promise<R> as is being done in Objection's QueryBuilder typings is incorrect, according to how Objection is implemented. The QueryBuilder does not actually have the Promise.prototype in its prototype chain. Saying that it does causes tsc to believe properties exist on them that actually don't.

Notably, the native Promise prototype in Node (as well as most browsers) implements finally. Objection's QueryBuilders do not implement finally, nor do they implement any other functionality of that type besides then and catch.

I'm not simply suggesting that they should add finally. The use of extends here is simply incorrect and will produce more problems like this if and when any additional functionality is added to the native Promise type.

TypeScript provides the PromiseLike<R> interface for these situations, though that interface only
requires then and not catch. What QueryBuilders implement as far as promise functionality is concerned is this:

interface CatchablePromiseLike<R> extends PromiseLike<R> {
    catch<FR = never>(
      onrejected?: ((reason: any) => FR | PromiseLike<FR>) | undefined | null
    ): Promise<R | FR>;
}

I will have a PR up shortly to make the necessary corrections, but this will be a breaking change for dependents written in TypeScript. Promise and PromiseLike (even CatchablePromiseLike) are not interchangeable, specifically because Promises implement additional stuff.

I understand there might me some unwillingness to make this change, but it is causing some problems for some people, notably me, as discussed here.

It's not something that should be rushed into, of course, but it would be nice to have this for the next major release.

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

No branches or pull requests

1 participant