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

Graduate experimental N-API feature napi_threadsafe_function #24249

Closed
pipobscure opened this issue Nov 8, 2018 · 9 comments
Closed

Graduate experimental N-API feature napi_threadsafe_function #24249

pipobscure opened this issue Nov 8, 2018 · 9 comments
Labels
node-api Issues and PRs related to the Node-API.

Comments

@pipobscure
Copy link
Contributor

I reworked fsevents to use N-API. For that I had to use the napi_threadsafe_function family of features, which are currently behind experimental. They work like a charm.

Can we graduate these out of experimental, backport them to v8.x and bump the N-API version?

See: fsevents/fsevents#237

(Came out of a discussion with @mhdawson)

@pipobscure
Copy link
Contributor Author

Interestingly enough, I found that the napi_threadsafe_function family of features are actually entirely unnecessary. That is because the entire 1.x branch of libuv is API/ABI stable to begin with.

Now that being said, it's a nice wrapper that may be easier to use than interacting directly with libuv, so I'm not saying it should be dropped.

@targos
Copy link
Member

targos commented Nov 11, 2018

@nodejs/n-api

@targos targos added the node-api Issues and PRs related to the Node-API. label Nov 11, 2018
@mhdawson
Copy link
Member

I'd like to correct that I still don't believe that we can depend on libuv stability. When Node.js moves to a new major version things will break. If you want ABI stability you can only depend on the functions which are part of N-API. It may be possible to implement checks/binaries for different Node.js versions with different major versions of libuv but nothing in our existing versioning will help with that (might be something to look at if we believe updates to the libuv major version are going to be far enough apart).

Based on this real-world use/validation as well as prior use I'd suggest we graduation napi_threadsafe_function from experimental

@mhdawson
Copy link
Member

@gabrielschulhof

@gabrielschulhof
Copy link
Contributor

@mhdawson this means we should create version 4 of N-API. What else can we bundle into it?

@gabrielschulhof
Copy link
Contributor

We concluded at today's N-API meeting that napi_threadsafe_function is the only API we could include in version 4, and that it would require a backport to Node.js v8.x before we could consider it stable.

@pipobscure
Copy link
Contributor Author

pipobscure commented Dec 28, 2018

Created a new PR for fsevents using these: fsevents/fsevents#247
So now I'm on hold/waiting for the backport for v8.x in #25002

@pipobscure
Copy link
Contributor Author

This is the latest version of fsevents based on napi. fsevents/fsevents#247

However to be able to release somewhat smoothly #25002 needs to be in.

@mhdawson
Copy link
Member

We talked today in the N-API team meeting and @gabrielschulhof is going to going to create a PR to move it out of experimental on master. The goal is to that that backported for a potential SemVer minor 8.x release being planned by the Release WG.

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this issue Jan 18, 2019
targos pushed a commit that referenced this issue Jan 24, 2019
Fixes: #24249
PR-URL: #25556
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this issue Jan 29, 2019
Fixes: nodejs#24249
PR-URL: nodejs#25556
Backport-PR-URL: nodejs#25633
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this issue Mar 4, 2019
Fixes: nodejs#24249
PR-URL: nodejs#25556
Backport-PR-URL: nodejs#25633
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
BethGriggs pushed a commit that referenced this issue Mar 12, 2019
Fixes: #24249
PR-URL: #25556
Backport-PR-URL: #25633
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
BethGriggs pushed a commit that referenced this issue Mar 20, 2019
Fixes: #24249
PR-URL: #25556
Backport-PR-URL: #25648
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants