Type definitions: QueryBuilder is no longer a Promise

[notaphplover]

Environment

Knex version: 0.20.1
Database + version: N/A.
OS: N/A.

This is a Typescript definition issue related, so I'm tagging @lorefnon.

Bug

Typescript is unable to threat a QueryBuilder as a Promise. On knex 0.19.5 it was possible to create a query like this one:

import * as Knex from 'knex';

export class SampleClass {
  /**
   * Query Builder.
   */
  protected _dbConnection: Knex;
  
  /**
   * Deletes an entity from its id.
   * @param id Id of the entity to delete.
   * @returns Promise of entity deleted.
   */
  public delete(id: string | number): Promise<any> {
    return this._dbConnection
      .from('SampleTable')
      .where('id', id)
      .del();
  }
}

But now it's no longer possible:

Conversion of type 'QueryBuilder<unknown, number>' to type 'Promise<any>' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first. Property '[Symbol.toStringTag]' is missing in type 'QueryBuilder<unknown, number>' but required in type 'Promise<any>'.

In order to fix this I have to use this ugly workaround:

import * as Knex from 'knex';

export class SampleClass {
  /**
   * Query Builder.
   */
  protected _dbConnection: Knex;
  
  /**
   * Deletes an entity from its id.
   * @param id Id of the entity to delete.
   * @returns Promise of entity deleted.
   */
  public delete(id: string | number): Promise<any> {
    return this._dbConnection
      .from('SampleTable')
      .where('id', id)
      .del() as unknown as Promise<any>;
  }
}

Is this a bug or it's the expected behavior?

[elhigu]

Your original typing was wrong. You are returning QueryBuilder from your method, not Promise. QueryBuilder is not a Promise and it should not be treated as such. For example when you return a QueryBuilder the query has not been executed and the QueryBuilder instance can still be modified.

Correct typing would be:

public delete(id: string | number): QueryBuilder {
...

If you want to return a Promise you must convert QueryBuilder to Promise. You can do it by making delete to async method, which will implicitly resolve QueryBuilder (thenable) to Promise.

  public async delete(id: string | number): Promise<any> {
    return this._dbConnection
      .from('SampleTable')
      .where('id', id)
      .del();
  }

Or you can resolve QueryBuilder manually to Promise

  public delete(id: string | number): Promise<any> {
    return Promise.resolve(this._dbConnection
      .from('SampleTable')
      .where('id', id)
      .del());
  }

[notaphplover]

@elhigu Thank you so much for you quick response.

Your original typing was wrong. You are returning QueryBuilder from your method, not Promise. QueryBuilder is not a Promise and it should not be treated as such. For example when you return a QueryBuilder the query has not been executed and the QueryBuilder instance can still be modified.

In Typescript, Promise is an interface. Interface implementation in Typescript can be achieved in two ways: explicitly and implicitly. Typescript, as an OOP language supports polymorphism. Keeping this in mind, knex 0.19.5 implemented implicitly Promise, but this implementation is no longer supported in knex 0.20.1, ¿is this made on purpose?

Correct typing would be:

public delete(id: string | number): QueryBuilder {
...

If you want to return a Promise you must convert QueryBuilder to Promise. You can do it by making delete to async method, which will implicitly resolve QueryBuilder (thenable) to Promise.

  public async delete(id: string | number): Promise<any> {
    return this._dbConnection
      .from('SampleTable')
      .where('id', id)
      .del();
  }

Thats true, but it's an unnecesary limit. Digging in the type definitions it seems you really wanted to implement the Promise interface:

interface ChainableInterface<T = any> extends Pick<Promise<T>, "then" | "catch" | "finally">
...

¿is there any reason for not implementing it?

I've been doing some tests to verify that QueryBuilder implements the Promise interface (https://repl.it/@RobertoPintos/knex-test), unfortunatelly it's true that the getter implementation of the Symbol.toStringTag is missing (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/toStringTag)

¿Do you consider interesting implementing this getter? QueryBuilder.toString is already implemented so it should be easy and this would allow to consider a QueryBuilder as a Promise

Or you can resolve QueryBuilder manually to Promise

  public delete(id: string | number): Promise<any> {
    return Promise.resolve(this._dbConnection
      .from('SampleTable')
      .where('id', id)
      .del());
  }

That approach would be correct, but this change has a cost at runtime and there is no need to do that.

[elhigu]

@lorefnon can decide about this, but in my opinion it is dangerous to thing that you are saying that object returned from method is a Promise, when the returned object is actually not a Promise and does not behave like one.

Though you are right that one can think about it from many sides, but I don't see any upside of making QueryBuilder to pretend being a promise - only confusion.

Thats true, but it's an unnecesary limit.

Typescript is pretty much all about adding unnecessary limits on top of javascript 😅 First adding types and then not breaking that added typesafety does not sound like an unnecessary limitation.

Digging in the type definitions it seems you really wanted to implement the Promise interface:

We really would have wanted to implement thenable interface. It would be then also semantically correct describing QueryBuilder's expected behaviour.

That approach would be correct, but this change has a cost at runtime and there is no need to do that.

Actually there is no extra cost, because you need to convert it to promise in either way at some point to execute that query. Cost is only moved to earlier and query execution starts right away when method is called.

It is also necessary if you want to get Promise returned instead of thenable. Otherwise you are better off returning QueryBuilder type which is the real type in this case. I don't see the logic why you want to make your app to think that you are returning a Promise when it is not a promise and does not behave like one.

I'm sure you know the difference what happens when you return a promise or thenable, but I'll add this example of expected behaviour anyways just to be extra clear:

 const delPromise = delete(1); // triggers the query
 await delPromise; // query result is returned when promise execution is finished
 await delPromise; // returns the same query result as above

 const delThenable = delete(1); // returns a query builder, which is ready to kill
 await delThenable; // triggers the query and returns the result
 await delThenable; // triggers the query again and returns the result of second query

Anyways this was my view and I leave this to typescript maintainers to decide what to do 👍

[elhigu]

Is PromiseLike the actual interface that thenable's should implement? Thank API looks like thenable http://jbeard4.github.io/SCION-CORE/interfaces/__nvm_versions_v8_4_0_lib_node_modules_typedoc_node_modules_typescript_lib_lib_es5_d_.promiselike.html

[notaphplover]

@elhigu now I understand your point of view. You are absolutely right. Sorry, this was a bit confusing. Sincerely, I knew that theenables do not need to conform the Promises/A+ spec, but I did not get it in this comment 😅.

Now everything is clear, I will reconsider my code. But maybe docs should be changed, some parts of them seem to be a bit confusing.

[notaphplover]

@elhigu I will keep the issue opened until the Typescript maintainers answer about the type question (on my opinion, PromiseLike is a good choice), but pls feel free to close this as soon as you want.

[lorefnon]

I agree with @elhigu that returning query builder from something whose return type is Promise is confusing esp. for someone new.

I am not opposed to adding a Symbol.toStringTag getter (which will return something like "QueryBuilder" and not "Promise") - though signature compatibility with Promise may not be the ideal reason for doing this.

Depending on PromiseLike seems like a good idea if you only need the then method, but I think it is better to just have the return type as Knex.QueryBuilder so people can chain them further if need be.

But maybe docs should be changed, some parts of them seem to be a bit confusing.

I don't quite see what is confusing in there. This seems well described to me:

[.then] Coerces the current query builder chain into a promise state, accepting the resolve and reject handlers as specified by the Promises/A+ spec ...

[notaphplover]

But maybe docs should be changed, some parts of them seem to be a bit confusing.

I don't quite see what is confusing in there. This seems well described to me:

[.then] Coerces the current query builder chain into a promise state, accepting the resolve and reject handlers as specified by the Promises/A+ spec ...

That part describes well the behavior of the queries, but what I miss is what @elhigu explained previously: successive calls to QueryBuilder.then will generate different query executions. I don't find that in the docs. Maybe I'm missing something.

[notaphplover]

I think we can close the issue now. Thank you so much for the quick responses and knowledge :)

[elhigu]

[.then] Coerces the current query builder chain into a promise state, accepting the resolve and reject handlers as specified by the Promises/A+ spec ...

I agree... it is not correct, since "coerces" means that it will modify QueryBuilder to behave like a promise, which it does not.

Originally QueryBuilder was meant to be implemented to work like that, but I'm glad it never was implemented correctly. There would have been internal promise which is resolved just once and its result would have been returned on every .then call.

I think that documentation should be fixed to say that every call to then .then executes the query builder and returns a promise for the result.