Skip to content

sqlite: db.prepare(sql).set* chain - #61263

Closed
mike-git374 wants to merge 4 commits into
nodejs:mainfrom
mike-git374:main
Closed

sqlite: db.prepare(sql).set* chain#61263
mike-git374 wants to merge 4 commits into
nodejs:mainfrom
mike-git374:main

Conversation

@mike-git374

@mike-git374mike-git374 commented Jan 3, 2026

Copy link
Copy Markdown
Contributor

This implements Proposal 1 of issue #61235

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/sqlite

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem. labels Jan 3, 2026

@himself65himself65 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you please update the test case?

@mike-git374

Copy link
Copy Markdown
ContributorAuthor

I updated all tests with statement.set* functions

@Renegade334Renegade334 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm -0.5 on this, as repeated calls across the JS-C++ boundary to set statement options is a very much less efficient paradigm than the alternative proposal, which was to allow options to be passed to prepare(). Passing a consolidated options object to set statement options also matches the pattern already in use in the database constructor.

@mike-git374

mike-git374 commented Jan 3, 2026

Copy link
Copy Markdown
ContributorAuthor

I updated the issue to address this and now recommend db.prepare(sql[, options]) as an alternative to or in addition to this PR

@codecov

codecovBot commented Jan 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.53%. Comparing base (95852d7) to head (302305c).
⚠️ Report is 41 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #61263 +/- ##
==========================================
- Coverage 88.54% 88.53% -0.02% 
==========================================
Files 704 704 Lines 208753 208757 +4 Branches 40280 40288 +8 ==========================================
- Hits 184847 184820 -27 - Misses 15907 15952 +45 + Partials 7999 7985 -14 
Files with missing linesCoverage Δ
src/node_sqlite.cc80.20% <100.00%> (+0.03%)⬆️

... and 29 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mike-git374

Copy link
Copy Markdown
ContributorAuthor

Closing in favor of Proposal 2

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.needs-ciPRs that need a full CI run.sqliteIssues and PRs related to the SQLite subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@mike-git374@nodejs-github-bot@Renegade334@himself65