Skip to content

sqlite: cleanup ERM support and export Session class #58378

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

sqlite: cleanup ERM support and export Session class #58378

wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented May 18, 2025

Update sqlite Session to support Symbol.dispose and move the definition of the dispose methods to c++ to close the open TODO

@jasnell jasnell requested review from cjihrig and aduh95 May 18, 2025 21:00
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/sqlite

@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 related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels May 18, 2025
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
cjihrig
cjihrig reviewed May 18, 2025
View reviewed changes
@jasnell jasnell force-pushed the jasnell/sqlite-session-erm branch from f4140ec to b70180d Compare May 18, 2025 23:56
jasnell
jasnell commented May 18, 2025
View reviewed changes
@jasnell jasnell requested a review from cjihrig May 18, 2025 23:57
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@codecov Codecov

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@jasnell jasnell added semver-minor PRs that contain new features and should be released in the next minor version. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. dont-land-on-v23.x PRs that should not land on the v23.x-staging branch and should not be released in v23.x. labels May 19, 2025
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@jasnell jasnell force-pushed the jasnell/sqlite-session-erm branch from b70180d to ddd5402 Compare May 24, 2025 21:14
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
aduh95
aduh95 approved these changes May 25, 2025
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Is it actually semver-minor? It's unclear to me what would be user-facing in the change.

@jasnell
Copy link
Member Author

jasnell commented May 25, 2025

Yes, I would think it is. using session = database.createSession(); is user-facing.

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@jasnell
sqlite: cleanup ERM support and export Session class
63f2dfc 
Update sqlite Session to support Symbol.dispose and
move the definition of the dispose methods to c++ to
close the open TODO
@jasnell jasnell force-pushed the jasnell/sqlite-session-erm branch from ddd5402 to 63f2dfc Compare May 26, 2025 20:34
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/67118/

Renegade334

This comment was marked as resolved.

Sign in to view

@jasnell
Copy link
Member Author

jasnell commented May 27, 2025

Nit: the disposable change and the exposed export change are probably disparate enough to warrant separate commits, as the latter should be cherry-pickable to v22.

I'll do a separate backport commit. There are several pieces in here that might be tricky to backport cleanly.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/67149/

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

@Renegade334 Renegade334 Renegade334 left review comments

@LiviaMedeiros LiviaMedeiros LiviaMedeiros left review comments

@cjihrig cjihrig cjihrig approved these changes

@aduh95 aduh95 aduh95 approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. dont-land-on-v23.x PRs that should not land on the v23.x-staging branch and should not be released in v23.x. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jasnell @nodejs-github-bot @cjihrig @Renegade334 @aduh95 @LiviaMedeiros