-
Notifications
You must be signed in to change notification settings - Fork 641
Fix idColumn type #2744
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
base: main
Are you sure you want to change the base?
Fix idColumn type #2744
Conversation
@@ -1442,7 +1442,7 @@ declare namespace Objection { | |||
QueryBuilder: typeof QueryBuilder; | |||
|
|||
tableName: string; | |||
idColumn: string | string[]; | |||
idColumn: null | string | string[]; |
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.
Should probably be last, not first? Also, it was suggested here to use never instead of null? What's more suitable?
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.
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.
@geeksilva97 @lehni
null - is not safe from the point of view of logic and types!
if you do not explicitly specify returning (for example returning(*)) then this will implicitly lead to the fact that it will try to use null (since the method will return null) and this will lead to an error at runtime!
see #2693 (comment)
that is, objection will compile an invalid SQL query!
therefore never is necessary - so that at the compilation stage (on the js side) there is an error, and an invalid SQL query does not sent to the database side!
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.
what does 'returning(*)' mean?
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.
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.
Should probably be last, not first? Also, it was suggested here to use never instead of null? What's more suitable?
I don't think the order matters. Just let me know if you want me to change it
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.
what does 'returning(*)' mean?
this is a normal sql expression....
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.
related threads
#2746 (comment)
#2693 (comment)
Fixes #2693