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.

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

What should go into N-API 5? #374

Closed
gabrielschulhof opened this issue Jun 3, 2019 · 20 comments
Closed

What should go into N-API 5? #374

gabrielschulhof opened this issue Jun 3, 2019 · 20 comments
Milestone

Comments

@gabrielschulhof
Copy link
Collaborator

@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 gabrielschulhof added this to the Milestone 11 milestone Jun 3, 2019
@gabrielschulhof
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

@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
Copy link
Contributor

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
Copy link
Member

devsnek commented Jun 3, 2019

@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
Copy link

josephg commented Jun 3, 2019

@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:

  • Extract the value from foundationdb's future object (if any)
  • Pass the value back into javascript via an associated callback or future
  • Cleanup foundationdb's future object
  • ref / unref the event loop so the process can terminate cleanly

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
Copy link

@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.

TryGhost/node-sqlite3#830 (comment)
mhdawson/node-sqlite3#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
Copy link
Member

devsnek commented Jun 4, 2019

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
Copy link
Member

mhdawson commented Jun 4, 2019

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
Copy link
Member

devsnek commented Jun 4, 2019

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
Copy link

cjihrig commented Jun 4, 2019

@mhdawson if you haven't seen it yet, nodejs/node#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
Copy link

josephg commented Jun 5, 2019

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
Copy link
Member

devsnek commented Jun 5, 2019

consider me nerd-sniped :P

@gabrielschulhof
Copy link
Collaborator Author

@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
Copy link
Member

mhdawson commented Jul 8, 2019

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
Copy link
Collaborator Author

gabrielschulhof commented Jul 8, 2019

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
Copy link
Member

NickNaso commented Jul 17, 2019

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
Copy link
Member

mhdawson commented Oct 7, 2019

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

@NickNaso
Copy link
Member

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

@mhdawson
Copy link
Member

@gabrielschulhof given the 10.17.0 release can this be closed?

@gabrielschulhof
Copy link
Collaborator Author

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

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

No branches or pull requests

8 participants