Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork19
Add support for bind params#42
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 ourterms of service andprivacy 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?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
blakeembrey commentedApr 21, 2024
@PhilippSalvisberg here's a simple prototype that's backward compatible. |
| * A SQL instance can be nested within each other to build SQL strings. | ||
| */ | ||
| exportclassSql{ | ||
| readonlybindParams=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 whethervalues should now become a getter that throws whenbindParams > 0? That would ensure the instance isn't incorrectly used without binding params.
| returnvalue; | ||
| } | ||
| 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`. | ||
| */ | ||
| exportconstBIND_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_PARAMETERBIND_VALUEBINDBIND_KEYPARAM
PhilippSalvisberg commentedApr 22, 2024
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. |
blakeembrey commentedApr 22, 2024
Good question. I was trying to make it easier to use constquery=sql`UPDATE x SET y =${BIND_PARAM} WHERE z =${BIND_PARAM}`;constresult=db.executeMany(query.statement,[[1,1],[2,2],[3,4]]); However, if you wanted better safety for the params: constresult=db.executeMany(query.statement,[query.bind(1,1),query.bind(2,2),query.bind(3,3)]); |
blakeembrey commentedApr 22, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
You are right that it's a very marginal improvement, and you could just use constquery=sql`UPDATE x SET y =${true} WHERE z =${BIND_PARAM}`;constresult=db.executeMany(query.statement,[query.bind(1),query.bind(2),query.bind(3)]);// Would be `[[true, 1], [true, 2], [true, 3]]`. |
PhilippSalvisberg commentedApr 22, 2024
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_PARAMsymbol, when seen it increments a counter, counter is used in a new.bindmethod to output a values list.