-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
node-api: enable napi_ref for all value types #42557
Changes from all commits
579d44c
e108b5e
e629646
246e224
c794f31
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -108,6 +108,37 @@ typedef enum { | |
// * the definition of `napi_status` in doc/api/n-api.md to reflect the newly | ||
// added value(s). | ||
|
||
#ifdef NAPI_EXPERIMENTAL | ||
// Features allow changing internal behavior of existing Node-API functions. | ||
// | ||
// We pass a node_api_features pointer to the napi_module struct | ||
// in the NAPI_MODULE_X macro. This macro is used for the module registration. | ||
// If the module is initialized without using this macro, then there will be | ||
// no features selected and the module will use the node_api_feature_none. | ||
// | ||
// Each Node-API version defines its own default set of features. | ||
// For the current version it can be accessed using node_api_default_features. | ||
// A module can override the set of its enabled features by adding | ||
// NODE_API_CUSTOM_FEATURES definition to the .gyp file and then defining the | ||
// value of the global node_api_module_features variable. | ||
// To check enabled features use the `node_api_is_feature_enabled` function. | ||
// | ||
// For example, to disables node_api_feature_reference_all_types: | ||
// node_api_features node_api_module_features = | ||
// node_api_default_features & ~node_api_feature_reference_all_types; | ||
typedef enum { | ||
// To be used when no features needs to be set. | ||
node_api_feature_none = 0, | ||
// Use napi_ref for all value types. | ||
// Not only objects, external objects, functions, and symbols as before. | ||
node_api_feature_reference_all_types = 1 << 0, | ||
// Each version of Node-API is going to have its own default set of features. | ||
node_api_default_experimental_features = node_api_feature_reference_all_types, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if we should have this, I think we should guard the availabilty to be able to turn on experimental features with NAPI_EXPERIMENTAL, but I'm not sure if having a set different than the default for the Node-API version makes sense. Otherwise somebody who wants to use any experimental API is also forced into to new defaults. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In case if someone does not want to use the new experimental default, they can always override it per module. |
||
// This variable must be conditionally set depending on the Node-API version. | ||
node_api_default_features = node_api_default_experimental_features, | ||
} node_api_features; | ||
#endif | ||
|
||
typedef napi_value(NAPI_CDECL* napi_callback)(napi_env env, | ||
napi_callback_info info); | ||
typedef void(NAPI_CDECL* napi_finalize)(napi_env env, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2495,9 +2495,11 @@ napi_status NAPI_CDECL napi_create_reference(napi_env env, | |
CHECK_ARG(env, result); | ||
|
||
v8::Local<v8::Value> v8_value = v8impl::V8LocalValueFromJsValue(value); | ||
if (!(v8_value->IsObject() || v8_value->IsFunction() || | ||
v8_value->IsSymbol())) { | ||
return napi_set_last_error(env, napi_invalid_arg); | ||
if (!env->IsFeatureEnabled(node_api_feature_reference_all_types)) { | ||
if (!(v8_value->IsObject() || v8_value->IsFunction() || | ||
v8_value->IsSymbol())) { | ||
return napi_set_last_error(env, napi_invalid_arg); | ||
} | ||
} | ||
|
||
v8impl::Reference* reference = | ||
|
@@ -3257,3 +3259,12 @@ napi_status NAPI_CDECL napi_is_detached_arraybuffer(napi_env env, | |
|
||
return napi_clear_last_error(env); | ||
} | ||
|
||
napi_status NAPI_CDECL node_api_is_feature_enabled(napi_env env, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if this should be in node_api.cc instead of this file. It is not tied to v8 right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is not tied to V8. I was thinking that we want to have features available for different JS engines. |
||
node_api_features feature, | ||
bool* result) { | ||
CHECK_ENV(env); | ||
CHECK_ARG(env, result); | ||
*result = env->IsFeatureEnabled(feature); | ||
return napi_clear_last_error(env); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,7 +38,12 @@ typedef struct napi_module { | |
napi_addon_register_func nm_register_func; | ||
const char* nm_modname; | ||
void* nm_priv; | ||
#ifdef NAPI_EXPERIMENTAL | ||
node_api_features* nm_features; | ||
void* reserved[3]; | ||
#else | ||
void* reserved[4]; | ||
#endif | ||
} napi_module; | ||
|
||
#define NAPI_MODULE_VERSION 1 | ||
|
@@ -73,16 +78,39 @@ typedef struct napi_module { | |
static void fn(void) | ||
#endif | ||
|
||
#ifdef NAPI_EXPERIMENTAL | ||
#ifdef NODE_API_CUSTOM_FEATURES | ||
|
||
// Define value of node_api_module_features variable in your module when | ||
// NODE_API_CUSTOM_FEATURES is set in gyp file. | ||
extern node_api_features node_api_module_features; | ||
#define NODE_API_DEFINE_DEFAULT_FEATURES | ||
|
||
#else // NODE_API_CUSTOM_FEATURES | ||
|
||
#define NODE_API_DEFINE_DEFAULT_FEATURES \ | ||
static node_api_features node_api_module_features = node_api_default_features; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comments as before, I think this will end up setting to be specific to the Node.js version used to build versus the Node-API version selected. Should discuss this more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure how to address this. Let's discuss it in the Node-API meeting. The concern as I understand it is that if we add more experimental features, we may fail to propagate them to the previous versions of Node.JS, and thus it may cause differences in the behavior between Node.JS versions, while we have the same version of Node-API, right? |
||
|
||
#endif // NODE_API_CUSTOM_FEATURES | ||
|
||
#define NODE_API_FEATURES_PTR /* NOLINT */ &node_api_module_features, | ||
|
||
#else // NAPI_EXPERIMENTAL | ||
#define NODE_API_DEFINE_DEFAULT_FEATURES | ||
#define NODE_API_FEATURES_PTR | ||
#endif // NAPI_EXPERIMENTAL | ||
|
||
#define NAPI_MODULE_X(modname, regfunc, priv, flags) \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we just create a new NAPI_MODULE_X_FEATURES which takes the features parameter? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It may be simpler - let me try. |
||
EXTERN_C_START \ | ||
NODE_API_DEFINE_DEFAULT_FEATURES \ | ||
static napi_module _module = { \ | ||
NAPI_MODULE_VERSION, \ | ||
flags, \ | ||
__FILE__, \ | ||
regfunc, \ | ||
#modname, \ | ||
priv, \ | ||
{0}, \ | ||
NODE_API_FEATURES_PTR{0}, \ | ||
}; \ | ||
NAPI_C_CTOR(_register_##modname) { napi_module_register(&_module); } \ | ||
EXTERN_C_END | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
#define NAPI_EXPERIMENTAL | ||
#include <node_api.h> | ||
|
||
EXTERN_C_START | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,8 @@ | |
"sources": [ | ||
"../entry_point.c", | ||
"test_reference.c" | ||
] | ||
], | ||
'defines': [ 'NODE_API_CUSTOM_FEATURES' ] | ||
} | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
{ | ||
"targets": [ | ||
{ | ||
"target_name": "test_reference_all_types", | ||
"sources": [ | ||
"../entry_point.c", | ||
"test_reference_all_types.c" | ||
] | ||
} | ||
] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For compatiblity I'm wondering if we should tie the defaults to a Node-API version versus the vesion of Node.js Unless the module author choses to opt-in, they behaviour should not change unless they have moved up to a newer Node-API vesion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking more closely I think that is the approach that is described, but but nothing is guarded because its under experimental. Will make a comment in src/js_native_api_types.h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My intent is to make it per Node-API version.