-
Notifications
You must be signed in to change notification settings - Fork 83
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
Added commit / rollback hooks #268
base: main
Are you sure you want to change the base?
Conversation
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 couldn't get any web tests working so I wouldn't at all be surprised if they're broken
I suppose due to the build setup, right? Let me know if there's something I can do to help (you can also download the sqlite3.wasm
from a recent release into example/web/sqlite3.wasm
to run tests, but that bundle won't expose the new commit hook methods).
The changes in general look good to me though, so I'm also happy to test the web-specific bits here.
const char *db, | ||
const char *table, | ||
sqlite3_int64 rowid); | ||
import_dart("function_update_hook") extern void dartUpdateHook(void *id, int kind, |
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.
Please keep the existing name unchanged, newer versions of package:sqlite3
need to be compatible with older WebAssembly bundles too.
@@ -228,6 +231,8 @@ base class DatabaseImplementation implements CommonDatabase { | |||
listener.close(); | |||
} |
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 think we should also close the rollback listeners here.
/// | ||
/// Updates are only sent across worker channels while a subscription to this | ||
/// stream is active. | ||
Stream<void> get rollbacks; |
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's not clear how we would best translate this feature to sqlite3_web
. We can't provide commit hooks that would be able to revert a transaction because that would require an async roundtrip between the worker and the main page.
We could provide a "read-only" Stream<void> get commits
stream that installs a () => true
filter reporting events if necessary. But I'm also fine with not exposing this in sqlite3_web
until it is needed (sqlite3_web
also provides the option of compiling custom workers where users have full control over the database, so they could install commit / rollback filters on the database if they need to).
Terribly sorry, work got hectic for a bit, then Christmas... and now I'm heading away for a few weeks. Just popping by to say this'll be near the top of the list of things I want to get onto when I'm back :) |
As requests in #265, I couldn't get any web tests working so I wouldn't at all be surprised if they're broken... but might be best to seek feedback on the overall design before getting too finicky there.