Skip to content

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.

Sign up for GitHub

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

Open
geeksilva97 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix idColumn type #2744

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion typings/objection/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1442,7 +1442,7 @@ declare namespace Objection {
QueryBuilder: typeof QueryBuilder;

tableName: string;
idColumn: string | string[];
idColumn: null | string | string[];
Copy link
Copy Markdown
Collaborator

@lehni lehni May 4, 2025

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?

#2693 (comment)

Copy link
Copy Markdown

@salisbury-espinosa salisbury-espinosa May 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#2746 (comment) @lehni @geeksilva97

Copy link
Copy Markdown

@salisbury-espinosa salisbury-espinosa May 27, 2025

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!

Copy link
Copy Markdown
Author

@geeksilva97 geeksilva97 May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does 'returning(*)' mean?

Copy link
Copy Markdown
Author

@geeksilva97 geeksilva97 May 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get this never thing when the doc explicitly says to use null

image

Maybe docs should be changed as well

Copy link
Copy Markdown
Author

@geeksilva97 geeksilva97 May 27, 2025

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?

#2693 (comment)

I don't think the order matters. Just let me know if you want me to change it

Copy link
Copy Markdown

@salisbury-espinosa salisbury-espinosa May 27, 2025 •
edited
Loading

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....

  1. https://github.com/Vincit/objection.js/blob/main/lib/queryBuilder/operations/InsertOperation.js#L42
  2. https://vincit.github.io/objection.js/recipes/returning-tricks.html
  3. https://www.postgresql.org/docs/17/dml-returning.html

Copy link
Copy Markdown

@salisbury-espinosa salisbury-espinosa May 27, 2025

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)

jsonSchema: JSONSchema;
relationMappings: RelationMappings | RelationMappingsThunk;
modelPaths: string[];
Expand Down