nodejs / abi-stable-node

Repository used by the Node-API team to manage work related to Node-API and node-addon-api
239 stars 47 forks source link

What should go into N-API 5? #374

Closed gabrielschulhof closed 4 years ago

gabrielschulhof commented 5 years ago

@jarrodconnolly we are considering making the JS Date-related N-APIs part of the N-API 5 stable release. That is, we are considering moving them out of experimental. Can you please share with us examples of their real-world usage? We do not necessarily need pointers to actual code if it's not open source, but please list any real-world use cases you are aware of!

gabrielschulhof commented 5 years ago

@devsnek we would like to remove the experimental flag from around all the bigint N-APIs. Can you please give us some real-world usage examples?

gabrielschulhof commented 5 years ago

@josephg can you please share with us real-world examples of where it is useful to have the JS function for thread-safe function be optional? We need such use cases to support the argument that this modification be considered stable and released with N-API 5.

KevinEady commented 5 years ago

Hey @gabrielschulhof ,

My real-world example of TSFN with no function callback may be kind of specific, but hopefully generalizable. I'm working on embedding node into a native C++ application. I'm compiled nodejs statically, and linking it to my own executable. I call node::Start in a separate thread to run a script that executes a linked-module (via process._linkedBinding), creating a TSFN. The TSFN has an empty function, because I perform all my "run this JavaScript code" in the call_js_cb callback (docs pasted for convenience)

[in] call_js_cb: Optional callback which calls the JavaScript function in response to a call on a different thread. This callback will be called on the main thread. If not given, the JavaScript function will be called with no parameters and with undefined as its this value.

My real-world is using the C++ node-addon-api wip ThreadSafeFunction wrapper, but... I do something like:

  1. TSFN is created by passing a function that does nothing
  2. When I make a call, I do all my logic in the call_js_cb. I do not even perform the "calls the JavaScript function" as said in the docs -- since I have nothing to do in the tsfn registered callback.

So, my "embedding node" case is pretty specific, but I can imagine there are other scenarios where people do all the logic in the call_js_cb (in native land) and not in the tsfn's registered function (in JS land)?

devsnek commented 5 years ago

@gabrielschulhof I am not sure what uses it out in the wild but yesterday I wrote this and it worked like a charm https://github.com/devsnek/node/blob/f0e4e469e45bfa5398a881ecb2671529e1a2036b/src/node_wasi.cc

I'll try to find more usage around the interweb but it might take a day or two.

josephg commented 5 years ago

@gabrielschulhof I'm using napi threadsafe function in the foundationdb bindings. Foundationdb queries resolve by calling a registered callback function on a thread owned by FDB. When this happens I need to:

This is fine - I use the call_js_cb argument of napi_create_threadsafe_function to signal my custom resolver. But I have to create and pass an empty function body into the napi_value func argument even though that function will never be called, because napi_create_threadsafe_function throws otherwise. Its not a lot of work, but its gross.

jarrodconnolly commented 5 years ago

@gabrielschulhof

I added the JS Date-related N-APIs for a couple of reasons.

There were a number of threads on the port of node-sqlite to napi talking about Date and Regex functions being missing and workarounds for them being missing.

https://github.com/mapbox/node-sqlite3/issues/830#issuecomment-389616856 https://github.com/mhdawson/node-sqlite3/pull/3

I still intend to add support for Rexex, the other missing Object type in napi. I would also like to add support to the C++ node-addon-api for these types.

Another reasons is a commercial product I work on that would benefit from using these functions for a couple of native add-ons used internally.

Also I believe rounding out support for these two basic Object types would be a great addition to the napi.

Thanks!

devsnek commented 5 years ago

It appears there is a good amount of ecosystem usage for bigint using this search: https://github.com/search?p=2&q=napi+bigint+-filename%3Atest_bigint.c+-filename%3Abigint.md&type=Code

mhdawson commented 5 years ago

Unrelated but @devsnek would you have some time to talk about WASI/N-API? From your example looks like you might have thought about this and I'm looking for some pointer for better understanding the potential longer term overlap or synergy.

devsnek commented 5 years ago

they don't have any specific overlap, I just needed to expose lseek and I thought it would be nice to use napi for it.

cjihrig commented 5 years ago

@mhdawson if you haven't seen it yet, https://github.com/nodejs/node/pull/27850 has @devsnek's WASI implementation. WASI defines a bunch of system calls that platforms provide to WebAssembly modules. Most of the system calls were able to be implemented using JavaScript and Node's public APIs. lseek() is missing from fs/libuv, so it was implemented in terms of N-API. Other than that, I don't think there is much overlap between N-API and WASI (as @devsnek noted).

josephg commented 5 years ago

It’s a bit of an aside but, I’ve long wanted this. If we could combine n-api and wasi, native modules could be put in NPM with prebuilt binaries for all platforms. There’s some solutions at the moment with prebuildify and various CI systems but (hopefully) with wasi + napi we shouldn’t need any extra infrastructure to allow me to build a native module from my Mac laptop and have it work everywhere that nodejs works.

devsnek commented 5 years ago

consider me nerd-sniped :P

gabrielschulhof commented 5 years ago

@nodejs/n-api I think we should also mark napi_add_finalizer() as stable, because there is a clear need for it in node-addon-api. The wrapper works without it, but it pollutes the objects it creates.

mhdawson commented 5 years ago

Initial PR's landed, backports in progress.

One backports land, will be one final round of PRs/backports to add add version 5 making the functions stable.

gabrielschulhof commented 5 years ago
feature v12.x ✓ v10.x✓ v8.x
update matrix [x]29600 [x]29643
mark stable [x] clean backport [x]29458
napi_add_finalizer [x]cf0e881 [x]bb2bbc8 [ ]28296
date object [x]13b1aaf [x]28298 [ ]28301
optional TSFN callback [x]53297e6 [x]28399 [ ]28400
NickNaso commented 5 years ago

PR on node-addon-api repo that depends on this backport activity:

We have to land this PR only after we end the backport.

mhdawson commented 4 years ago

Everything is landed, just waiting on at SemVer minor release of 10.x to go out.

NickNaso commented 4 years ago

I want report that the new the version 10.17.0 is out see: https://nodejs.org/en/blog/release/v10.17.0/

mhdawson commented 4 years ago

@gabrielschulhof given the 10.17.0 release can this be closed?

gabrielschulhof commented 4 years ago

N-API 5 has landed in all versions in which it will be supported. Closing.