-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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 types for "returning" methods #5031
Conversation
test-tsd/types.test-d.ts
Outdated
knexInstance | ||
.table<User>('users') | ||
.insert({ id: 10, active: true }) | ||
.onConflict('id') | ||
.ignore() | ||
.debug(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why was debug added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was a copy paste thing from the previous test, it's removed
Type tests are failing: https://github.com/knex/knex/runs/5271463571?check_suite_focus=true |
0f07695
to
f3f48bf
Compare
fixed, sorry I missed those. |
DeferredKeySelection<User, 'id', true, {}, true, {}, never>[] | ||
> | ||
>( | ||
expectAssignable<QueryBuilder>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it no longer possible to have a more granular type here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is possible but the type would be "wrong" anyways due to the issue that I mentioned before.
Insert/update methods without "returning" still have pre v1 signature of returning a number[]
but they are not actually returning that. They return a "Result" object. I'm not sure if this is something that happens with just pg or it happens with every backend db.
const users = await knex
.from<Contact>("contact").insert([{
email: "xxxx@gmail.com",
first_name: "jeff"
}])
// users is suposed to be a number[];
console.log(users)
// Result {
// command: 'INSERT',
// rowCount: 2,
// oid: 0,
// rows: [],
// fields: [],
// ...
// }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gotcha
thank you! |
Can we have this released? It's a bit of a blocker for our knex upgrade |
I will release it tomorrow |
This is still broken on |
Closes #4942
With the release of v1, the methods for
returning
changed a bit. However, the type definitions were not updated.Reviewing the rest of the types I found that the
onConflict
clause was setting the Result type with the passed conflicting columns. This looked very strange and at least when using postgres as the backend, this is not the case.Also, (this is another issue) when not using the "returning", the types specify that the result is
number[]
. However (at least using pg) a Result object is returned instead.