Skip to content
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

Use Drift database on Web #1585

Merged
merged 10 commits into from
Nov 3, 2024
Merged

Conversation

gwbischof
Copy link
Contributor

@gwbischof gwbischof commented Oct 26, 2024

This PR configures Drift to work on Web, mobile, and desktop. It also removes the sqflite and sqlite3 deps.

Do we need the sqflite and sqlite3 deps? It looks like they are only being used for migrating from the the old stile database. Can we use Drift to connect to the old style database and migrate it to the new one?

I tested the app on macos, web, android, and ios, it apprears to be working correctly.

@gwbischof gwbischof changed the title WIP: Fix the web build Use Drift database on Web Oct 30, 2024
@gwbischof gwbischof marked this pull request as ready for review October 30, 2024 23:34
@hjiangsu
Copy link
Member

Do we need the sqflite and sqlite3 deps? It looks like they are only being used for migrating from the the old stile database. Can we use Drift to connect to the old style database and migrate it to the new one?

You're correct, we're only using sqflite for database migration tests. However, I would still like to keep the dependencies here for a bit longer (just in case there are still users using the old database schemas). In the future, we'll most likely remove the dependencies and the migration tests, but for now, I would keep them as is!

On a different note, is it okay if I apply some modifications to this PR? I think I might be able to simplify some of the logic further!

@gwbischof
Copy link
Contributor Author

Yep go for it, do you need permissions?
I was thinking a new PR might be easier than adding the SQLite deps back to this one.

@hjiangsu
Copy link
Member

hjiangsu commented Nov 1, 2024

Thanks, I can make a commit directly to this branch to keep things in order!

@hjiangsu
Copy link
Member

hjiangsu commented Nov 1, 2024

I'll list out the changes I made here (f92770a):

  • Updated drift and related dependencies
  • Brought back sqflite and sqflite_common_ffi as dev_dependencies (we'll remove these in the future)
  • Improved some of the logic for the database initialization (removed constructDb in favour of driftDatabase)

@gwbischof Could you run a few tests on your end and let me know if this builds properly for all the platforms you mentioned above? Thanks!

@gwbischof
Copy link
Contributor Author

Looks good, I tested on all of my platforms. There are some handled exceptions on web, but its building and it looks like its working.

Copy link
Member

@hjiangsu hjiangsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@hjiangsu hjiangsu merged commit 1c9829e into thunder-app:develop Nov 3, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants