Skip to content
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 taggedExecute helper #1539

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add taggedExecute helper #1539

wants to merge 3 commits into from

Conversation

sidorares
Copy link
Owner

@sidorares sidorares commented Mar 22, 2022

RFC at this stage, but I think the feature was mentioned / requested few times in the past and looks like a safe and useful addition

At this stage I intentionally adding this to "real" prepared statements only ( AKA .execute() ) and also to promise wrapper only. Also intentional is that fields part is not returned, no more const [rows] = await ... destructuring. If fields are needed always possible to use slightly lower level .execute()

partially inspired by https://github.com/google/zx

examples:

import mysql from 'mysql2/promise';
const pool = mysql.createPool({ user: 'root', password: 'test' });
const $ = pool.taggedExecute();
const input = 'test\" --';
const rows = await $`select ${input} as solution`;
import mysql from 'mysql2/promise';
const connection = await mysql.createConnection({ user: 'root', password: 'test' });
const $ = connection.taggedExecute();
const input = 'test\" --';
const rows = await $`select ${input} as solution`;
  • base functionality
  • unit tests
  • documentation update
  • collect feedback on API design

@github-actions
Copy link
Contributor

Coverage report

The coverage rate is 89.13920056100982%

The branch rate is 84.98563218390804%

100% of new lines are covered.

@sidorares
Copy link
Owner Author

sidorares commented Mar 24, 2022

Something that I don't like but not sure how to handle well in a backwards compatible way.

With addition of tagged templates based PS there will be 4 ways of sending parametrised sql, which potentially makes everything very confusing:

// 1 - client side interpolation, PS-like but not "real" prepared statements:
const [rows1, columns1] = await connection.query('select ? as solution', ["parameter"]);

// 2 - real PS, prepare(sql) + execute(param) command the first time its called, execute(param) after
//  api aims to mimic (1) but due to limitations of prepared statements there are differences
const [rows2, columns2] = await connection.execute('select ? as solution', ["parameter"]);

// 3 - named placeholders. Same as (2) but a bit easier to read
const [rows3, columns3] = await connection.execute('select :parameter as solution', { parameter: "parameter"});

// 4 - tagged template
const rows = await sql`select ${"parameter"} as solution`;

Is having (3) and (4) too much? Given that (3) is relatively unknown, maybe we should deprecate it in favour of (4) ?

@sidorares
Copy link
Owner Author

might be a good idea to model api to match https://github.com/porsager/postgres#await-sql---result

not sure though if it belongs to base driver or independent wrapper library

@sidorares
Copy link
Owner Author

add to docs: this extension should provide syntax highlighting with no additional config - https://marketplace.visualstudio.com/items?itemName=frigus02.vscode-sql-tagged-template-literals

@sidorares
Copy link
Owner Author

I'm leaning towards not merging this and creating separate package instead

@sidorares
Copy link
Owner Author

Existing external modules as of Nov 2023:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant