Skip to content

Conversation

@ibilux
Copy link

@ibilux ibilux commented Oct 25, 2025

Hello,

This is a work-in-progress for #91 aiming to add support for named parameters.
I’ve added tests, and they’re all passing so far. However, the current implementation is not perfect (far from it, actually), there’s definitely room for improvement.

Any feedback or suggestions for a cleaner or more efficient approach would be very welcome.

Thank you.

@DallasHoff DallasHoff marked this pull request as draft October 25, 2025 20:56
Copy link
Owner

@DallasHoff DallasHoff Oct 25, 2025

Choose a reason for hiding this comment

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

sqlTag and normalizeSql should probably just be combined into one function. I'd look into replacing sqlTag in the places it is used with the normalizeSql implementation. I'd keep the sqlTag naming though.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it would be better i think.

Copy link
Owner

Choose a reason for hiding this comment

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

I meant that normalizeSql should be renamed to sqlTag, and the old implementation of sqlTag should be deleted. normalizeSql can take its place.

Copy link
Owner

@DallasHoff DallasHoff left a comment

Choose a reason for hiding this comment

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

This is looking really good so far.

@ibilux
Copy link
Author

ibilux commented Oct 26, 2025

This is looking really good so far.

What do you think about using BindingSpec type instead of unknown[] | Record<string, unknown> in the Statement params:

export type Statement = {
	sql: string;
	params: unknown[] | Record<string, unknown>;
};

The BindingSpec is the bind type in db.exec:

bind: statement.params,

Referenced from SQLite wasm:
https://github.com/sqlite/sqlite-wasm/blob/31e4b6e478a3f10a11b477feb08f1e54b53c791a/index.d.ts#L318

@ibilux ibilux force-pushed the main branch 3 times, most recently from fc772d2 to 17c2228 Compare October 27, 2025 20:11
wip: testing the sqlite/wasm Bindable typing
@ibilux ibilux changed the title WIP: add named params support feat: add named params support Oct 28, 2025
@ibilux
Copy link
Author

ibilux commented Oct 28, 2025

  • I have a updated the named parameters extraction method for all cases with more tests.
  • I have add tests for the numbered positional parameters ?NNN.
  • The last commit is just for testing the sqlite/wasm Bindable types (we can revert it if it's out of this PR scope).

@ibilux
Copy link
Author

ibilux commented Oct 28, 2025

  • I have a updated the named parameters extraction method for all cases with more tests.
  • I have add tests for the numbered positional parameters ?NNN.
  • The last commit is just for testing the sqlite/wasm Bindable types (we can revert it if it's out of this PR scope).

We can also export the normalizeNamedParams as a helper function and give the user the option to chose if he wants to use the helper function of provide a valid named parameters object:

// Use the `normalizeNamedParams` helper function
await sql('INSERT INTO groceries (name) VALUES (:name)', normalizeNamedParams({ name: 'bread' }));

// Or provide a valid binding object instead
await sql('INSERT INTO groceries (name) VALUES (:name)', { ':name': 'bread' });

This provide more flexibility, but may introduce some confusion.

@DallasHoff
Copy link
Owner

Let's not use BindingSpec or BindableValue. Simpler types will suffice.

@DallasHoff
Copy link
Owner

If you finish making changes, please feel free to request re-review or remove the draft status from the PR.

@ibilux ibilux marked this pull request as ready for review November 11, 2025 07:38
This reverts commit d632b0b, reversing
changes made to 17c2228.
@ibilux
Copy link
Author

ibilux commented Nov 11, 2025

Let's not use BindingSpec or BindableValue. Simpler types will suffice.

I reverted that commit.

If you finish making changes, please feel free to request re-review or remove the draft status from the PR.

Yes, it should be ready for review.

@DallasHoff
Copy link
Owner

I also left this comment last night. Did you see it and does it make sense?

@ibilux
Copy link
Author

ibilux commented Nov 11, 2025

I also left this comment last night. Did you see it and does it make sense?

I didn't notice it. I think that would be better.

I meant that normalizeSql should be renamed to sqlTag, and the old implementation of sqlTag should be deleted. normalizeSql can take its place.

The last commit merges normalizeSql into sqlTag, now there is one entry for handling SQL queries.

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.

2 participants