Skip to content

Conversation

@penberg
Copy link
Contributor

@penberg penberg commented Apr 24, 2025

This pull request ports the libSQL JavaScript package to use napi-rs to 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 around napi-rs and it has more complete support for NAPI anyway.

Fixes #184

@kaaax0815
Copy link
Contributor

what is the priority of this? should i rebase #169 on this when stable?

@penberg
Copy link
Contributor Author

penberg commented Apr 24, 2025

Hey @kaaax0815, this is still a bit early so let's just treat and merge #169 independently.

@kaaax0815 kaaax0815 mentioned this pull request May 12, 2025
@penberg penberg force-pushed the napi-rs branch 2 times, most recently from dbea1b8 to edc040b Compare June 2, 2025 11:42
@penberg penberg force-pushed the napi-rs branch 2 times, most recently from 3953fdb to 9ad62ef Compare June 10, 2025 10:00
@penberg penberg changed the title Switch to using napi-rs Switch libSQL JavaScript SDK to use napi-rs Jun 10, 2025
@penberg penberg force-pushed the napi-rs branch 3 times, most recently from 4ec8e33 to fda747a Compare June 11, 2025 08:08
@penberg penberg marked this pull request as ready for review June 11, 2025 08:08
@penberg penberg requested a review from Copilot June 11, 2025 08:11

This comment was marked as outdated.

Copy link

Copilot AI left a 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-derive and add a build.rs for 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.04 runner label is not a valid GitHub Actions runner (supported labels include ubuntu-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 let over var for block-scoped variables. Consider changing var idx = 0; to let 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`

penberg added 2 commits June 12, 2025 12:54
The tests fail randomly with Node 20 on Windows.
@penberg penberg merged commit 2f62305 into main Jun 12, 2025
33 of 34 checks passed
@penberg penberg deleted the napi-rs branch June 12, 2025 10:23
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.

Make iterator actually asynchronous

3 participants