Add Expo Packages (expo-sqlite & op-sqlite)#3841
Add Expo Packages (expo-sqlite & op-sqlite)#3841austinm911 wants to merge 29 commits intorocicorp:mainfrom
Conversation
This reverts commit bad185c.
|
@austinm911 is attempting to deploy a commit to the Rocicorp Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Can someone confirm this is the correct type? I can cast back to any if preferred
austinm911
changed the title
|
Austin, is this ready for another look? |
|
@aboodman a few misc. comments i'll need to likely clean up but it's good for you to review. |
arv
left a comment
arv
left a comment
There was a problem hiding this comment.
This is very nice.
I like where this is going. I tried to be as detailed as possible so do not be deterred by the number of comments.
If you are low on time I'd be more than willing to take over this work that you started.
.gitmodules
Outdated
There was a problem hiding this comment.
orphaned file from colocating the react-native-replicache package as a submodule during dev, should be deleted
| const genericDatabase: GenericSQLiteDatabaseManager = { | ||
| open: (name: string) => { | ||
| const db = OPSQLite.open({name}); | ||
| return Promise.resolve({ |
There was a problem hiding this comment.
Another option might be to use a MaybePromise type so that these extra microtasks can be skipped.
Not a big deal but worth thinking about/benchmarking sometime.
There was a problem hiding this comment.
I'll leave in for now
| await new Promise<void>((resolve, reject) => { | ||
| let didResolve = false; | ||
| try { | ||
| void this._db.transaction(async tx => { |
There was a problem hiding this comment.
same consideration here.
Maybe the test will tell.
| args?: (string | number | null)[] | undefined, | ||
| ): Promise<SQLResultSetRowList> { | ||
| const tx = this.#assertTransactionReady(); | ||
| const {rows} = await tx.execute(sqlStatement, args); |
There was a problem hiding this comment.
Do we want to use a prepare (cache) for this one too?
https://op-engineering.github.io/op-sqlite/docs/api#prepared-statements
There was a problem hiding this comment.
Also, use executeRaw?
https://op-engineering.github.io/op-sqlite/docs/api#raw-execution
There was a problem hiding this comment.
Do we want to use a prepare (cache) for this one too?
https://op-engineering.github.io/op-sqlite/docs/api#prepared-statements
Docs seem to discourage that:
Using prepared statements for writes inside a transaction is discouraged, you gain very little performance when writting to the database and they are not part of the internal lock mechanism of op-sqlite.
https://op-engineering.github.io/op-sqlite/docs/api#transactions
There was a problem hiding this comment.
holding off on the prepared statement per the comments above. @arv maybe executeRaw could be refactored into this later?
|
@Braden1996 FYI |
I'm pretty jammed this week so I won't be able to take a look in great detail until this weekend, so whatever you'd like. I'm good with you taking over. |
|
Thanks for the update @austinm911 |
|
@arv For the remainder of the comments that deal more with internals of how sqlite work under the hood (like withExclusiveTransactionAsync), I think it's better that someone with a better understanding takes over. I made an attempt with the expo-sqlite test (with AI to help) that i'll leave here, but seems like expo-sqlite has to mocked. I don't know if my test attempt is going in the right direction. So perhaps someone else could take it over from here? |
|
Thanks for the contribution @austinm911 - moved this here! #4669 |
5 participants
See https://discord.com/channels/830183651022471199/1329821985999552582
Went ahead and migrated much of the integration done in React Native Replicache into the rocicorp monorepo.
I wasn't really sure to publish this code to test in a standalone react native/expo app and might not have much time to work on that. So figured I would throw this PR up for now. I'll leave that up to y'all how you might want to handle.
hello-zero-expostandalone package