-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
[sqlite][ios] introduce sqlite/next #24812
Conversation
ENG-9890 Drop WebSQL interfaces
Originally expo-sqlite is built on top on WebSQL interface, so that we have some legacy code that is not reasonable to SQLite. For example, Given that WebSQL is deprecated, we should just drop WebSQL support. |
ca24747
to
66d2188
Compare
1e3f265
to
5ff2d42
Compare
Looks great @Kudo 👏👏 Added @tsapeta and @lukmccall to get some more opinions |
@tsapeta suggested me to try the shared objects from expo-modules-core. i'm looking into the integration. will update this pr later for the shared objects integration. |
5ff2d42
to
2679388
Compare
updated the pr for shared objects we could go ahead to review the pr again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it! 💙 Pretty powerful example of what we can build with shared objects!
I wonder if we could also have synchronous versions of these methods? Are they expensive to run?
case unknown | ||
|
||
static func fromCode(value: Int32) -> SQLAction { | ||
switch value { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the other cases? Can we maybe link to a documentation where we can find what these "magic" numbers mean? I think it's especially important that there are no specific constants which names would already mean something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are 34 of them 😅 In our case, for the update hook, these are the ones we are interested in but the full list is here https://www.sqlite.org/c3ref/c_alter_table.html
* @param source A string containing the SQL query. | ||
* @param params Parameters to bind to the query. | ||
*/ | ||
public runAsync(source: string, ...params: VariadicBindParams): Promise<RunResult>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if this will render correctly in our docs 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks fine. please see #24962 and preview at http://docs.expo.dev-pr-24962.s3-website-us-east-1.amazonaws.com/versions/unversioned/sdk/sqlite-next/
/** | ||
* A class that represents an instance of the SQLite database. | ||
*/ | ||
export declare class NativeDatabase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like that we don't need any additional JS code here, just the types 😍
# Why the weak shared objects will have exception when deallocating on hermes engine. # How - when the concrete js object is deallocated, we don't have to reset its `__expo_shared_object_id__` property. # Test Plan tested on #24812 + shared objects integration (still local, will update #24812 later)
*/ | ||
public async transactionAsync(txn: () => Promise<void>): Promise<void> { | ||
try { | ||
await this.nativeDatabase.execAsync('BEGIN'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems susceptible to the problem that matt brought up in notion: https://www.notion.so/CRSqlite-REST-syncing-example-69b0d4423d5547c98f24b80fe2323071?d=1f7423c8e5ce4510b26abfcac9bc0815&pvs=4#172d47cb5f794c90aeb4f69f70af4b2b -- i don't think this is a dealbreaker for this api, just something that people might want to watch out for that we may need to document.
in general, what do you think about adding support for sync functions to the api as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with adding sync functions too, if that doesn't degrade the performance. See my question from #24812 (review)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have been convinced by Brent on a call to support sync apis. my only concern is about the api naming. i will do in a separate pr.
we also talked about the problem of async transaction api. i'm still looking for a proper solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems susceptible to the problem that matt brought up in notion: https://www.notion.so/CRSqlite-REST-syncing-example-69b0d4423d5547c98f24b80fe2323071?d=1f7423c8e5ce4510b26abfcac9bc0815&pvs=4#172d47cb5f794c90aeb4f69f70af4b2b
addressed by c11839e from the new transactionExclusiveAsync
call. close ENG-10394
Co-authored-by: Tomasz Sapeta <tomasz.sapeta@swmansion.com>
rebase main to fix the ci errors. |
35ef387
to
6cd231e
Compare
both sync apis and async transaction issues are resolved. the pr is good to review again. also having #24962 for doc update. |
ENG-10394 Enhance transactionAsync in `expo-sqlite/next`
As mentioned by Matt in the comment, async transaction may introduce some out-of-order issue, we should find a way to enhance the API to be more solid. Asides from serialized call or locking call, there's another option to create a new connection ( ENG-10388 Having synchronous API from `expo-sqlite/next`
It has some use cases to have synchronous API in advanced and would be good if we could support this. However, I don't like to have similar APIs and confuse people, like
|
# Why follow up with #24812 to add android implementation # How - add android implementation - [js] update `addDatabaseChangeListener` listener arguments where `dbName = 'main'` by default and having `dbFilePath` to the exact opened file path - [js] update `getAsync` and `getSync` to fix android [returns undefined result](https://github.com/expo/expo/blob/7204c425c62306b7b72c01e1e065a52f97905377/packages/expo-modules-core/android/src/main/java/expo/modules/kotlin/jni/JavaCallback.kt#L16-L19) # Test Plan test-suite SQLiteNext passed on android
# Why the weak shared objects will have exception when deallocating on hermes engine. # How - when the concrete js object is deallocated, we don't have to reset its `__expo_shared_object_id__` property. # Test Plan tested on #24812 + shared objects integration (still local, will update #24812 later)
# Why close ENG-9488 close ENG-9890 close ENG-10388 close ENG-10394 the doc updates and android implementation will be in separate prs. # How drop websql interfaces and rewrite everything apis in `expo-sqlite/next`. the apis are heavily inspired by node-sqlite3 and better-sqlite3. # Test Plan - ts-only unit tests through better-sqlite3 - e2e tests in SQLiteNext.ts
# Why follow up with #24812 to add android implementation # How - add android implementation - [js] update `addDatabaseChangeListener` listener arguments where `dbName = 'main'` by default and having `dbFilePath` to the exact opened file path - [js] update `getAsync` and `getSync` to fix android [returns undefined result](https://github.com/expo/expo/blob/7204c425c62306b7b72c01e1e065a52f97905377/packages/expo-modules-core/android/src/main/java/expo/modules/kotlin/jni/JavaCallback.kt#L16-L19) # Test Plan test-suite SQLiteNext passed on android
# Why following up #24812 to update the doc --------- Co-authored-by: Aman Mittal <amandeepmittal@live.com> Co-authored-by: Bartosz Kaszubowski <gosimek@gmail.com>
# Why following up expo#24812 to update the doc --------- Co-authored-by: Aman Mittal <amandeepmittal@live.com> Co-authored-by: Bartosz Kaszubowski <gosimek@gmail.com>
Why
close ENG-9488
close ENG-9890
close ENG-10388
close ENG-10394
the doc updates and android implementation will be in separate prs.
How
drop websql interfaces and rewrite everything apis in
expo-sqlite/next
.the apis are heavily inspired by node-sqlite3 and better-sqlite3.
Test Plan
Checklist
npx expo prebuild
& EAS Build (eg: updated a module plugin).