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.

Sign up for GitHub

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

Jump to bottom

Implementing SQLite to use promises does not work on the second step in a transaction #13357

Closed
trajano opened this issue Jun 21, 2021 · 7 comments · Fixed by #23109
Closed

Implementing SQLite to use promises does not work on the second step in a transaction #13357

trajano opened this issue Jun 21, 2021 · 7 comments · Fixed by #23109
Assignees
@Kudo
Labels
Issue accepted outdated SQLite

Comments

@trajano
Copy link
Contributor

trajano commented Jun 21, 2021 •
edited

Summary

I am trying to use promises to reduce the code depth for the SQLite callbacks. However, it appears that the way expo-sqlite is implemented, using immediate it when it starts and completes the first SQL in a transaction, it marks the transaction as "complete" and executing a second SQL in the same transaction hangs.

Managed or bare workflow? If you have ios/ or android/ directories in your project, the answer is bare!

managed

What platform(s) does this occur on?

Android
iOS

SDK Version (managed workflow only)

40

Environment

Expo CLI 4.5.2 environment info:
System:
OS: Windows 10 10.0.19043
Binaries:
Node: 12.18.0 - ~\scoop\apps\nvm\current\nodejs\nodejs\node.EXE
npm: 6.14.4 - ~\scoop\apps\nvm\current\nodejs\nodejs\npm.CMD
IDEs:
Android Studio: Version 4.2.0.0 AI-202.7660.26.42.7351085
npmPackages:
expo: ^40.0.0 => 40.0.1
react: 16.13.1 => 16.13.1
react-dom: 16.13.1 => 16.13.1
react-native: https://github.com/expo/react-native/archive/sdk-40.0.1.tar.gz => 0.63.2
react-native-web: ^0.16.3 => 0.16.3
Expo Workflow: managed

Reproducible demo or steps to reproduce from a blank project

https://snack.expo.io/@trajano/sqlite-async
@trajano trajano added the needs validation Issue needs to be validated label Jun 21, 2021
@trajano
Copy link
Contributor Author

trajano commented Jun 21, 2021

My suspicion is that the issue is caused by the use of immediate to start the processing of the SQLTask queue and upon completing the first task it makes the transaction "complete"

@trajano trajano mentioned this issue Jun 21, 2021
Open
@trajano
Copy link
Contributor Author

trajano commented Jun 30, 2021

I narrowed it down a bit further on a new expo snack to confirm my suspicion which is the transactions are being managed by WebSQL rather than SQLite itself. When I took out the websql transaction code (I kept most of it still) I am able to do a simple chain of await execAsync(sql) with no issues and also have rollback support.

@trajano
Copy link
Contributor Author

trajano commented Nov 11, 2021

I am noticing though that there is a scenario where the transactions clash, which really shoudn't happen, but since it appears that there's a single connection to the database and all calls go through that single connection. As such you can't really isolate the transactions using SQLite itself.

I think I experimented at one point having every transaction requiring a new connection, but that didn't work either.

@github-actions
Copy link
Contributor

This issue is stale because it has been open for 60 days with no activity. If there is no activity in the next 7 days, the issue will be closed.

@github-actions github-actions bot added the stale label Jan 20, 2022
@github-actions
Copy link
Contributor

This issue was closed because it has been inactive for 7 days since being marked as stale. Please open a new issue if you believe you are encountering a related problem.

@mrmurphy
Copy link

Can this issue be re-opened? I'm surprised that, when the mainstream idiomatic way to handle async operations in JS/TS is now Async/Await, Expo's sql library auto-closes transactions after the first line of execution when wrapping promises are used. In my opinion, to make expo-sqlite a viable offering (with respect to developer ergonomics) it'd retain the same operational semantics when called within a promise chain as when called using callbacks.

Ideally, the library would expose a promise-based API in the first place. But promise compatibility at the very least feels essential.

It seems that promises were omitted because the author wanted to follow the design of the WebSQL standard, but that standard has been basically abandoned long ago. I don't think that conforming to the WebSQL API will help any developers get a productivity boost here, but the opposite.

I ran into this bug a couple of years ago when I tried to use Expo SQLite, and now again when I came back for another try.

Thanks for all your hard work! The Expo team is doing amazing things.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2023
@danielt-ent danielt-ent mentioned this issue May 16, 2023
Closed
@Kudo Kudo added Issue accepted and removed stale needs validation Issue needs to be validated outdated labels May 23, 2023
@Kudo Kudo self-assigned this May 23, 2023
@expo-bot
Copy link
Collaborator

Thank you for filing this issue!
This comment acknowledges we believe this may be a bug and there’s enough information to investigate it.
However, we can’t promise any sort of timeline for resolution. We prioritize issues based on severity, breadth of impact, and alignment with our roadmap. If you’d like to help move it more quickly, you can continue to investigate it more deeply and/or you can open a pull request that fixes the cause.

@Kudo Kudo reopened this May 23, 2023
@expo expo unlocked this conversation Jun 5, 2023
@Kudo Kudo mentioned this issue Jun 26, 2023
Merged
3 tasks
Kudo added a commit that referenced this issue Jun 27, 2023
@Kudo
[sqlite] add experimental promise support (#23109)
4a7bfa1 
# 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
  ```tsx
  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](https://github.com/expo/expo/blob/065419647694cf9341261bc7ac614d05e4bac27d/packages/expo-sqlite/src/SQLite.types.ts#L167-L177) but not the [`SQLResultSet` type](https://github.com/expo/expo/blob/065419647694cf9341261bc7ac614d05e4bac27d/packages/expo-sqlite/src/SQLite.types.ts#L93C18-L121). 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](https://github.com/nolanlawson/node-websql/blob/b3e48284572108feff1cd019dc7f13c1d8aa34b2/lib/websql/WebSQLTransaction.js#L12-L36)

# Test Plan

add some SQLite Async unit tests and test suite ci should be passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@Kudo Kudo

Labels
Issue accepted outdated SQLite
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[sqlite] add experimental promise support expo/expo
5 participants
@Kudo @trajano @byCedric @mrmurphy @expo-bot