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
Closed
jasnell wants to merge 1 commit into from
Closed

sqlite: cleanup ERM support and export Session class #58378

jasnell wants to merge 1 commit into from
+74 −14

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 aduh95 and cjihrig May 18, 2025 21:00
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 18, 2025

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

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 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

nodejs-github-bot commented May 26, 2025

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

nodejs-github-bot commented May 27, 2025

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

@jasnell
Copy link
Member Author

jasnell commented May 31, 2025

Landed in 62ba619

@jasnell jasnell closed this May 31, 2025
@targos targos added the backport-requested-v24.x PRs awaiting manual backport to the v24.x-staging branch. label May 31, 2025
@targos
Copy link
Member

targos commented May 31, 2025 •
edited
Loading

v8::Symbol::GetAsyncDispose does not exist in V8 13.6. This either needs a manual backport to v24.x-staging or to be labeled dont-land-on-v24.x

@Renegade334
Copy link
Member

Renegade334 commented Aug 8, 2025 •
edited
Loading

After #58818, this now lands cleanly on v24.x-staging.

Does this still need a PR, or can this just be flagged to releasers?

(@targos)

@targos targos removed the backport-requested-v24.x PRs awaiting manual backport to the v24.x-staging branch. label Sep 24, 2025
@github-actions github-actions bot mentioned this pull request Sep 24, 2025
Merged
@github-actions github-actions bot mentioned this pull request Jan 3, 2026
Closed
@Renegade334 Renegade334 mentioned this pull request Jan 4, 2026
Open
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. 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.

7 participants

@jasnell @nodejs-github-bot @targos @Renegade334 @cjihrig @aduh95 @LiviaMedeiros