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 synchronous execute function #186

Closed
wants to merge 2 commits into from

Conversation

rikur
Copy link

@rikur rikur commented Nov 19, 2024

Fixes #173

Add synchronous execute API to the project.

  • cpp/DBHostObject.cpp
    • Add a new executeSync function for synchronous execution.
    • Directly call opsqlite_execute without using invokeAsync.
    • Register the new executeSync function in the function map.
  • cpp/libsql/bridge.cpp
    • Add a new executeSync function for synchronous execution.
    • Directly call libsql_query_stmt without using libsql_prepare.
  • cpp/DBHostObject.h
    • Declare the new executeSync function.
  • cpp/libsql/bridge.h
    • Declare the new executeSync function.
  • example/src/tests/queries.spec.ts
    • Add test cases for the new executeSync function.

For more details, open the Copilot Workspace session.

@rikur
Copy link
Author

rikur commented Nov 19, 2024

Testing Github Copilot Workspace.

@rikur
Copy link
Author

rikur commented Nov 19, 2024

@ospfranco if you don't mind having a look. I have near zero experience with JSI or C++, this implementation was done 90% by AI given the instructions you posted here: #173 (comment)

I had to manually remove a couple mistakes the AI made, but other than that, it looks pretty darn good to my eyes.

@rikur
Copy link
Author

rikur commented Nov 19, 2024

I couldn't really run the JS / react-native tests, I think my environment is not setup correctly. Instructions on how to setup the dev environment could me useful.

I'll mark this ready for review for the CI to run the tests.

@rikur rikur marked this pull request as ready for review November 19, 2024 15:07
@rikur
Copy link
Author

rikur commented Nov 19, 2024

Oh, and if the code is garbage, please feel free to close this. In that case, I apologize for the noise :)

@ospfranco
Copy link
Contributor

It's wrong. But if you don't mind, I'm waiting for Rodrigo to take a look. There's more behind it than just code. Thanks anyways!

@rikur
Copy link
Author

rikur commented Nov 20, 2024

Great news, maybe us developers get to keep our jobs for a bit longer then 😛

@gituser8796
Copy link

hi @ospfranco - do you think we can give it priority? we're using the sync version and it will require us to make some changes. also, what will be the performance gain when using async compared to sync? will it clone the object for example?

@ospfranco
Copy link
Contributor

There is no performance difference between async and sync, at most there is a small penalty to schedule work on the JS runtime, the code is the same with the exception it's executed in a different thread.

We will get to it when we can, we are just busy with paid work atm.

@ospfranco ospfranco closed this Nov 26, 2024
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.

[Request]: Bring back sync execute
3 participants