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

feat: sql.join #905

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

feat: sql.join #905

wants to merge 10 commits into from

Conversation

DanielFGray
Copy link

@DanielFGray DanielFGray commented Jun 27, 2024

closes #807, to add sql.join:

let dynamicFilters = [
  sql`somecol = ${somevalue}`,
  sql`col2 = ${v2}`
];
const dynamicColumns = [sql('somecol'), sql('col2')];
await sql`
  select ${sql.join(sql`, `, dynamicColumns)} from t
  where ${sql.join(sql` and `, dynamicFilters)};
`;

i'm not 100% sure the types are right, and I couldn't get the tests to run, so those probably need some work

@DanielFGray DanielFGray marked this pull request as ready for review June 27, 2024 15:36
@DanielFGray
Copy link
Author

DanielFGray commented Jun 27, 2024

if you have a weird workaround you can try this fix in your own projects

npm i github:danielfgray/postgres.js

@Destreyf @davepar your feedback would be appreciated!

src/index.js Outdated Show resolved Hide resolved
@DanielFGray
Copy link
Author

DanielFGray commented Jun 27, 2024

I also really want to enable currying to allow things like

const and = sql.join(sql` and `)
and(sql`...`, ...fragments)

which could be as simple as

- function join(sep, xs) {
+ function join(sep, ...xs) {
+   if (!xs) return join.bind(null, sep)
    return xs.flatMap((x, i) => {

but this would probably also need further bike-shedding

I also wonder if the API could be further improved to simply allow

const and = sql.join` and `
const params = and(f1, f2, f3)
// would work the same as
const params = sql.join` and `(f1, f2, f3)

I'm not sure that postgres.js goals are to be a whole query builder, but it seems like we can add a lot of expressivity with a few tweaks to a simple feature

but currying is likely to increase the complexity of the TS types and perhaps make the API more complex and difficult to document
⚖️

src/index.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@phosmium
Copy link

neat feature, hope this makes it in +rep

@dhardtke
Copy link

I agree though that it should not be called "join" to avoid confusion with SQL's JOIN statements. In my opinion, "sql.concat" is a better choice.

@maumercado
Copy link

maumercado commented Jul 17, 2024

solid addition 👍

@DanielFGray
Copy link
Author

I agree though that it should not be called "join" to avoid confusion with SQL's JOIN statements. In my opinion, "sql.concat" is a better choice.

I've gone back and forth on this in my head, and while there's definitely potential for ambiguity, join here is behaving the same as it does on arrays

It would be even more surprising behavior to me to have something called concat behaving as Array#join, and if a user is confused on what sql.join does the docs would explain. I also think actually seeing the code in context would pretty well indicate it has nothing to do with the SQL clause

So all this is to say, semantically, I think join is the best name for this feature

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.

sql.join() for building dynamic queries
6 participants