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

Property 'timeout' does not exist on type 'QueryBuilder' #3422

Closed
3mini opened this issue Sep 2, 2019 · 16 comments · Fixed by #3427 or #3502
Closed

Property 'timeout' does not exist on type 'QueryBuilder' #3422

3mini opened this issue Sep 2, 2019 · 16 comments · Fixed by #3427 or #3502

Comments

@3mini
Copy link
Contributor

@3mini 3mini commented Sep 2, 2019

Environment

Knex version: 0.19.3
Database + version: MySQL 5.7.22
OS: 10.14.6

If issue is about TypeScript definitions, tag @lorefnon.

Bug

  1. Explain what kind of behaviour you are getting and how you think it should do

    I'm using Typescript Version 3.5.1.
    When I upgrade knex version from 0.17.3 to 0.19.3, I got a compile error regarding timeout.
    It worked well before. 🙃

  2. Error message

    Property 'timeout' does not exist on type 'QueryBuilder<unknown, 
    DeferredKeySelection<unknown, never, false, {}, false, {}, never>[]>'
    
@kibertoad

This comment has been minimized.

Copy link
Collaborator

@kibertoad kibertoad commented Sep 2, 2019

It was a bluebird method. We no longer support bluebird.
That said, timeout functionality is, indeed, useful. Wonder if we handle it otherwise right now.

@Heikkohoo

This comment has been minimized.

Copy link

@Heikkohoo Heikkohoo commented Sep 3, 2019

Same issue with the map - property. It works with 0.17.6 knex but breaks with 0.18.0 and newer.

Property 'map' does not exist on type 'QueryBuilder<unknown, (DeferredKeySelection<unknown, string, true, {}, boolean, {}, unknown> | DeferredKeySelection<unknown, "id", true, {}, false, {}, never>)[]>'

@elhigu

This comment has been minimized.

Copy link
Member

@elhigu elhigu commented Sep 3, 2019

It was a bluebird method. We no longer support bluebird.
That said, timeout functionality is, indeed, useful. Wonder if we handle it otherwise right now.

IIRC http://knexjs.org/#Builder-timeout is not from bluebird, but actually implemented in knex. It actually cancels the query if timeout is triggered.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

@kibertoad kibertoad commented Sep 4, 2019

@Heikkohoo map is definitely gone for good, just await results and call map on array that you get.
Please check if timeout actually works on latest version if you add ts-ignore, if it does, we need to add it to our knex types.

@Heikkohoo

This comment has been minimized.

Copy link

@Heikkohoo Heikkohoo commented Sep 5, 2019

@kibertoad thanks. Got the .map thinghy working.

kibertoad added a commit that referenced this issue Sep 5, 2019
- Add missing typedefs for or and timeout (Closes #3422)
- Move clone from QueryInterface to QueryBuilder
@kibertoad

This comment has been minimized.

Copy link
Collaborator

@kibertoad kibertoad commented Sep 9, 2019

@3mini Types were fixed in 0.19.4 that was just published.

@3mini

This comment has been minimized.

Copy link
Contributor Author

@3mini 3mini commented Sep 10, 2019

@kibertoad Thanks 🙏

@3mini

This comment has been minimized.

Copy link
Contributor Author

@3mini 3mini commented Sep 11, 2019

@lorefnon There's still a compile error as below.

timeout(ms: number, options: {cancel?: boolean}): QueryBuilder<TRecord, TResult>;
                            ~~~~~~~~~~~~~~~~~~~~~~~~~~~
    An argument for 'options' was not provided.

According to the document, the second argument is optional as in the example.

knex.select().from('books').timeout(1000)

But options is not optional argument anymore.

Instead of an empty option ({}), I think it's better to do this.

(If it need more options, cancel might as well be optional.)

timeout(ms: number, options?: { cancel: boolean }): QueryBuilder<TRecord, TResult>;

What do you think?

@yigaldviri

This comment has been minimized.

Copy link

@yigaldviri yigaldviri commented Oct 27, 2019

I'm having the same error as in the title.
Migrated to 0.20.0 from 0.15.1
I'm using Bluebird in my project instead of the native Promise (which is overrided by Bluebird) so there's a conflict between my promise's timeout and Knex's timeout.

Interface 'QueryBuilder<TRecord, TResult>' incorrectly extends interface 'ChainableInterface<Resolve<TResult>>'.
  Types of property 'timeout' are incompatible.
    Type '(ms: number, options?: { cancel?: boolean | undefined; } | undefined) => QueryBuilder<TRecord, TResult>' is not assignable to type '(ms: number, message?: string | Error | undefined) => Bluebird<Resolve<TResult>>'.
      Types of parameters 'options' and 'message' are incompatible.
        Type 'string | Error | undefined' is not assignable to type '{ cancel?: boolean | undefined; } | undefined'.
          Type 'string' is not assignable to type '{ cancel?: boolean | undefined; } | undefined'.

Is there a way to use Bluebird as my project's Promise and still use Knex?

@kibertoad

This comment has been minimized.

Copy link
Collaborator

@kibertoad kibertoad commented Oct 27, 2019

@yigaldviri timeout method in Knex does not come from Bluebird, it is implemented internally, hence signature is not compatible.
As long as you don't expect Knex to return Bluebird promises to you, you should be able to mix Knex native promises with your in-project Bluebird promises. Probably simplest way to achieve that would be to wrap all knex interactions within repository files that would only expose Bluebird promises outside.
That said, if you would ever start using async/await, aren't you going to hit same issue of your promises not being Bluebird promises?

@kibertoad

This comment has been minimized.

Copy link
Collaborator

@kibertoad kibertoad commented Oct 27, 2019

I wonder if signature will become compatible if we make options an optional argument. I see no reasons why not to. @lorefnon, what do you think?

@yigaldviri

This comment has been minimized.

Copy link

@yigaldviri yigaldviri commented Oct 27, 2019

@kibertoad - the thing is that i do "expect Knex to return Bluebird promises to you". I'm using Bluebird instead of native Promises sort of like this: global.Promise = BluebirdPromise; and that's why I'm getting this error. Is there any thing I can do to solve this?

Also, I'm using async/await and don't have a problem with Bluebird

@kibertoad

This comment has been minimized.

Copy link
Collaborator

@kibertoad kibertoad commented Oct 27, 2019

@yigaldviri Right, so that forces all calls to new Promise to actually instantiate a Bluebird promise? Other than casting Knex native promises to Bluebird promises, I don't think there is a possible solution for existing version; however, we are definitely going to look into at least addresing the .timeout use-case.

@lorefnon

This comment has been minimized.

Copy link
Collaborator

@lorefnon lorefnon commented Oct 28, 2019

In response to @3mini's comment we had already made the options argument optional, and it was released in 0.20.0.

However with 0.20.0 the error reported by @yigaldviri should not be possible (absent external interference with types) because ChainableInterface no longer extends Bluebird so ChainableInterface#timeout should not exist.

@yigaldviri Can you please let us know if any of the following are true:

  1. Do you also have @types/knex installed ?
  2. Do you have @types/bluebird-global (or something similar) installed, which augments the type of global Promise ?

As @kibertoad mentioned, we have found global promise->bluebird substitution to not be a viable solution because irrespective of what you set global.Promise to be, async functions will always return the native one unless you transpile all async/await usage as well which incurs a non-trivial performance hit.

@yigaldviri

This comment has been minimized.

Copy link

@yigaldviri yigaldviri commented Oct 28, 2019

Hi @lorefnon ,

  1. I'm not using @types/knex.
  2. I'm using @types/bluebird-global
lorefnon added a commit that referenced this issue Oct 29, 2019
- Update QueryInterface to proxy to only standard promise methods
- Update QueryInterface type to be explicit about the methods being
  proxied

This primarily handles pollution of global Promise (either the type or
the global object at runtime) by other libraries.

Fixes: #3422 (comment)
lorefnon added a commit that referenced this issue Oct 29, 2019
- Update QueryInterface type to be explicit about the methods being
  proxied

This primarily handles pollution of global Promise type by other
libraries which can break Knex typings.

Resolves: #3422 (comment)
kibertoad added a commit that referenced this issue Oct 29, 2019
Update QueryInterface to proxy to only standard promise methods
Update QueryInterface type to be explicit about the methods being
proxied
This primarily handles pollution of global Promise (either the type or
the global object at runtime) by other libraries.

Fixes: #3422
@kibertoad

This comment has been minimized.

Copy link
Collaborator

@kibertoad kibertoad commented Oct 29, 2019

Released in 0.20.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.