Support latest version of package:web in drift adapter

[davidmartos96]

Latest drift already supports package web 1.0.0, which would make it on par with sqlite_async 0.9

[davidmartos96]

Marking as draft. I encountered with a breaking change with the new exclusively function in the drift API, which makes the QueryExecutors implement beginExclusive. I'm not sure what the optimal solution would be here, since the TransactionExecutor in this repo is already being taken from a private implementation in the drift source code.

cc @simolus3 Any insight?

[simolus3]

I'll see if there's a way to use sqlite_asnyc's lock mechanisms within the QueryDelegate APIs exposed by drift to avoid implementing TransactionExecutor directly.

[simolus3]

See https://github.com/davidmartos96/sqlite_async.dart/pull/1 for my approach that uses the writeTransaction method from sqlite_async with the high-level drift APIs instead of implementing TransactionExecutor manually.

A downside is that this does not support nested transactions anymore. I think it's a bit weird that the drift adapter has supported them in the first place considering that the rest of sqlite_async doesn't seem to have support for them. TheSupportedTransaction delegate that my PR is migrating to doesn't support nested transactions, but that can be changed in drift. It would still require the underlying sqlite_async package to offer first-class for nested transactions with savepoint management as a prerequisite.

If we're okay with dropping drift support for nested transaction in the executor, I'm in favor of an approach that doesn't implement QueryExecutor directly, as that is an internal class and not suitable for subclassing (adding methods is not considered a breaking change in drift). The delegate classes are stable.

[davidmartos96]

Thank you @simolus3 ! I guess the decision to drop or keep the nested transactions functionality (possibly adding them to sqlite_async) would be for the team at PowerSync to decide. I was just trying to update to the latest drift version in the adapter when I encountered this. I would opt for adding support for nested transactions to sqlite_async if there is the possibility to extend the "SupportedTransaction" API. That would avoid breaking changes for people using PowerSync + drift with nested transactions

[rkistner]

Thanks for the contributions! I'm finally getting around to reviewing this.

On nested transactions: My original thinking was that Drift supports nested transactions, I want to be able to swap in sqlite_async without changing any other code in an app. Connection locking is performed in the outer transaction, and not specifically needed in the nested transactions, so I'm happy with that being implemented on top of sqlite_async instead of inside sqlite_async itself.

That said, I don't expect nested transactions to be a very common use case - I've never had a need for it myself. I think we can remove the nested transaction support here for now, then revisit later if needed.

[davidmartos96]

@rkistner In that case I mark it as ready to review. With Simon's refactor, the custom TransactionExecutor is not needed any more by using drift helper classes. The nested transaction is kept with a skip in case it's revisited in the future.

I also increased the version to 0.2.0-alpha.1 because of the breaking change

[mugikhan]

@davidmartos96 We haven't added a releasing guide to this monorepo yet, but it follows a similar process to our PowerSync package (minus the automated publishing). You should use melos version to version the packages and create tags for the packages. See here for more details: https://melos.invertase.dev/commands/version

[davidmartos96]

@mugikhan I created an entry in the changelog and updated the version of the drift adapter package.

Wouldn't tagging affect only my fork? I thought the release mechanism would be done after merge and when you are ok with publishing it to pub.dev.

[mugikhan]

@davidmartos96 That's correct, we can merge this and I will tag and release the correct versions.