chore: move all node-api impl to ext (#24662)

these symbols are re-exported from runtime/cli using `build.rs`, so we don't need them in the same crate.
denoland · Jul 22, 2024 · 92abdb7 · 92abdb7
1 parent a459b43
commit 92abdb7
Show file tree

Hide file tree

Showing 22 changed files with 164 additions and 165 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -5,7 +5,6 @@ resolver = "2"
 members = [
   "bench_util",
   "cli",
-  "cli/napi/sym",
   "ext/broadcast_channel",
   "ext/cache",
   "ext/canvas",
@@ -19,6 +18,7 @@ members = [
   "ext/io",
   "ext/kv",
   "ext/napi",
+  "ext/napi/sym",
   "ext/net",
   "ext/node",
   "ext/url",
@@ -52,7 +52,7 @@ deno_media_type = { version = "0.1.4", features = ["module_specifier"] }
 deno_permissions = { version = "0.21.0", path = "./runtime/permissions" }
 deno_runtime = { version = "0.169.0", path = "./runtime" }
 deno_terminal = "0.2.0"
-napi_sym = { version = "0.91.0", path = "./cli/napi/sym" }
+napi_sym = { version = "0.91.0", path = "./ext/napi/sym" }
 test_util = { package = "test_server", path = "./tests/util/server" }
 
 denokv_proto = "0.8.1"

diff --git a/cli/Cargo.toml b/cli/Cargo.toml
@@ -78,7 +78,6 @@ deno_semver = "=0.5.7"
 deno_task_shell = "=0.17.0"
 deno_terminal.workspace = true
 eszip = "=0.72.2"
-napi_sym.workspace = true
 
 async-trait.workspace = true
 base32.workspace = true

diff --git a/cli/build.rs b/cli/build.rs
@@ -396,7 +396,7 @@ fn main() {
     }
     os => format!("generated_symbol_exports_list_{}.def", os),
   };
-  let symbols_path = std::path::Path::new("napi")
+  let symbols_path = std::path::Path::new("../ext/napi")
     .join(symbols_file_name)
     .canonicalize()
     .expect(

diff --git a/cli/main.rs b/cli/main.rs
@@ -15,7 +15,6 @@ mod js;
 mod jsr;
 mod lsp;
 mod module_loader;
-mod napi;
 mod node;
 mod npm;
 mod ops;

diff --git a/cli/mainrt.rs b/cli/mainrt.rs
@@ -15,7 +15,6 @@ mod errors;
 mod file_fetcher;
 mod http_util;
 mod js;
-mod napi;
 mod node;
 mod npm;
 mod resolver;

diff --git a/cli/napi/README.md b/cli/napi/README.md
diff --git a/cli/napi/mod.rs b/cli/napi/mod.rs
diff --git a/ext/napi/Cargo.toml b/ext/napi/Cargo.toml
@@ -16,4 +16,7 @@ path = "lib.rs"
 [dependencies]
 deno_core.workspace = true
 deno_permissions.workspace = true
+libc.workspace = true
 libloading = { version = "0.7" }
+log.workspace = true
+napi_sym.workspace = true
diff --git a/ext/napi/README.md b/ext/napi/README.md
@@ -0,0 +1,114 @@
+# napi
+
+This directory contains source for Deno's Node-API implementation. It depends on
+`napi_sym` and `deno_napi`.
+
+Files are generally organized the same as in Node.js's implementation to ease in
+ensuring compatibility.
+
+## Adding a new function
+
+Add the symbol name to
+[`cli/napi_sym/symbol_exports.json`](../napi_sym/symbol_exports.json).
+
+```diff
+{
+  "symbols": [
+    ...
+    "napi_get_undefined",
+-   "napi_get_null"
++   "napi_get_null",
++   "napi_get_boolean"
+  ]
+}
+```
+
+Determine where to place the implementation. `napi_get_boolean` is related to JS
+values so we will place it in `js_native_api.rs`. If something is not clear,
+just create a new file module.
+
+See [`napi_sym`](../napi_sym/) for writing the implementation:
+
+```rust
+#[napi_sym::napi_sym]
+pub fn napi_get_boolean(
+  env: *mut Env,
+  value: bool,
+  result: *mut napi_value,
+) -> Result {
+  // ...
+  Ok(())
+}
+```
+
+Update the generated symbol lists using the script:
+
+```
+deno run --allow-write tools/napi/generate_symbols_lists.js
+```
+
+Add a test in [`/tests/napi`](../../tests/napi/). You can also refer to Node.js
+test suite for Node-API.
+
+```js
+// tests/napi/boolean_test.js
+import { assertEquals, loadTestLibrary } from "./common.js";
+const lib = loadTestLibrary();
+Deno.test("napi get boolean", function () {
+  assertEquals(lib.test_get_boolean(true), true);
+  assertEquals(lib.test_get_boolean(false), false);
+});
+```
+
+```rust
+// tests/napi/src/boolean.rs
+
+use napi_sys::Status::napi_ok;
+use napi_sys::ValueType::napi_boolean;
+use napi_sys::*;
+
+extern "C" fn test_boolean(
+  env: napi_env,
+  info: napi_callback_info,
+) -> napi_value {
+  let (args, argc, _) = crate::get_callback_info!(env, info, 1);
+  assert_eq!(argc, 1);
+
+  let mut ty = -1;
+  assert!(unsafe { napi_typeof(env, args[0], &mut ty) } == napi_ok);
+  assert_eq!(ty, napi_boolean);
+
+  // Use napi_get_boolean here...
+
+  value
+}
+
+pub fn init(env: napi_env, exports: napi_value) {
+  let properties = &[crate::new_property!(env, "test_boolean\0", test_boolean)];
+
+  unsafe {
+    napi_define_properties(env, exports, properties.len(), properties.as_ptr())
+  };
+}
+```
+
+```diff
+// tests/napi/src/lib.rs
+
++ mod boolean;
+
+...
+
+#[no_mangle]
+unsafe extern "C" fn napi_register_module_v1(
+  env: napi_env,
+  exports: napi_value,
+) -> napi_value {
+  ...
++ boolean::init(env, exports);
+
+  exports
+}
+```
+
+Run the test using `cargo test -p tests/napi`.
diff --git a/...i/generated_symbol_exports_list_linux.def → ...i/generated_symbol_exports_list_linux.def b/...i/generated_symbol_exports_list_linux.def → ...i/generated_symbol_exports_list_linux.def
diff --git a/...i/generated_symbol_exports_list_macos.def → ...i/generated_symbol_exports_list_macos.def b/...i/generated_symbol_exports_list_macos.def → ...i/generated_symbol_exports_list_macos.def
diff --git a/...generated_symbol_exports_list_windows.def → ...generated_symbol_exports_list_windows.def b/...generated_symbol_exports_list_windows.def → ...generated_symbol_exports_list_windows.def
diff --git a/cli/napi/js_native_api.rs → ext/napi/js_native_api.rs b/cli/napi/js_native_api.rs → ext/napi/js_native_api.rs
@@ -5,7 +5,7 @@
 
 const NAPI_VERSION: u32 = 9;
 
-use deno_runtime::deno_napi::*;
+use crate::*;
 use libc::INT_MAX;
 
 use super::util::check_new_from_utf8;
@@ -17,9 +17,9 @@ use super::util::napi_set_last_error;
 use super::util::v8_name_from_property_descriptor;
 use crate::check_arg;
 use crate::check_env;
-use deno_runtime::deno_napi::function::create_function;
-use deno_runtime::deno_napi::function::create_function_template;
-use deno_runtime::deno_napi::function::CallbackInfo;
+use crate::function::create_function;
+use crate::function::create_function_template;
+use crate::function::CallbackInfo;
 use napi_sym::napi_sym;
 use std::ptr::NonNull;
 
@@ -1644,7 +1644,7 @@ fn napi_get_cb_info(
     check_arg!(env, argc);
     let argc = unsafe { *argc as usize };
     for i in 0..argc {
-      let mut arg = args.get(i as _);
+      let arg = args.get(i as _);
       unsafe {
         *argv.add(i) = arg.into();
       }

diff --git a/ext/napi/lib.rs b/ext/napi/lib.rs
@@ -5,6 +5,21 @@
 #![allow(clippy::undocumented_unsafe_blocks)]
 #![deny(clippy::missing_safety_doc)]
 
+//! Symbols to be exported are now defined in this JSON file.
+//! The `#[napi_sym]` macro checks for missing entries and panics.
+//!
+//! `./tools/napi/generate_symbols_list.js` is used to generate the LINK `cli/exports.def` on Windows,
+//! which is also checked into git.
+//!
+//! To add a new napi function:
+//! 1. Place `#[napi_sym]` on top of your implementation.
+//! 2. Add the function's identifier to this JSON list.
+//! 3. Finally, run `tools/napi/generate_symbols_list.js` to update `cli/napi/generated_symbol_exports_list_*.def`.
+
+pub mod js_native_api;
+pub mod node_api;
+pub mod util;
+
 use core::ptr::NonNull;
 use deno_core::error::type_error;
 use deno_core::error::AnyError;