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

doc: clarify N-API version 1 #34344

Closed
wants to merge 9 commits into from
Closed

Conversation

mhdawson
Copy link
Member

Refs: nodejs/node-addon-api#760

Clarify which version of 8.x in which N-API version 1
matches the shape in later versions like 10.x

Signed-off-by: Michael Dawson michael_dawson@ca.ibm.com

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

Refs: nodejs/node-addon-api#760

Clarify which version of 8.x in which N-API version 1
matches the shape in later versions like 10.x

Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com>
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. node-api Issues and PRs related to the Node-API. labels Jul 13, 2020
doc/api/n-api.md Outdated Show resolved Hide resolved
doc/api/n-api.md Outdated Show resolved Hide resolved
Co-authored-by: Anna Henningsen <github@addaleax.net>
doc/api/n-api.md Outdated Show resolved Hide resolved
doc/api/n-api.md Outdated Show resolved Hide resolved
@nodejs nodejs deleted a comment from Trott Jul 20, 2020
doc/api/n-api.md Outdated Show resolved Hide resolved
@mhdawson
Copy link
Member Author

hmm, meant to delete my suggestion that made no change but seem to have deleted other comments as well. @Trott sorry about that.

In any case, hopefully fixed up the linting issue now, we'll see if the linters pass.

Copy link
Contributor

@gabrielschulhof gabrielschulhof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a few comments.

doc/api/n-api.md Outdated Show resolved Hide resolved
included N-API as experimental, version 1 continued to evolve until
v8.7.0 and therefore the shape of the API in earlier versions is not
truly version 1 (in hindsight we should have called it version 0).
We recommend version 3 or later.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
We recommend version 3 or later.
This also means that our ABI stability guarantee starts at version v8.7.0.
We recommend version 3 or later.

Feel free to rephrase or even discard. The objective I'm trying to achieve with the suggestion is that the final phrasing include a reference to ABI stability and the earliest version which is ABI-compatible with what we have today.

Is v8.7.0 truly the first version of Node.js whose N-API implementation is ABI-compatible with today's? IIRC we had the final ABI in place even while we were still in experimental. Of course, it may not be worth pushing back the documented number a couple of minor versions just to "maximize our coverage".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gabrielschulhof 8.7.0 is the earliest level that @devsnek reported as being compatible so believe it was the earliest.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gabrielschulhof I think I'd also prefer to leave out the extra sentence as I think it restates what the earlier lines say in a bit more detail.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know exactly. I gauged it based on when node-addon-api headers didn't have undefined symbols and such. I don't exhaustively test if it actually works.

Copy link
Member Author

@mhdawson mhdawson Jul 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diff between 8.7.0 and the last 8.x version (8.17.0) for node_api.h is:

iff --git a/src/node_api.h b/src/node_api.h
index a3a07a6..f683f8d 100644
--- a/src/node_api.h
+++ b/src/node_api.h
@@ -1,19 +1,23 @@
-/******************************************************************************
- * Experimental prototype for demonstrating VM agnostic and ABI stable API
- * for native modules to use instead of using Nan and V8 APIs directly.
- *
- * The current status is "Experimental" and should not be used for
- * production applications.  The API is still subject to change
- * and as an experimental feature is NOT subject to semver.
- *
- ******************************************************************************/
 #ifndef SRC_NODE_API_H_
 #define SRC_NODE_API_H_

 #include <stddef.h>
 #include <stdbool.h>
+
+#ifndef NAPI_VERSION
+#ifdef NAPI_EXPERIMENTAL
+// Use INT_MAX, this should only be consumed by the pre-processor anyway.
+#define NAPI_VERSION 2147483647
+#else
+// The baseline version for N-API
+#define NAPI_VERSION 4
+#endif
+#endif
+
 #include "node_api_types.h"

+struct uv_loop_s;  // Forward declaration.
+
 #ifdef _WIN32
   #ifdef BUILDING_NODE_EXTENSION
     #ifdef EXTERNAL_NAPI
@@ -98,7 +102,7 @@ typedef struct {
   EXTERN_C_END

 #define NAPI_MODULE(modname, regfunc) \
-  NAPI_MODULE_X(modname, regfunc, NULL, 0)
+  NAPI_MODULE_X(modname, regfunc, NULL, 0)  // NOLINT (readability/null_usage)

 #define NAPI_AUTO_LENGTH SIZE_MAX

@@ -175,7 +179,7 @@ NAPI_EXTERN napi_status napi_create_range_error(napi_env env,
                                                 napi_value msg,
                                                 napi_value* result);

-// Methods to get the the native napi_value from Primitive type
+// Methods to get the native napi_value from Primitive type
 NAPI_EXTERN napi_status napi_typeof(napi_env env,
                                     napi_value value,
                                     napi_valuetype* result);
@@ -581,6 +585,76 @@ NAPI_EXTERN napi_status napi_run_script(napi_env env,
                                         napi_value script,
                                         napi_value* result);

+#if NAPI_VERSION >= 2
+
+// Return the current libuv event loop for a given environment
+NAPI_EXTERN napi_status napi_get_uv_event_loop(napi_env env,
+                                               struct uv_loop_s** loop);
+
+#endif  // NAPI_VERSION >= 2
+
+#if NAPI_VERSION >= 3
+
+NAPI_EXTERN napi_status napi_open_callback_scope(napi_env env,
+                                                 napi_value resource_object,
+                                                 napi_async_context context,
+                                                 napi_callback_scope* result);
+
+NAPI_EXTERN napi_status napi_close_callback_scope(napi_env env,
+                                                  napi_callback_scope scope);
+
+NAPI_EXTERN napi_status napi_fatal_exception(napi_env env, napi_value err);
+
+NAPI_EXTERN napi_status napi_add_env_cleanup_hook(napi_env env,
+                                                  void (*fun)(void* arg),
+                                                  void* arg);
+
+NAPI_EXTERN napi_status napi_remove_env_cleanup_hook(napi_env env,
+                                                     void (*fun)(void* arg),
+                                                     void* arg);
+
+#endif  // NAPI_VERSION >= 3
+
+#if NAPI_VERSION >= 4
+
+// Calling into JS from other threads
+NAPI_EXTERN napi_status
+napi_create_threadsafe_function(napi_env env,
+                                napi_value func,
+                                napi_value async_resource,
+                                napi_value async_resource_name,
+                                size_t max_queue_size,
+                                size_t initial_thread_count,
+                                void* thread_finalize_data,
+                                napi_finalize thread_finalize_cb,
+                                void* context,
+                                napi_threadsafe_function_call_js call_js_cb,
+                                napi_threadsafe_function* result);
+
+NAPI_EXTERN napi_status
+napi_get_threadsafe_function_context(napi_threadsafe_function func,
+                                     void** result);
+
+
+NAPI_EXTERN napi_status
+napi_call_threadsafe_function(napi_threadsafe_function func,
+                              void* data,
+                              napi_threadsafe_function_call_mode is_blocking);
+
+NAPI_EXTERN napi_status
+napi_acquire_threadsafe_function(napi_threadsafe_function func);
+
+NAPI_EXTERN napi_status
+napi_release_threadsafe_function(napi_threadsafe_function func,
+                                 napi_threadsafe_function_release_mode mode);
+
+NAPI_EXTERN napi_status
+napi_unref_threadsafe_function(napi_env env, napi_threadsafe_function func);
+
+NAPI_EXTERN napi_status
+napi_ref_threadsafe_function(napi_env env, napi_threadsafe_function func);
+
+#endif  // NAPI_VERSION >= 4
+
 EXTERN_C_END

 #endif  // SRC_NODE_API_H_

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the additions I don't see any API changes from 8.7.0 and the last 8.x version in the diff above (+struct uv_loop_s; // Forward declaration. was added as part of the NAPI_VERSION_2 addition, not sure why we did that but that's a separate issue)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

git diff v8.6.0..v8.7.0 src/node_api.h shows no changes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

git diff v8.5.0..v8.6.0 src/node_api.h show a number of changes in the API.

I think that would likely say that v8.6.0 is the right one to specify. There don't seem to be any changes in the api file between v8.6.0 that and the last 8.x version which would affect the API shape. It was still experimental so there would be no guarantees about it working properly or internal behavior changing but that is not relevant to us adding the comment about when the shape of V1 was finalized.

@devsnek I'll update to v8.6.0 unless what you saw contradicts that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checking this again, it appears to be 8.6.0, not 8.7.0. 8.5.0 is missing napi_async_context and NAPI_AUTO_LENGTH which are both in version 1.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@devsnek thanks for confirming, we must have both been investigating at the same time.

Co-authored-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
doc/api/n-api.md Outdated Show resolved Hide resolved
@mhdawson
Copy link
Member Author

@gabrielf , @devsnek updated to say v8.6.0 and I think that's correct based on the info I pasted above.

doc/api/n-api.md Outdated Show resolved Hide resolved
Co-authored-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
@mhdawson mhdawson added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 29, 2020
@mhdawson
Copy link
Member Author

Only changes doc, so once linters complete running on the latest change I'll go ahead and land.

mhdawson added a commit that referenced this pull request Jul 29, 2020
Refs: nodejs/node-addon-api#760

Clarify which version of 8.x in which N-API version 1
matches the shape in later versions like 10.x

Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com>

PR-URL: #34344
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
@mhdawson
Copy link
Member Author

Landed in 15333ad

@mhdawson mhdawson closed this Jul 29, 2020
codebytere pushed a commit that referenced this pull request Aug 5, 2020
Refs: nodejs/node-addon-api#760

Clarify which version of 8.x in which N-API version 1
matches the shape in later versions like 10.x

Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com>

PR-URL: #34344
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
@codebytere codebytere mentioned this pull request Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants