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
Closed

Comments

@CodyEakins
Copy link

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
Copy link
Contributor

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
Copy link
Collaborator

#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
Copy link
Collaborator

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

@kibertoad
Copy link
Collaborator

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

@CodyEakins
Copy link
Author

Thanks for the fast and thorough response to this.

@kibertoad
Copy link
Collaborator

Released in 0.19.5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants