-
Notifications
You must be signed in to change notification settings - Fork 642
Description
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.