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 Typescript type inference for `returning()` to better support wildcard ("*") calls #3444

Closed
CodyEakins opened this issue Sep 17, 2019 · 6 comments · Fixed by #3446

Comments

@CodyEakins
Copy link

@CodyEakins CodyEakins commented Sep 17, 2019

Environment

Knex version: 0.19.3
Database + version: Postgres 11
OS: Windows 10, WSL

@lorefnon - Hopefully you can address this... Or at least tell me if I am wrong.

Bug

When using .returning() in a Query Builder chain, it's possible to pass in a wildcard ("*") to indicate that you want all of a table's columns returned with the result.

For example, when using Typescript Generics:

interface User {
  id: number;
  email: string;
  password: string;
}

const queryResult = await knex<User>("user").insert({
  email: "name@example.com",
  password: "passwordHash"
}).returning("*");

One expects queryResult's type to be User[] here, but it is actually Partial<User>[].

This result makes sense when calling returning() with a column identifier (e.g. - returning("id")), but not when it is called with a wildcard. When called with a wildcard, the result set is an array of fully qualified User objects, not partial ones.


Note:

I know you can pass the User type to returning() more directly, like so:

.returning<User>("*")

And get the expected result... But this bug is more about the type inference from the main Query Builder to returning().


Suggested Fix:

Maybe update the definition to something like:

returning<TResult2 = SafePartial<TRecord>[] | TRecord[]>(
  column: string | string[]
): QueryBuilder<TRecord, TResult2>;
@CodyEakins CodyEakins changed the title Fix type inference for `returning()` to better support wildcard ("*"} calls Fix Typescript type inference for `returning()` to better support wildcard ("*") calls Sep 17, 2019
@KristjanTammekivi

This comment has been minimized.

Copy link
Contributor

@KristjanTammekivi KristjanTammekivi commented Sep 18, 2019

Mildly OT but maybe enable a zero-parameter .returning() which automatically means return all columns as a possible fix for this

@lorefnon

This comment has been minimized.

Copy link
Collaborator

@lorefnon lorefnon commented Sep 19, 2019

#3446 adds overrides that special case '*' so that the returned value will be the complete record.

Maybe update the definition to something like:

returning<TResult2 = SafePartial[] | TRecord[]>(
column: string | string[]
): QueryBuilder<TRecord, TResult2>;

This is unlikely to get you what you want because T extends Partial<T> and hence T | Partial<T> is effectively Partial<T>.

@lorefnon

This comment has been minimized.

Copy link
Collaborator

@lorefnon lorefnon commented Sep 19, 2019

I also noticed that travis no longer run lint:types. Was this an intentional change ? cc. @kibertoad

@kibertoad

This comment has been minimized.

Copy link
Collaborator

@kibertoad kibertoad commented Sep 22, 2019

@lorefnon Huh, I wonder how did that happen. Opened #3450 to address that.

@CodyEakins

This comment has been minimized.

Copy link
Author

@CodyEakins CodyEakins commented Sep 22, 2019

Thanks for the fast and thorough response to this.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

@kibertoad kibertoad commented Oct 6, 2019

Released in 0.19.5

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.