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

[sqlite] add experimental promise support #23109

Merged
merged 6 commits into from
Jun 27, 2023
Merged

Conversation

Kudo
Copy link
Contributor

@Kudo Kudo commented Jun 26, 2023

Why

fixes #13357
close ENG-8685

How

  • originally, the callbacks for websql doesn't support Promise. when people using either 'async/async or '.then inside the callback, the statement will be executed after the "transaction end" statement.
  • we should we low level control without websql at all.
  • introduce low-level execAsync
  • introduce transactionAsync
    usage
    const db = SQLite.openDatabase('dbName', version);
    
    const readOnly = true;
    await db.transactionAsync(async tx => {
      const result = await tx.executeSqlAsync('SELECT COUNT(*) FROM USERS', []);
      console.log('Count:', result.rows[0]['COUNT(*)']);
    }, readOnly);
    note that the result is the ResultSet type but not the SQLResultSet type. people can access the result items by rows[0] rather than rows.item(0). i was thinking to deprecate websql somehow and it doesn't make sense to wrap the result by the WebSQLResultSet again

Test Plan

add some SQLite Async unit tests and test suite ci should be passed

Checklist

@linear
Copy link

linear bot commented Jun 26, 2023

ENG-8685 Implementing SQLite to use promises does not work on the second step in a transaction

This issue was automatically imported from GitHub: #13357

Issue accepted by kudo


Summary:

  • User is attempting to use promises with SQLite callbacks to reduce code depth.
  • Issue occurs on both Android and iOS platforms in a managed workflow with SDK version 40.
  • The problem arises when the first SQL statement in a transaction is marked as "complete" using immediate.
  • Executing a second SQL statement in the same transaction hangs due to the transaction being considered "complete."
  • A reproducible demo is provided in an Expo Snack: https://snack.expo.io/trajano/sqlite-async

@expo-bot expo-bot added bot: suggestions ExpoBot has some suggestions bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Jun 26, 2023
@Kudo Kudo marked this pull request as ready for review June 26, 2023 16:18
@Kudo Kudo requested review from brentvatne and alanjhughes June 26, 2023 16:18
): Promise<void> {
await this.execAsync([{ sql: 'BEGIN;', args: [] }], false);
try {
const transaction = new ExpoSQLTransactionAsync(this, readOnly);
Copy link
Member

Choose a reason for hiding this comment

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

out of curiosity - how did you decide on wrapping this in an ExpoSQLTransaction class rather than inlining the execAsync here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the idea is to make transactionAsync like transaction but only for async mode. for transaction, it gives you a transaction interface and that is actually a wrapped WebSQLTransaction as well.

i'm open to the design, feel free to comment if there's any thought.

Copy link
Member

Choose a reason for hiding this comment

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

sounds reasonable, thanks for clarifying!

@Kudo Kudo force-pushed the @kudo/sqlite-enhancement-async branch from 26a4e4f to 5688cea Compare June 27, 2023 14:40
@Kudo
Copy link
Contributor Author

Kudo commented Jun 27, 2023

landing this pr because it doesn't have other concerns and it's experimental.

@sean-fletching
Copy link

sean-fletching commented Jul 2, 2023

Hello,

Thanks for this change! I discovered the transactional problem in my code today (trying to wrap promises around it) and then discovered this thread and the new expo-sqlite 11.3.1.

I've integrated this and I noticed a few things I'd like to share; hopefully this is an OK place:

  • My app now throws an error at startup

Cannot find native module 'ExpoSQLite'

and I assumed that this could be fixed by building an expo-dev-client, which it did. I didn't see this in the documentation but 11.3.1 isn't an approved SDK 48.0.0 package, so I'd assume that also means it doesn't get built into Expo Go either, at least not yet?

@Kudo
Copy link
Contributor Author

Kudo commented Jul 3, 2023

hi there! this feature is available in sdk 49 and it's now public beta.

@mphill
Copy link

mphill commented Jul 10, 2023

Hello,

Thanks for this change! I discovered the transactional problem in my code today (trying to wrap promises around it) and then discovered this thread and the new expo-sqlite 11.3.1.

I've integrated this and I noticed a few things I'd like to share; hopefully this is an OK place:

  • My app now throws an error at startup

Cannot find native module 'ExpoSQLite'

and I assumed that this could be fixed by building an expo-dev-client, which it did. I didn't see this in the documentation but 11.3.1 isn't an approved SDK 48.0.0 package, so I'd assume that also means it doesn't get built into Expo Go either, at least not yet?

This is an interesting situation I saw too.

Make sure you install this using npx expo install expo-sqlite and not yarn add expo-sqlite. This new version of expo-sqlite will fail on expo 48.

mphill added a commit to mphill/kysely-expo that referenced this pull request Jul 10, 2023
@sean-fletching
Copy link

hi there! this feature is available in sdk 49 and it's now public beta.

Thanks! Hey I believe I found a typing error in the code here:
https://github.com/expo/expo/blob/main/packages/expo-sqlite/src/SQLite.ts#L57

This will cause errors to go unnoticed, at least for me, until I checked that the property error was on the result object and throw that result to kill a transaction. I don't want to keep your PR here running though - can I help file a bug on that or a PR?

Kudo added a commit that referenced this pull request Jul 28, 2023
# Why

fixes #23667
close ENG-9446

# How

- from #23109, we moved the returned type of `openDatabase()` from `WebSQLDatabase` to `SQLiteDatabase`. however some properties from websql like the `transaction` and `readTransaction` are missing from `SQLiteDatabase`. this pr tries to add those missing properties.
- migrates **test-suite/tests/SQLite** to typescript based which to dogfood our typings
- update generated doc

# Test Plan

- test test-suite SQLite from ios bare-expo
- run `yarn tsc` from **apps/test-suite**
Kudo added a commit that referenced this pull request Jul 28, 2023
fixes #23667
close ENG-9446

- from #23109, we moved the returned type of `openDatabase()` from `WebSQLDatabase` to `SQLiteDatabase`. however some properties from websql like the `transaction` and `readTransaction` are missing from `SQLiteDatabase`. this pr tries to add those missing properties.
- migrates **test-suite/tests/SQLite** to typescript based which to dogfood our typings
- update generated doc

- test test-suite SQLite from ios bare-expo
- run `yarn tsc` from **apps/test-suite**

(cherry picked from commit c52e83e)
@tanoc
Copy link

tanoc commented Aug 3, 2023

Am I wrong thinking that there should be some sort of locks (or a queue like in @expo/websql) for transactionAsync? It looks if you are running 2 concurrent transactions it could result in something like this with unexpected results:

...
BEGIN
INSERT ...
BEGIN
INSERT ...
END
ROLLBACK
...

In @expo/websql the _createTransaction function is actually appending the transaction to a queue, making every transaction independent, waiting one to finish before starting the next one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: passed checks ExpoBot has nothing to complain about published Changes from the PR have been published to npm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implementing SQLite to use promises does not work on the second step in a transaction
6 participants