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

Update node_api modules at src/ with threadsafe functionality #509

Closed
markBETA opened this issue Jul 11, 2019 · 9 comments
Closed

Update node_api modules at src/ with threadsafe functionality #509

markBETA opened this issue Jul 11, 2019 · 9 comments

Comments

@markBETA
Copy link

Hi to everyone!

I have tried to compile with the last update with the ThreadSafeFunction class but it seems that the node_api modules at the src/ folder are not updated with the threadsafe functionality.
I think that is needed to update these files for using the new functionality.

@mhdawson
Copy link
Member

Those files should not be being used for any active LTS version. Instead the code being used should come from the Node runtime.

@gabrielschulhof
Copy link
Contributor

@mhdawson we need to drop the N-API implementation from node-addon-api once v8.x becomes unmaintained.

@mhdawson
Copy link
Member

@gabrielschulhof agreed :)

@mhdawson
Copy link
Member

mhdawson commented Jul 11, 2019

@markBETA I had meant to ask what version of Node are you using. I see that I missed that in my first post.

@markBETA
Copy link
Author

@mhdawson I tried to use the source files from the Node runtime and it worked! I'm using the last LTS version (10.16.0), where the threadsafe operations are marked as stable.

I'm working with Cmake-JS to compile the source files of my addon. I think it will be good to specify in the readme to use the files from the Node runtime. As @gabrielschulhof said, I think it will be good to remove the src/ folder that is not maintained anymore.

@mhdawson
Copy link
Member

@markBETA thanks. By default if you use node-gyp it will find the right files but seems like for other cases we need more doc. Any chance you'd be able to submit a PR to the README.md to add what you would have found useful?

@ghost
Copy link

ghost commented Aug 15, 2019

Yes the src folder with old files is highly confusing. Would be nice to have it deleted.

@marco-ms
Copy link

marco-ms commented Aug 17, 2019

Hi,
I am also exploring node-addon-api v1.7.1 and ThreadSafeFunction. Sorry if I am off topic, but I got a bit overwhelmed regarding documentation and compatibility matrix, I'll make some statements, feel free to correct me, whenever they are incorrect.

  1. I understand that to work ThreadSafeFunction requires N-API 4
  2. N-API 4 is supported by: Node 8.16.0+, 11.8.0+ or 12.0.0+
    https://nodejs.org/docs/latest-v12.x/api/n-api.html#n_api_n_api_version_matrix
  3. If you have a Node that supports N-API 3 you can still build correctly, but you need N-API 4 support to run it (the phrase in that url leaves a bit to interpretation).
  4. If I am using my add-on onto Electron, I need an Electron version that runs at least a Node version taken from list in point 2.
  5. Which Node exactly is running Electron 3.1? All I could find is Electron 3.1 forked from Electron 3.0 which is running Node 10.2.0 https://electronjs.org/blog/electron-3-0 (unsure)
  6. Electron 3.x does NOT support N-API 4
  7. Even if I compile my node module with N-API 4 compatible Node (say Node 8.16.0), running on Electron 3.x won't make ThreadSafeFunction to work (possibly won't even run?)
  8. If all my previous statements are correct, what are my options if N-API 4 is not supported to actually syncronize a native callback back to the main thread in order to post results to JavaScript? If I remove it currently my node add-on crashes, so I need a solution.

@mhdawson looking at this documentation[1] I find very hard if not impossible to get the correct information regarding "API X will work with my Electron Y.Z"? It looks to me like a maze, but maybe is because new to this API.

[1] https://github.com/nodejs/node-addon-api/blob/master/doc/threadsafe_function.md

Thank you so much for your attention and sorry if this is off topic.

@mhdawson
Copy link
Member

Unfortunately, I'm not up to speed on the mapping between Node versions and Electron. Maybe @codebytere can help.

If you have a Node that supports N-API 3 you can still build correctly, but you need N-API 4 support to run it (the phrase in that url leaves a bit to interpretation).

I don't think this is the case. You need a Node.js version which supports N-API 4 to build using N-API v4 features. The exception is if you enable EXPERIMENTALly and those features were EXPERIMENTAL in the earlier Node.js version. In which case you may build but them later not run if in a Node.js version that does not support v4 features (or have the feature as EXPERIMENTAL)

If all my previous statements are correct, what are my options if N-API 4 is not supported to actually syncronize a native callback back to the main thread in order to post results to JavaScript? If I remove it currently my node add-on crashes, so I need a solution.

Unfortunately, there won't be a great answer to this. We've added the ThreadSafeFunction as it became apparent that it was a common use case. You might be able to look at the methods within the C N-API and implement those yourself, but that will likely mean using internal APIs and that the native module would need ot be re-compiled for each version of Node.js (although that's no worse than without N-API). Not sure how easy it will be to bridge between the non N-API and N-API code required to do that either.

gabrielschulhof pushed a commit to gabrielschulhof/node-addon-api that referenced this issue Jan 1, 2020
Remove the files associated with the external implementation of N-API.
Making this move is possible because all supported versions of Node.js
now have an internal implementation of N-API.

Fixes: nodejs#463
Fixes: nodejs#509
gabrielschulhof pushed a commit to gabrielschulhof/node-addon-api that referenced this issue Jan 1, 2020
Remove the files associated with the external implementation of N-API
and the v8.x Travis CI testing. This move is possible because of v8.x
EOL, which means that all supported versions of Node.js now have an
internal implementation of N-API.

Fixes: nodejs#463
Fixes: nodejs#509
gabrielschulhof pushed a commit to gabrielschulhof/node-addon-api that referenced this issue Jan 2, 2020
Remove the files associated with the external implementation of N-API
and the v8.x Travis CI testing. This move is possible because of v8.x
EOL, which means that all supported versions of Node.js now have an
internal implementation of N-API.

Fixes: nodejs#463
Fixes: nodejs#509
Fixes: nodejs#640
gabrielschulhof pushed a commit to gabrielschulhof/node-addon-api that referenced this issue Jan 3, 2020
Remove the followings:

* the files associated with the external implementation of N-API
* Travis CI jobs for v8.x and v6.x
* documentation instructing users to add the external N-API
  implementation to their dependencies.
* conversion tool code that adds the external N-API implementation as a
  dependency to the user's addon.

This move is possible because of v8.x EOL, which means that all
supported versions of Node.js now have an internal implementation of
N-API.

Fixes: nodejs#463
Fixes: nodejs#509
Fixes: nodejs#640
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this issue Aug 24, 2022
Remove the followings:

* the files associated with the external implementation of N-API
* Travis CI jobs for v8.x and v6.x
* documentation instructing users to add the external N-API
  implementation to their dependencies.
* conversion tool code that adds the external N-API implementation as a
  dependency to the user's addon.

This move is possible because of v8.x EOL, which means that all
supported versions of Node.js now have an internal implementation of
N-API.

Fixes: nodejs/node-addon-api#463
Fixes: nodejs/node-addon-api#509
Fixes: nodejs/node-addon-api#640
PR-URL: nodejs/node-addon-api#643
Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this issue Aug 26, 2022
Remove the followings:

* the files associated with the external implementation of N-API
* Travis CI jobs for v8.x and v6.x
* documentation instructing users to add the external N-API
  implementation to their dependencies.
* conversion tool code that adds the external N-API implementation as a
  dependency to the user's addon.

This move is possible because of v8.x EOL, which means that all
supported versions of Node.js now have an internal implementation of
N-API.

Fixes: nodejs/node-addon-api#463
Fixes: nodejs/node-addon-api#509
Fixes: nodejs/node-addon-api#640
PR-URL: nodejs/node-addon-api#643
Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this issue Sep 19, 2022
Remove the followings:

* the files associated with the external implementation of N-API
* Travis CI jobs for v8.x and v6.x
* documentation instructing users to add the external N-API
  implementation to their dependencies.
* conversion tool code that adds the external N-API implementation as a
  dependency to the user's addon.

This move is possible because of v8.x EOL, which means that all
supported versions of Node.js now have an internal implementation of
N-API.

Fixes: nodejs/node-addon-api#463
Fixes: nodejs/node-addon-api#509
Fixes: nodejs/node-addon-api#640
PR-URL: nodejs/node-addon-api#643
Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
austinli64 added a commit to austinli64/node-addon-api that referenced this issue May 9, 2023
Remove the followings:

* the files associated with the external implementation of N-API
* Travis CI jobs for v8.x and v6.x
* documentation instructing users to add the external N-API
  implementation to their dependencies.
* conversion tool code that adds the external N-API implementation as a
  dependency to the user's addon.

This move is possible because of v8.x EOL, which means that all
supported versions of Node.js now have an internal implementation of
N-API.

Fixes: nodejs/node-addon-api#463
Fixes: nodejs/node-addon-api#509
Fixes: nodejs/node-addon-api#640
PR-URL: nodejs/node-addon-api#643
Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this issue Aug 11, 2023
Remove the followings:

* the files associated with the external implementation of N-API
* Travis CI jobs for v8.x and v6.x
* documentation instructing users to add the external N-API
  implementation to their dependencies.
* conversion tool code that adds the external N-API implementation as a
  dependency to the user's addon.

This move is possible because of v8.x EOL, which means that all
supported versions of Node.js now have an internal implementation of
N-API.

Fixes: nodejs/node-addon-api#463
Fixes: nodejs/node-addon-api#509
Fixes: nodejs/node-addon-api#640
PR-URL: nodejs/node-addon-api#643
Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
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

Successfully merging a pull request may close this issue.

4 participants