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: Support nodejs v8 promise typing #3507

Closed
wants to merge 1 commit into from

Conversation

@alexkvak
Copy link

alexkvak commented Oct 31, 2019

No description provided.

@drwatsno

This comment has been minimized.

Copy link

drwatsno commented Oct 31, 2019

Wait.. I think it will break someone who already using this typings 🤔

@drwatsno

This comment has been minimized.

Copy link

drwatsno commented Oct 31, 2019

Anyway, could someone please explain this Pick<Promise<T>, "then" | "catch" | "etc"> typing? It looks confusing

@alexkvak

This comment has been minimized.

Copy link
Author

alexkvak commented Oct 31, 2019

@kibertoad I guess the solution of #3422 was wrong.

Could you explain why the previous version
interface ChainableInterface<T = any> extends Promise<T>
does the wrong things?

@lorefnon

This comment has been minimized.

Copy link
Collaborator

lorefnon commented Nov 1, 2019

@alexkvak can you please provide some context on what are you trying to fix ?

Wait.. I think it will break someone who already using this typings 🤔

Yes, finally will not be available in QueryInterface where as it is exposed.

@alexkvak

This comment has been minimized.

Copy link
Author

alexkvak commented Nov 1, 2019

I try to return node v8 support.
If you create new project from scratch with @types/node@8 and latest knex version, you become Typescript compile error.

Sorry, I can’t provide some code just node.

@lorefnon

This comment has been minimized.

Copy link
Collaborator

lorefnon commented Nov 1, 2019

Could you explain why the previous version
interface ChainableInterface<T = any> extends Promise
does the wrong things?

So, typescript interfaces are mutable. And it is possible for other packages (like @types/bluebird-global as was the case in #3422) to extend global interfaces and add keys etc. You can read more about this feature here.

ChainableInterface (what knex.select etc. returns) is not a Promise, but it exposes some promise methods which get delegated to the resolved promise when invoked.

When this happens then ChainableInterface extends Promise is wrong because it claims that it is compatible with Promise but in reality it is not. It is only exposing a subset of the methods. (Essentially you are claiming its domain is a superset where as its actually a subset)

This is usually not a problem in practice, but one case in particular (timeout) is problematic because Bluebird's timeout doesn't match the signature of an identically named function implemented in Knex.

There shouldn't really be a conflict because Bluebird's timeout is not actually available through QueryInterface even if it was the global Promise. But the conflict arises because of the lax type definition of QueryInterface.

Anyway, could someone please explain this Pick<Promise, "then" | "catch" | "etc"> typing? It looks confusing

@drwatsno

It is conceptually similar to Lodash.pick. You get only the keys that you pick mapped to the corresponding values:

type A = { a: string, b: string, c: string }
type B = Pick<A, "a" | "b">;
// Same as: 

type B = { a: string, b: string }
@lorefnon

This comment has been minimized.

Copy link
Collaborator

lorefnon commented Nov 1, 2019

@alexkvak Can you please share the error that you get ?

lorefnon added a commit that referenced this pull request Nov 1, 2019
was not available

- More closely align the ChainableInterface type to reality

Resolves #3507
lorefnon added a commit that referenced this pull request Nov 1, 2019
was not available

- More closely align the ChainableInterface type to reality

Resolves #3507
@lorefnon lorefnon closed this in af12940 Nov 2, 2019
@alexkvak alexkvak deleted the alexkvak:support-node8-promise branch Nov 12, 2019
@alexkvak

This comment has been minimized.

Copy link
Author

alexkvak commented Nov 12, 2019

@lorefnon thanks!

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 14, 2019

Released in 0.20.2

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