Attachment package refactor

[dean-journeyapps]

This ports the new attachment APIs from the Kotlin and Swift SDKs over to Dart. For simplicity, the new APIs are added to package:powersync_core (with an independent package:powersync_core/attachments/attachments.dart import).

Unlike the old attachments helper package, the new APIs generally support the web platform as well (instead of causing compilation errors). However, one needs to explicitly provide a storage implementation, and the only builtin implementation (IOLocalStorage) will not work on the web. We can revisit proper web support at a later point in time.

[simolus3]

Some more comments from a quick look.

Also, I assume supabase-todolist-new-attachments was copied from the old one? For a review it would be easier if the changes were in the same project so I can see what has changed. But that can happen in the end.

[simolus3]

A few more comments, most of them pretty minor again :)

[simolus3]

The implementation looks good now, I have a few nits on types. I also think we should strive to not introduce classes / interfaces we don't strictly need, especially if they're publicly exported. Also more generally:

• Please run dart format on the package. I also recommend enabling format on save.
• I think a few more tests with an actual database would be nice to have, you can take a look at the Kotlin tests for attachments for inspiration. We can test on native platforms only in the beginning, you can probably copy something like this to set up databases. But it's still a bit tricky, let me know if you need help with this. I can also push a basic scaffold for tests to this branch.

[stevensJourney]

I left some minor comments. Overall I'm very happy with the changes here. The code is well documented and very clean :)

[stevensJourney]

This looks good to me 👍 . I'm happy to release this as is if needed, we can port the localURI changes later - potentially when we update the Kotlin SDK.

[stevensJourney]

The changes here look amazing to me.
The last thing I can think of is we might want to update the current Attachments package to maybe deprecate that and add a small reference to this new functionality.

[simolus3]

The last thing I can think of is we might want to update the current Attachments package to maybe deprecate that and add a small reference to this new functionality.

I will do that in a separate PR once we prepare the release 👍

[n0mad-d3v]

Thanks @simolus3 and @stevensJourney, I'll try to do the migration on PuffinPilot app this weekend and let you know if I see anything 🤞

I'll be able to remove my

  powersync_attachments_helper:
    git:
      url: https://github.com/n0mad-d3v/powersync.dart
      path: packages/powersync_attachments_helper
      ref: feature/file-extension

Which is always nice 😁

[n0mad-d3v]

TL;DR: Everything went fine, thanks guys! 🙌

@simolus3 @stevensJourney I migrated from my fork to this new version and everything went fine. A lot of code I was doing manually is now part of the logic of the library. So I could remove lines, which is always nice! 😎

The only "issue" I got is that I'm using Drift to manage my local database and I wasn't able to use my DAO in the afterSaveHook/afterDeleteHook as I had a lock conflict. It might be a nice addition to the documentation at some point, but nothing blocking really.
I didn't catch any issue and the ‎demos/supabase-todolist was really nice to follow to do the migration 👍

Thanks again!

[simolus3]

I wasn't able to use my DAO in the afterSaveHook/afterDeleteHook as I had a lock conflict

This is a good point, thanks for sharing that. This is something we should be able to fix in drift and drift_sqlite_async (by essentially letting drift use an existing transaction opened by the attachments helper).