sqlite_async.dart icon indicating copy to clipboard operation
sqlite_async.dart copied to clipboard

Prototype for impl with sqlite3_web package

Open simolus3 opened this issue 1 year ago • 7 comments

This demonstrates how an async wrapper implementing interfaces from this package could look like when using the new shared sqlite3_web package. That package extracts most of the logic currently part of drift to detect browser features and to host sqlite3 databases in a web worker.

I only had a quick look at this package and so I didn't integrate this with the open factories yet, but still it implements SqliteDatabase and should support running statements and receiving update notifications. Update notifications are shared between tabs when using AccessMode.throughSharedWorker. The sqlite3_web package will provide a mechanism for custom requests. Drift will use that to broadcast query updates since it's not using native update notifications at the moment. sqlite_async can use that to implement locks, by making clients issue a lock request before accessing the database. This functionality is not yet implemented in sqlite3_web though, once that's done I can integrate that here. Another thing not yet implemented is the automated detection of a suitable VFS implementation or worker setup based on supported browser features. Browser features can already be detected though, so it's just a matter of using that information.

I've seen that there is an existing implementation for local databases with SyncSqliteConnection. We may have to use that if the browser doesn't support workers at all, of if we're using per-tab IndexedDB where using workers doesn't make much of a difference. But since drift will have to do the same thing, it's also something that could be implemented in the sqlite3_web package too. For scenarios in which different tabs don't share the actual sqlite3 connection through a shared worker (for instance because shared workers aren't available), things like update notifications would have to be broadcast using another mechanism (I'm using currently using the Broadcast Channel API for that in drift). I'm not sure if that part should be implemented in sqlite3_web as well since at that point we really have multiple "processes" accessing the same sqlite database with no shared-memory between them.

simolus3 avatar Feb 18 '24 16:02 simolus3

Thanks, it looks great so far!

I only had a quick look at this package and so I didn't integrate this with the open factories yet

We're busy refactoring those to simplify things a little - so don't worry about that just yet.

We'll hopefully have more feedback on the other details later this week / early next week.

rkistner avatar Feb 20 '24 13:02 rkistner

Did you have a chance to look at this yet? There's still work for me to do in the shared package, but if there is a fundamental limitation of the approach/API that doesn't work for you, it would be good to know before porting everything else over.

simolus3 avatar Mar 04 '24 15:03 simolus3

Did you have a chance to look at this yet? There's still work for me to do in the shared package, but if there is a fundamental limitation of the approach/API that doesn't work for you, it would be good to know before porting everything else over.

Hi @simolus3 ! We've made some changes to the structure for opening connections here https://github.com/powersync-ja/sqlite_async.dart/pull/30.

The SqliteDatabase web implementation now uses the DefaultSqliteOpenFactory from lib/src/web/web_sqlite_open_factory.dart's openConnection method to create its primary asynchronous database connection.

SqliteDatabase (on web) is now essentially a thin wrapper for the SqliteConnection which is returned from the open factory it is constructed with.

It should not take much effort to move this PR implementation to the DriftSqliteConnection.

stevensJourney avatar Mar 04 '24 18:03 stevensJourney

Sorry for the delay on this - I've moved the implementation over to the sqlite3_web package now. When the package uses a shared worker, we can avoid the local lock management and coordinate that through the shared worker hosting the database. We're still using the existing mutex for connections made through dedicated workers.

Are there any tests or working examples I could use to test the web implementation?

simolus3 avatar Mar 29 '24 16:03 simolus3

Sorry for the delay on this - I've moved the implementation over to the sqlite3_web package now. When the package uses a shared worker, we can avoid the local lock management and coordinate that through the shared worker hosting the database. We're still using the existing mutex for connections made through dedicated workers.

Are there any tests or working examples I could use to test the web implementation?

This looks amazing so far. There should be some automated tests for web which you can verify with. This example app is a bit upstream, it uses the powersync.dart package which links to this sqlite_async.dart package ,but we have a working example of a todolist Flutter app here

You should be able to test using the supabase-todolist demo in the demos folder. The README should contain all the instructions required to get that demo up and running. I suspect there might be some issues if getAutoCommit is not yet implemented.

stevensJourney avatar Apr 02 '24 07:04 stevensJourney

I've published the first version of sqlite3_web to pub.dev

simolus3 avatar May 15 '24 15:05 simolus3

I've published the first version of sqlite3_web to pub.dev

~~Thanks! Will try and test. Are the sqlite3 package changes also published?~~ I see it is already published.

stevensJourney avatar May 15 '24 15:05 stevensJourney

@simolus3 I implemented some of the notes on this PR here, in testing that work and some additional upstream testing I noticed that calling the close method seems to throw an exception. image Here is an example of where breaking could occur. Do you have any ideas of what would be causing this error?

stevensJourney avatar May 22 '24 07:05 stevensJourney

Thanks, the problem was that we first ask the worker to close the database and then close the stream controller. Once the cancel callback from the stream controller for updates gets invoked, we ask the worker to stop sending us update notifications for that database. The worker then gets confused since the database we're asking about doesn't exist.

I've fixed this in version 0.1.1-wip of sqlite3_web.

simolus3 avatar May 22 '24 20:05 simolus3

Thanks, the problem was that we first ask the worker to close the database and then close the stream controller. Once the cancel callback from the stream controller for updates gets invoked, we ask the worker to stop sending us update notifications for that database. The worker then gets confused since the database we're asking about doesn't exist.

I've fixed this in version 0.1.1-wip of sqlite3_web.

Thanks for the update @simolus3 . I tested the new packages and the close method no longer throws an exception. I did notice that it seems like the DB is cleared after closing a connection. Is that expected? I added a test for this here. I have confirmed that the same path is being used for both connections, but maybe I'm missing something silly.

stevensJourney avatar May 23 '24 14:05 stevensJourney

It was my silly mistake - despite IndexedDB being available, sqlite3_web determined that it should use an in-memory database. https://github.com/simolus3/sqlite3.dart/commit/cc095786705aea1d54be31580bf37f1d3987ad29 fixes that, I've released it as 0.1.2.

simolus3 avatar May 23 '24 20:05 simolus3

Hi, I'm wondering if you have additional feedback or things you need from sqlite3_web here? If the current API matches what you have in mind and works for you, I'll start working on integration tests on the sqlite3_web side to start finishing this up.

simolus3 avatar Jun 27 '24 21:06 simolus3

Hi, I'm wondering if you have additional feedback or things you need from sqlite3_web here? If the current API matches what you have in mind and works for you, I'll start working on integration tests on the sqlite3_web side to start finishing this up.

Hi @simolus3 . Sorry for the late reply. I think we have everything we need in the current API. We've just merged the work from this PR in this PR here.. I'll close this pull request, since your work is included there. Thank you for the wonderful library!

stevensJourney avatar Jul 01 '24 13:07 stevensJourney