-
-
Notifications
You must be signed in to change notification settings - Fork 19
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
Add support for bind params #42
base: main
Are you sure you want to change the base?
Conversation
@PhilippSalvisberg here's a simple prototype that's backward compatible. |
@@ -12,6 +17,7 @@ export type RawValue = Value | Sql; | |||
* A SQL instance can be nested within each other to build SQL strings. | |||
*/ | |||
export class Sql { | |||
readonly bindParams = 0; |
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.
The only question to myself is whether values
should now become a getter that throws when bindParams > 0
? That would ensure the instance isn't incorrectly used without binding params.
@@ -90,6 +101,23 @@ export class Sql { | |||
return value; | |||
} | |||
|
|||
bind(...params: Value[]) { |
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.
Doesn't technically need this method if you know what you're doing, but the array length safety and merging with any non-params seems like a good pattern to enforce.
/** | ||
* A param that's expected to be bound later of included in `values`. | ||
*/ | ||
export const BIND_PARAM = Symbol("BIND_PARAM"); |
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.
Name options:
BIND_PARAMETER
BIND_VALUE
BIND
BIND_KEY
PARAM
Thanks for the follow-up. AFAIU the cases I mentioned in #41 (comment) (bind definitions e.g. for output parameters and I can see that this change simplifies some code for a series of similar SQL statements (bind/execute in a loop). However, using Maybe I didn't understand the intention and the use case. |
Good question. I was trying to make it easier to use const query = sql`UPDATE x SET y = ${BIND_PARAM} WHERE z = ${BIND_PARAM}`;
const result = db.executeMany(query.statement, [[1, 1], [2, 2], [3, 4]]); However, if you wanted better safety for the params: const result = db.executeMany(query.statement, [query.bind(1, 1), query.bind(2, 2), query.bind(3, 3)]); |
You are right that it's a very marginal improvement, and you could just use const query = sql`UPDATE x SET y = ${true} WHERE z = ${BIND_PARAM}`;
const result = db.executeMany(query.statement, [query.bind(1), query.bind(2), query.bind(3)]);
// Would be `[[true, 1], [true, 2], [true, 3]]`. |
Excellent example. Thanks for the explanation. I agree. This new feature can simplify the code, especially with many "fixed" bind values and a few delayed bindings. |
Follow up from #41 (comment), prototyping a backward compatible way to add bind parameters.
TL;DR adds
BIND_PARAM
symbol, when seen it increments a counter, counter is used in a new.bind
method to output a values list.