-
Notifications
You must be signed in to change notification settings - Fork 40
Switch libSQL JavaScript SDK to use napi-rs
#182
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
Conversation
|
what is the priority of this? should i rebase #169 on this when stable? |
|
Hey @kaaax0815, this is still a bit early so let's just treat and merge #169 independently. |
dbea1b8 to
edc040b
Compare
3953fdb to
9ad62ef
Compare
napi-rs
4ec8e33 to
fda747a
Compare
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.
Pull Request Overview
This PR ports the libSQL JavaScript SDK from Neon to napi-rs bindings, updates the build toolchain, and revises CI workflows accordingly.
- Replace Neon bindings with
napi/napi-deriveand add abuild.rsfor setup - Update Cargo.toml: bump version, swap dependencies, adjust release profile
- Revamp GitHub Actions: drop neon-based workflows and introduce a unified
CI.yml
Reviewed Changes
Copilot reviewed 51 out of 52 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| npm//*.json & npm//README.md | Add platform-specific npm packages and minimal READMEs |
| integration-tests/tests/async.test.js | Update async iterator usage (await it.next(), for await) |
| Cargo.toml & build.rs | Bump version, swap Neon for napi, add build-dependency |
| .github/workflows/CI.yml | Replace old CI/publish workflows with napi-rs pipeline |
| .cargo/config.toml | Adjust musl linkers |
Files not reviewed (1)
- integration-tests/package-lock.json: Language not supported
Comments suppressed due to low confidence (3)
.github/workflows/CI.yml:45
- The
runs-on: ubuntu-24.04runner label is not a valid GitHub Actions runner (supported labels includeubuntu-latest,ubuntu-22.04, etc.). Please update this to a valid runner name.
- host: ubuntu-24.04
integration-tests/tests/async.test.js:124
- [nitpick] Prefer
letovervarfor block-scoped variables. Consider changingvar idx = 0;tolet idx = 0;for clearer, modern JS semantics.
var idx = 0;
npm/linux-arm64-musl/README.md:3
- [nitpick] Consider adding installation or usage instructions (or a link back to the main README) so consumers know how to install or import this platform-specific package.
This is the **aarch64-unknown-linux-musl** binary for `libsql`
Node 18 is EOL.
The tests fail randomly with Node 20 on Windows.
This pull request ports the libSQL JavaScript package to use
napi-rsto replace Neon bindings. The main motivation is that -- despite multiple attempts -- I have not been able to upgrade Neon bindings (the toolchain changed significantly). But perhaps more importantly, most projects seem to have converged aroundnapi-rsand it has more complete support for NAPI anyway.Fixes #184