Conversation
Contributor
|
what is the priority of this? should i rebase #169 on this when stable? |
Contributor
Author
|
Hey @kaaax0815, this is still a bit early so let's just treat and merge #169 independently. |
Closed
dbea1b8 to
edc040b
Compare
3953fdb to
9ad62ef
Compare
napi-rs
4ec8e33 to
fda747a
Compare
There was a problem hiding this comment.
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 was referenced Jun 13, 2025
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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