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

Fix Deno packaging and add example to CI #297

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kj-9
Copy link

@kj-9 kj-9 commented Feb 8, 2025

refs #138

changes:

  • add test for Deno to run example script in ci
  • allow Deno to use local file or :memory: by simply using lib-esm/node.js instead of ./lib-esm/web.js

enables:

  • deno to run on local db file or :memory: as node.js

downside:

@edard3v
Copy link

edard3v commented Feb 11, 2025

Sorry bro, I got tired of trying things, if you want I'll leave the repository here, it currently works remotely, but not locally.

https://github.com/edard3v/template-deno

@zephraph
Copy link

Yeah, I think this would be nice to see land. An alternative implementation (if we wanted to avoid ffi) would be to use node:sqlite which landed in Deno 2.2.0 and is supported in node as of v22.5.0. Personally I'd prefer just to land this as is though. There are certain cases that restict the ffi usage, but it's the minimal viable change to get a lot of local cases working. This issue is also experienced upstream in drizzle which is where I first encountered it.

@penberg
Copy link
Contributor

penberg commented Feb 24, 2025

Hey @kj-9! Why is this pull request in draft mode?

@kj-9 kj-9 marked this pull request as ready for review February 24, 2025 10:28
@kj-9
Copy link
Author

kj-9 commented Feb 24, 2025

@penberg
thanks for taking time. because I wasn't sure if it was a good option, but I'm now removing the draft status.

It'd be awesome if you could take a look at this.

@penberg penberg changed the title update ci to run example in deno and fix it Fix Deno packaging and add example to CI Feb 24, 2025
@penberg
Copy link
Contributor

penberg commented Feb 24, 2025

@notrab I am fine with this, please have a look.

@notrab
Copy link
Member

notrab commented Feb 24, 2025

@penberg @kj-9 @edard3v @zephraph this looks fine to me, although I did want to check that there could be no unintended side effects by using the node.js entrypoint, but since Deno now has full Node.js compatibility, it should work as is without needing to change any internal import statements?

If there's no pushback from anyone else on this, let's merge and release.

@zephraph
Copy link

The imports are fine. The one thing I'm still thinking on is the FFI requirement. At first I was thinking that wouldn't be too big of a deal, but I'm a little more concerned about it because there are certainly usecases (like deno deploy) that this will break for.

@zephraph
Copy link

Yeah, so this is unfortunately a breaking change and there's not an easy way I can see to get around it. @kj-9 enumerated some of the challenges in the original issue. If FFI is denied then the module will throw an exception. In existing deployments that rely on it (deno deploy, val.town, etc) that don't (or can't) provide FFI that'll render the module unusable. The ideal case here would be that the web path should still work if FFI isn't enabled. @kj-9 pointed out conditional imports, which would also be a breaking change because it would indeed make createClient async which is a breaking change.

@edard3v
Copy link

edard3v commented Feb 24, 2025

Greetings! I don't know if I understood correctly. Does it already work locally with Deno? I'm currently on Neon so I haven't tried it. If there's a green light, I might switch.

@notrab
Copy link
Member

notrab commented Feb 24, 2025

@zephraph and I spoke briefly about this today. This is considered a breaking change, so @zephraph kindly offered to think through the proposal a bit more and we can see where we end up.

@kj-9
Copy link
Author

kj-9 commented Feb 25, 2025

Hi folks, thanks for the comments!
I feel like this PR isn’t ideal, as @zephraph pointed out—it breaks compatibility with Deno Deploy 😅.
To clarify my thoughts so far, here’s a summary of the options I’ve considered. None of them are perfect though...

  1. Requiring FFI (this PR)
  • This approach seems not practical since most deployment environments (e.g., Deno Deploy) do not support FFI. (and are unlikely to do so in the future for security reasons?😅)
  1. Dynamically Import FFI
  • Use dynamic imports to load FFI functions only when needed.
  • This would require changing the function signature to async or introducing a separate async
    version like createClientAsync.
  • While not ideal, this could be an acceptable solution as it improves usability and makes importing local SQLite functionality more noticeable and accessible.
  • However, users would still need to adopt the new function if they want to use it, which takes time and effort to fix downstream tools like drizzle.
  1. Use libsql-client-wasm
  • Replace the current implementation with a libsql-client-wasm for local SQLite functionality in Deno.
  • IMO, this aligns well with Deno’s philosophy of using WASM as the primary interface for external binaries.
  • However, the current libsql-wasm-experimental library does not yet provide full functionality (e.g. I tried but batch() does not work properly in the exmaple). It’s also unclear whether it will be fully compatible with libsql-client?

@edard3v
Copy link

edard3v commented Feb 25, 2025

It's a shame. Well, hopefully one day you can use Turso in Deno. Thanks

@notrab
Copy link
Member

notrab commented Feb 25, 2025

It's a shame. Well, hopefully one day you can use Turso in Deno. Thanks

@edard3v you can? Just not local file with the libsql client. Turso works over HTTP, so you should have no problems communicating with Turso using a remote database.

@jsharma44
Copy link

I am having same issue. is there a way we can use local db with this SDK in deno. Thanks

@zephraph
Copy link

zephraph commented Mar 3, 2025

I am having same issue. is there a way we can use local db with this SDK in deno. Thanks

You can directly import the node version: import { createClient } from "npm:@libsql/client/node";

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.

6 participants