Conversation
|
Review requested:
|
d90b1f9 to
b52ec3e
Compare
|
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #65988 +/- ##
==========================================
+ Coverage 90.29% 90.35% +0.06%
==========================================
Files 790 790
Lines 272531 273835 +1304
Branches 52037 52358 +321
==========================================
+ Hits 246070 247422 +1352
+ Misses 16915 16870 -45
+ Partials 9546 9543 -3
🚀 New features to boost your workflow:
|
|
The
notable-change
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. |
|
@nodejs/sqlite should we rename |
I'd say so. It was in my plan in our first attempt. |
7debaa5 to
3fa9279
Compare
3fa9279 to
8fec2eb
Compare
|
@RafaelGSS do you still have request changes? |
RafaelGSS
left a comment
There was a problem hiding this comment.
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.
8fec2eb to
2b3ccb5
Compare
Rename the DatabaseSync and StatementSync classes to Database and Statement, and the internal DatabaseSyncLimits helper to DatabaseLimits. The old names are kept as aliases of the new classes and are Documentation-only deprecated (DEP0210, DEP0211). 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>
2b3ccb5 to
8a0dc69
Compare
DatabaseSyncclass toDatabaseandStatementSynctoStatement.See #57445 (comment)