-
-
Notifications
You must be signed in to change notification settings - Fork 421
Add updateHook
, commitHook
, and rollbackHook
#1337
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.
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
base: master
Are you sure you want to change the base?
Conversation
These new `Database` methods expose the functionality of `sqlite3_update_hook`, `sqlite3_commit_hook`, and `sqlite3_rollback_hook`, respectively.
We're using these in our port of PowerSync to Node.js: |
I'm happy to add documentation for these functions to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on this! I normally don't accept PRs for large features, but the code here is written very carefully. I think I can merge it if these comments are addressed, and if docs are added.
UpdateHook * self = static_cast<UpdateHook *>(data); | ||
|
||
v8::Isolate * isolate = self->isolate; | ||
v8::Isolate::Scope isolateScope(isolate); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using v8::Isolate::Scope
is new to this library, and I'm not familiar with it. What does it do? And do you think it should be used in other similar situations (such as user-defined SQL functions, or the verbose
hook)?
db(db), | ||
fn(isolate, fn) {} | ||
|
||
static void invoke(void * data, int op, char const * dbName, char const * tableName, sqlite3_int64 rowid) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since these hooks don't work with WITHOUT ROWID
tables or with the truncate optimization, I think that should be called out in the docs.
v8::Local<v8::Value> opArg = self->addon->cs.Op(isolate, op); | ||
v8::Local<v8::Value> dbNameArg = v8::String::NewFromUtf8(isolate, dbName, v8::NewStringType::kNormal).ToLocalChecked(); | ||
v8::Local<v8::Value> tableNameArg = v8::String::NewFromUtf8(isolate, tableName, v8::NewStringType::kNormal).ToLocalChecked(); | ||
v8::Local<v8::Value> rowidArg = v8::BigInt::New(isolate, rowid); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be more appropriate to use v8::Number
if safeIntegers
is not enabled, as that would be consistent with the rest of the library's behavior. I think it should accept a safeIntegers
option similarly to how db.function()
works.
'use strict'; | ||
const Database = require('../.'); | ||
|
||
describe('Database hooks', function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see an additional test: attempting to use the database within a hook should fail, since the db is busy
These new
Database
methods expose the functionality ofsqlite3_update_hook
,sqlite3_commit_hook
, andsqlite3_rollback_hook
, respectively.