FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src,sqlite: replace tolocalchecked with tolocal by geeksilva97 · Pull Request #60028 · nodejs/node · GitHub

/ node Public

src,sqlite: replace tolocalchecked with tolocal - #60028

Closed
geeksilva97 wants to merge 3 commits into
nodejs:mainfrom
geeksilva97:fix-filter-function-crashing
ConversationCommits (3)ChecksFiles changed
Closed

src,sqlite: replace tolocalchecked with tolocal#60028
geeksilva97 wants to merge 3 commits into
nodejs:mainfrom
geeksilva97:fix-filter-function-crashing

Conversation

geeksilva97 commented Sep 26, 2025

Copy link
Copy Markdown
Contributor

This PR replaces ToLocalChecked with ToLocal in multiple places so we prevent unexpected crashes.

nodejs-github-bot commented Sep 26, 2025

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/sqlite

nodejs-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 Sep 26, 2025
addaleax mentioned this pull request Sep 26, 2025
Closed
addaleax approved these changes Sep 26, 2025
View reviewed changes
Comment thread src/node_sqlite.cc Outdated
Comment thread src/node_sqlite.cc
Comment thread src/node_sqlite.cc Outdated

codecov Bot commented Sep 26, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 72.22222% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.47%. Comparing base (3312e4e) to head (1490d09).
⚠️ Report is 22 commits behind head on main.

Files with missing lines Patch % Lines
src/node_sqlite.cc 72.22% 3 Missing and 2 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #60028      +/-   ##
==========================================
- Coverage   88.47%   88.47%   -0.01%     
==========================================
  Files         703      703              
  Lines      207546   207551       +5     
  Branches    40006    40001       -5     
==========================================
- Hits       183631   183622       -9     
- Misses      15907    15912       +5     
- Partials     8008     8017       +9
Files with missing lines Coverage Δ
src/node_sqlite.cc 79.42% <72.22%> (-0.15%) ⬇️

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

geeksilva97 added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 28, 2025
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 28, 2025

nodejs-github-bot commented Sep 28, 2025

Copy link
Copy Markdown
Collaborator

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

geeksilva97 added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. and removed author ready PRs that have at least one approval, no outstanding review comments, and a CI started. labels Sep 28, 2025
louwers reviewed Sep 28, 2025
View reviewed changes
Comment thread test/parallel/test-sqlite-session.js Outdated
louwers reviewed Sep 28, 2025
View reviewed changes

louwers left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks, this was still one of the open issues for the session extension integration.

I think this implementation makes it possible that a changeset is being applied partially. Not a big deal, the user could just wrap it in a transaction. Just something to be aware of.

I think we should do a rollback if any exception is being thrown from either the filter callback or the conflict handler before stabilizing the SQLite API. But for now, this is an improvement.

geeksilva97 added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 28, 2025
github-actions Bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Sep 28, 2025

github-actions Bot commented Sep 28, 2025

Copy link
Copy Markdown
Contributor
Failed to start CI
   ⚠  Commits were pushed since the last approving review:
   ⚠  - src,sqlite: replace tolocalchecked with tolocal
   ⚠  - src: implement addaleax suggestions
   ✘  Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/18080613898

geeksilva97 requested a review from cjihrig September 29, 2025 13:35
geeksilva97 added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no outstanding review comments, and a CI started. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Sep 29, 2025
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 29, 2025

nodejs-github-bot commented Sep 29, 2025

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Sep 29, 2025

Copy link
Copy Markdown
Collaborator

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

geeksilva97 added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2025
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2025

nodejs-github-bot commented Sep 30, 2025

Copy link
Copy Markdown
Collaborator

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

addaleax pushed a commit that referenced this pull request Sep 30, 2025
sqlite: replace ToLocalChecked and improve filter error handling
4136934 
PR-URL: #60028
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax commented Sep 30, 2025

Copy link
Copy Markdown
Member

Landed in 4136934

addaleax closed this Sep 30, 2025
geeksilva97 mentioned this pull request Sep 30, 2025
Closed
targos pushed a commit that referenced this pull request Oct 6, 2025
sqlite: replace ToLocalChecked and improve filter error handling
d949222 
PR-URL: #60028
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
github-actions Bot mentioned this pull request Oct 7, 2025
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
cjihrig cjihrig approved these changes
gurgunday gurgunday approved these changes
addaleax Awaiting requested review from addaleax
+1 more reviewer
louwers louwers approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. 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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL