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

Add expo-sqlite/kv-store persist plugin #413

Closed
wants to merge 21 commits into from

Conversation

catalinmiron
Copy link
Contributor

Add expo-sqlite/kv-store to the persist plugins list.

More about the [expo-sqlite/kv-store])(https://docs.expo.dev/versions/latest/sdk/sqlite/#key-value-storage)

@jmeistrich
Copy link
Contributor

This looks great! I wonder about a couple of things:

  1. I don't think it needs the complexity of configuring sqliteStorage. That's on the AsyncStorage plugin because there's multiple implementations of it out there, since it was split from core to a community library. But since there's only one kv-store we can just import it directly.

  2. Because kv-store supports synchronous usage, I think we can simplify the plugin a lot. The initialize and loadTable functions are for preloading all data so that getTable can be synchronous. But since kv-store has getItemSync I think you could just use that in getTable, and just remove initialize and loadTable. Check out the mmkv and local storage plugins for examples of synchronous persist plugins.

Does that make sense? Please let me know if I can help with anything!

@jmeistrich
Copy link
Contributor

Hey @catalinmiron do you want to look into those changes and then we can merge this in? Or if you're too busy I can make those changes on top of the PR and go for merge. I'm very excited to get this in!

@catalinmiron
Copy link
Contributor Author

@jmeistrich Happy New Year!

I was semi offline during the Holidays. I am going to take another look at it next week. I know that the kv-store has the sync API available but I was not sure if everything was exposed as sync (multiGet may not be sync) but I will look into mmkv implementation as well. I'll ping you if I have any issues maybe you can unblock me. Thanks!

@jmeistrich
Copy link
Contributor

Ok great! Hope you had a good holiday 😁

@catalinmiron
Copy link
Contributor Author

@jmeistrich I've made the changes, tested locally and everything works.
Please let me know if this looks good

PS: I don't know what happened to the PR (merged the main branch and it exploded 🤯 ) -> I can open a different one if this is too much noise.
PS2: Since Storage it is initialized with new SQLiteStorage('ExpoSQLiteStorage'), this plugin can receive a different database name as well like:

export const persistOptions = configureSynced({
  persist: {
    plugin: observablePersistSqlite(new SQLiteStorage("myOtherSQLiteDatabase")),
  },
});

@jmeistrich
Copy link
Contributor

Awesome, this looks great now!

But yes the PR seems to have exploded 😂. It looks like it's reverting a bunch of core changes. So can you clear the branch up somehow or just make a new PR? Then I think it looks good to merge.

@catalinmiron catalinmiron closed this by deleting the head repository Jan 21, 2025
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.

2 participants