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.

Sign up for GitHub

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: Make native modules built with Neon work in Bun #914

Merged
merged 1 commit into from
Jul 8, 2022
Merged

fix: Make native modules built with Neon work in Bun #914

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ Neon projects to 0.10!
Support for [LTS versions of Node](https://github.com/nodejs/LTS#release-schedule) and current are expected. If you're
using a different version of Node and believe it should be supported, let us know.

### Bun (experimental)

[Bun](https://bun.sh/) is an alternate JavaScript runtime that targets Node compatibility. In many cases Neon modules will work in bun; however, at the time of this writing, some Node-API functions are [not implemented](https://github.com/Jarred-Sumner/bun/issues/158).

### Rust

Neon supports Rust stable version 1.18 and higher. We test on the latest stable, beta, and nightly versions of Rust.
Expand Down
10 changes: 5 additions & 5 deletions crates/neon/src/sys/bindings/functions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,19 +371,19 @@ pub(super) unsafe fn load(env: Env) -> Result<(), libloading::Error> {
// with `Error: Module did not self-register` if N-API does not exist.
let version = get_version(&host, env).expect("Failed to find N-API version");

napi1::load(&host, version, 1)?;
napi1::load(&host, version, 1);

#[cfg(feature = "napi-4")]
napi4::load(&host, version, 4)?;
napi4::load(&host, version, 4);

#[cfg(feature = "napi-5")]
napi5::load(&host, version, 5)?;
napi5::load(&host, version, 5);

#[cfg(feature = "napi-6")]
napi6::load(&host, version, 6)?;
napi6::load(&host, version, 6);

#[cfg(feature = "napi-8")]
napi8::load(&host, version, 8)?;
napi8::load(&host, version, 8);

Ok(())
}
22 changes: 16 additions & 6 deletions crates/neon/src/sys/bindings/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ macro_rules! generate {

#[inline(never)]
fn panic_load<T>() -> T {
panic!("Must load N-API bindings")
panic!("Node-API symbol has not been loaded")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently this panic stack looks like:

thread '<unnamed>' panicked at 'Node-API symbol has not been loaded', crates/neon/src/sys/bindings/functions.rs:307:5
stack backtrace:
   0: std::panicking::begin_panic
             at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/std/src/panicking.rs:525:12
   1: neon::sys::bindings::functions::napi6::panic_load
             at /work/crates/neon/src/sys/bindings/mod.rs:121:13
   2: neon::sys::bindings::functions::napi6::NAPI::get_instance_data

The panic message itself doesn't explain what symbol is missing; instead it requires the backtrace to determine. This is by design because it's never expected to happen on Node and only having a single panic method for the defaults reduces binary bloat.

Do you think it's worth the cost to binary sizes (a bunch of unique functions that should never be executed) to have more clear error messages in bun? I personally feel that the backtrace is sufficient since eventually all symbols should be implemented.

}

static mut NAPI: Napi = {
Expand All @@ -139,21 +139,31 @@ macro_rules! generate {
host: &libloading::Library,
actual_napi_version: u32,
expected_napi_version: u32,
) -> Result<(), libloading::Error> {
) {
assert!(
actual_napi_version >= expected_napi_version,
"Minimum required N-API version {}, found {}.",
"Minimum required Node-API version {}, found {}.",
expected_napi_version,
actual_napi_version,
);

let print_warn = |err| eprintln!("WARN: {}", err);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sample output:

WARN: bun: undefined symbol: napi_get_value_external
WARN: bun: undefined symbol: napi_close_escapable_handle_scope
WARN: bun: undefined symbol: napi_open_escapable_handle_scope
WARN: bun: undefined symbol: napi_create_external
WARN: bun: undefined symbol: napi_escape_handle
WARN: bun: undefined symbol: napi_set_instance_data
WARN: bun: undefined symbol: napi_get_instance_data


NAPI = Napi {
$(
$name: *host.get(napi_name!($name).as_bytes())?,
$name: match host.get(napi_name!($name).as_bytes()) {
Ok(f) => *f,
// Node compatible runtimes may not have full coverage of Node-API
// (e.g., bun). Instead of failing to start, warn on start and
// panic when the API is called.
// https://github.com/Jarred-Sumner/bun/issues/158
Err(err) => {
print_warn(err);
NAPI.$name
},
},
)*
};

Ok(())
}

$(
Expand Down