Skip to content

sqlite: rename DatabaseSync to Database - #65988

Open
araujogui wants to merge 6 commits into
nodejs:mainfrom
araujogui:sqlite-rename-database-class
Open

araujogui wants to merge 6 commits into
nodejs:mainfrom
araujogui:sqlite-rename-database-class

Conversation

@araujogui

@araujogui araujogui commented Sep 11, 2026

Copy link
Copy Markdown
Member
  • Rename the DatabaseSync class to Database and StatementSync to Statement.
  • Keep old names as aliases with doc-only deprecations.

See #57445 (comment)

Copilot AI lite review requested due to automatic review settings September 11, 2026 17:12
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/performance
  • @nodejs/security-wg
  • @nodejs/sqlite
  • @nodejs/tsc

This comment was marked as low quality.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Sep 11, 2026
@araujogui
araujogui force-pushed the sqlite-rename-database-class branch from d90b1f9 to b52ec3e Compare September 11, 2026 17:20
@cjihrig

cjihrig commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

I didn't review the code fully, but does this leave an alias behind? If not, this will break every single current user.

@araujogui

Copy link
Copy Markdown
Member Author

I didn't review the code fully, but does this leave an alias behind? If not, this will break every single current user.

No, it doesn't. I will add it later

Comment thread lib/sqlite.js Outdated
@codecov

codecov Bot commented Sep 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.00000% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.29%. Comparing base (3a15c5d) to head (2b3ccb5).
⚠️ Report is 24 commits behind head on main.

Files with missing lines Patch % Lines
src/node_sqlite.cc 90.64% 16 Missing ⚠️
src/node_sqlite.h 38.46% 8 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main   #65988    +/-   ##
========================================
  Coverage   90.29%   90.29%            
========================================
  Files         790      789     -1     
  Lines      272531   272879   +348     
  Branches    52037    52111    +74     
========================================
+ Hits       246070   246405   +335     
- Misses      16915    16928    +13     
  Partials     9546     9546            
Files with missing lines Coverage Δ
lib/sqlite.js 100.00% <100.00%> (ø)
src/node_sqlite.h 87.27% <38.46%> (ø)
src/node_sqlite.cc 81.98% <90.64%> (-0.08%) ⬇️

... and 49 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.

@mcollina mcollina 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.

lgtm

@mcollina mcollina added the notable-change PRs with changes that should be highlighted in changelogs. label Sep 13, 2026
@github-actions

Copy link
Copy Markdown
Contributor

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @mcollina.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

@mcollina mcollina added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 13, 2026

@gurgunday gurgunday 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.

lgtm

@RafaelGSS RafaelGSS added the deprecations Issues and PRs related to deprecations. label Sep 14, 2026
Comment thread doc/api/deprecations.md
@araujogui

Copy link
Copy Markdown
Member Author

@nodejs/sqlite should we rename StatementSync to Statement too?

@geeksilva97

Copy link
Copy Markdown
Contributor

@nodejs/sqlite should we rename StatementSync to Statement too?

I'd say so. It was in my plan in our first attempt.

@araujogui
araujogui force-pushed the sqlite-rename-database-class branch from 7debaa5 to 3fa9279 Compare September 14, 2026 18:17
@araujogui
araujogui force-pushed the sqlite-rename-database-class branch from 3fa9279 to 8fec2eb Compare September 22, 2026 13:06
@araujogui

Copy link
Copy Markdown
Member Author

@RafaelGSS do you still have request changes?

@trivikr
trivikr requested a review from RafaelGSS September 22, 2026 17:40

@RafaelGSS RafaelGSS 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.

The doc-only deprecation and aliases address my concern

Existing usage continues to work without a runtime warning, so semver-minor makes sense to me. LGTM.

@araujogui araujogui added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 23, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 23, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Rename the DatabaseSync class to Database, and the internal
DatabaseSyncLimits helper to DatabaseLimits to match.

Assisted-by: Claude Code
Signed-off-by: Guilherme Araújo <arauujogui@gmail.com>
DatabaseSync is kept as an alias of Database, wrapped with a DEP0210
runtime deprecation warning, so existing code using the pre-rename
name keeps working.

Assisted-by: Claude Code
Signed-off-by: Guilherme Araújo <arauujogui@gmail.com>
lib/sqlite.js used object-spread (`...sqlite`) to re-export the
internal binding, which crashed tools/doc's api-links generator: it
walks module.exports's ObjectExpression properties expecting a
`value` on each one, and a SpreadElement has no such property
(TypeError: Cannot read properties of undefined (reading 'type')).
This broke the "Build from tarball" CI job.

Destructure the binding's exports into bare identifiers instead,
matching the pattern already used in lib/crypto.js for its deprecated
Hash/Hmac exports.

Also fill in the DEP0210 changes entry's pr-url, now that the pull
request exists.

Assisted-by: Claude Code
Signed-off-by: Guilherme Araújo <arauujogui@gmail.com>
New deprecations start out Documentation-only and only gain a runtime
warning in a later release, same as DEP0179/DEP0181 did for
crypto.Hash/Hmac. Drop the util.deprecate() wrapper for now: export
DatabaseSync as a plain alias of Database again (same function
identity, no warning), and mark DEP0210 as Documentation-only in the
docs.

Assisted-by: Claude Code
Signed-off-by: Guilherme Araújo <arauujogui@gmail.com>
Signed-off-by: Guilherme Araújo <arauujogui@gmail.com>
Signed-off-by: Guilherme Araújo <arauujogui@gmail.com>
@araujogui
araujogui force-pushed the sqlite-rename-database-class branch from 8fec2eb to 2b3ccb5 Compare September 23, 2026 18:58
Sign up for free to 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++. deprecations Issues and PRs related to deprecations. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.