Dynamic Loading

[kjvalencik]

As described in #584 and tested in experimented with in #645.

Follow-ups:

• Refactor neon-build
• Remove delay load hook
• Convert electron test to N-API

Closes #645

[goto-bus-stop]

this is looking fantastic 🥰

We talked about having new N-API versions behind feature flags so we can introduce them in minor releases. How could we support that in this approach?

I don't know if it's feasible to do this with macro_rules!:

#[cfg(feature = "napi-7")]
fn detach_arraybuffer(...) -> Status;

If not, perhaps we could generate separate structs for each version.

mod napi1 {
    generate!(extern "C" {
        fn create_undefined(...) -> Status;
    });
}
mod napi7 {
    generate!(extern "C" {
        fn detach_arraybuffer(...) -> Status;
    });
}

pub fn setup() {
    // wrapped in ONCE.call_once() stuff
    napi1::load().expect("Could not load N-API functions");
    #[cfg(feature = "napi-7")]
    {
        napi7::load().expect("Could not load N-API version 7 functions");
    }
}

[kjvalencik]

I don't know if it's feasible to do this with macro_rules!:

#[cfg(feature = "napi-7")]
fn detach_arraybuffer(...) -> Status;

I did some reading and learned a couple new things about macros. Yes, this is possible! The meta type can be used for capturing attribute contents.

Additionally, doc comments are syntactic sugar for doc attributes. It will catch them as well, so we can add doc comments to these functions.

Are there any functions that we need to feature gate already?

[goto-bus-stop]

I think the most recent addition thta we use is napi_get_all_property_names, which was added in N-API 6 in early 2020. So not strictly necessary to gate, but it could be a way to ensure that it all works at least.

Another more useful option could be the type tagging stuff that is not yet in a numbered N-API version, but is released in Node.js 12.x and 14.x. But that would probably best be done in a separate PR that also starts to make use of it in JsBox.

[kjvalencik]

@goto-bus-stop After thinking about it a bit more, I think your module suggestion is cleaner. Using attributes, would require making assumptions about those attributes--that they are specifically for conditional compiling--because they would need to be applied in three different places.

Putting them in versioned modules is nice and simple.

[kjvalencik]

Sample macro output:

#[cfg(not(windows))]
use libloading::os::unix::Library;
use std::os::raw::{c_char, c_void};
use super::types::*;

pub(crate) struct Napi {
    get_undefined: unsafe extern "C" fn(env: Env, result: *mut Value) -> Status,
    /* ... repeat for each N-API function */
}

pub(crate) unsafe fn load() -> Result<(), libloading::Error> {
    let host = Library::this();

    NAPI = Napi {
        get_undefined: *host.get("napi_get_undefined".as_bytes())?,
        /* ... repeat for each N-API function */
    };

    Ok(())
}

#[inline]
pub(crate) unsafe fn get_undefined(env: Env, result: *mut Value) -> Status {
    (NAPI.get_undefined)(env, result)
}

fn panic_load<T>() -> T {
    panic!("Must load N-API bindings")
}

static mut NAPI: Napi = {
    unsafe extern "C" fn get_undefined(_: Env, _: *mut Value) -> Status {
        panic_load()
    }
    /* ... repeat for each N-API function */

    Napi {
        get_undefined,
        /* ... repeat for each N-API function */
    }
};

[kjvalencik]

Need to add this as a comment to more clearly document the output of the macro.

[dherman]

This example is so helpful. Can you put it into a comment directly in the source file, maybe with a few sentences explaining the overall approach?

[kjvalencik]

@goto-bus-stop I think we can defer the conditional compiling to a different PR since it's not strictly necessary and we still need to iron out the ergonomics of the feature flags. What do you think?

A quick sketch, I'm thinking something like:

# neon/Cargo.toml
[features]
# Default N-API version
napi = ["napi6"]
napi6 = ["napi-runtime/napi6"]
napi7 = ["napi-runtime/napi7"]
napi-experimental = ["napi-runtime/napi-experimental"]

# neon-runtime/Cargo.toml
[features]
napi = ["libloading"]
napi6 = ["napi"]
napi7 = ["napi6"]
napi-experimental = ["napi7"]

[dherman]

This is too exciting. And I can't believe how much easier it was to understand than I expected. My only comments were really some suggestions for comments explaining things maintainers will want to know down the line.

[dherman]

Maybe be even more explicit in the comment, like "Not strictly needed; just here for easy manual copying"

[kjvalencik]

Done.

[dherman]

These naming conventions aren't, like, gonna change the world, but they're so nice 😎

[kjvalencik]

What's not to love about repeating the word napi 3x in a single identifier? 😆 s/napi::napi_status::napi_ok/napi::Status::Ok/

napi, napi.... napi.

...napi

[dherman]

This example is so helpful. Can you put it into a comment directly in the source file, maybe with a few sentences explaining the overall approach?

[dherman]

I think this file should have a comment at the top explaining the manual process we use for extracting the type declarations from nodejs_sys and pasting them here.

[kjvalencik]

I added this to bindings/mod.rs instead because the approach is identical in types.rs and functions.rs.

[goto-bus-stop]

@goto-bus-stop I think we can defer the conditional compiling to a different PR since it's not strictly necessary and we still need to iron out the ergonomics of the feature flags. What do you think?

Yeah, if we are OK with the module approach then there is no need to figure the details out now 👍

[dherman]

Looks great!