Address PR feedback (nodejs#201)

 - Consistent documentation for --napi-modules
 - Add underscore to napi_get_property_names
 - Avoid using V8 APIs marked as pending deprecation
 - Avoid unnecessary copying of args arrays
 - Other miscellaneous cleanup

Author: jasongin

doc/node.1

@@ -121,7 +121,8 @@ Silence all process warnings (including deprecations).
 
 .TP
 .BR \-\-napi\-modules
-Load N-API modules (experimental, opt-in by adding this flag).
+Enable loading native modules compiled with the ABI-stable Node.js API (N-API)
+(experimental).
 
 .TP
 .BR \-\-trace\-warnings

src/node_api.cc

@@ -186,7 +186,7 @@ class Reference {
     }
 
     if (delete_self) {
-      delete reference;
+      Delete(reference);
     }
   }
 
@@ -237,7 +237,6 @@ class CallbackWrapper {
   CallbackWrapper(napi_value this_arg, size_t args_length, void* data)
       : _this(this_arg), _args_length(args_length), _data(data) {}
 
-  virtual napi_value Holder() = 0;
   virtual bool IsConstructCall() = 0;
   virtual void Args(napi_value* buffer, size_t bufferlength) = 0;
   virtual void SetReturnValue(napi_value value) = 0;
@@ -254,7 +253,7 @@ class CallbackWrapper {
   void* _data;
 };
 
-template <typename Info, int InternalFieldIndex>
+template <typename Info, int kInternalFieldIndex>
 class CallbackWrapperBase : public CallbackWrapper {
  public:
   CallbackWrapperBase(const Info& cbinfo, const size_t args_length)
@@ -267,11 +266,6 @@ class CallbackWrapperBase : public CallbackWrapper {
                 ->Value();
   }
 
-  /*virtual*/
-  napi_value Holder() override {
-    return JsValueFromV8LocalValue(_cbinfo.Holder());
-  }
-
   /*virtual*/
   bool IsConstructCall() override { return false; }
 
@@ -281,7 +275,7 @@ class CallbackWrapperBase : public CallbackWrapper {
         static_cast<CallbackWrapper*>(this));
     napi_callback cb = reinterpret_cast<napi_callback>(
         v8::Local<v8::External>::Cast(
-            _cbdata->GetInternalField(InternalFieldIndex))->Value());
+            _cbdata->GetInternalField(kInternalFieldIndex))->Value());
     v8::Isolate* isolate = _cbinfo.GetIsolate();
     cb(v8impl::JsEnvFromV8Isolate(isolate), cbinfo_wrapper);
 
@@ -634,7 +628,11 @@ napi_status napi_create_function(napi_env env,
   v8::Local<v8::FunctionTemplate> tpl = v8::FunctionTemplate::New(
       isolate, v8impl::FunctionCallbackWrapper::Invoke, cbdata);
 
-  return_value = scope.Escape(tpl->GetFunction());
+  v8::Local<v8::Context> context = isolate->GetCurrentContext();
+  v8::MaybeLocal<v8::Function> maybe_function = tpl->GetFunction(context);
+  CHECK_MAYBE_EMPTY(maybe_function, napi_generic_failure);
+
+  return_value = scope.Escape(maybe_function.ToLocalChecked());
 
   if (utf8name != nullptr) {
     v8::Local<v8::String> name_string;
@@ -713,7 +711,7 @@ napi_status napi_define_class(napi_env env,
 
       tpl->PrototypeTemplate()->SetAccessor(
         property_name,
-        v8impl::GetterCallbackWrapper::Invoke,
+        p->getter ? v8impl::GetterCallbackWrapper::Invoke : nullptr,
         p->setter ? v8impl::SetterCallbackWrapper::Invoke : nullptr,
         cbdata,
         v8::AccessControl::DEFAULT,
@@ -740,7 +738,7 @@ napi_status napi_define_class(napi_env env,
     napi_status status =
         napi_define_properties(env,
                                *result,
-                               static_cast<int>(static_descriptors.size()),
+                               static_descriptors.size(),
                                static_descriptors.data());
     if (status != napi_ok) return status;
   }
@@ -760,9 +758,9 @@ napi_status napi_set_return_value(napi_env env,
   return GET_RETURN_STATUS();
 }
 
-napi_status napi_get_propertynames(napi_env env,
-                                   napi_value object,
-                                   napi_value* result) {
+napi_status napi_get_property_names(napi_env env,
+                                    napi_value object,
+                                    napi_value* result) {
   NAPI_PREAMBLE(env);
   CHECK_ARG(result);
 
@@ -1028,27 +1026,23 @@ napi_status napi_define_properties(napi_env env,
       auto set_maybe = obj->SetAccessor(
           context,
           name,
-          v8impl::GetterCallbackWrapper::Invoke,
+          p->getter ? v8impl::GetterCallbackWrapper::Invoke : nullptr,
           p->setter ? v8impl::SetterCallbackWrapper::Invoke : nullptr,
           cbdata,
           v8::AccessControl::DEFAULT,
           attributes);
 
-      // IsNothing seems like a serious failure,
-      // should we return a different error code if the set failed?
-      if (set_maybe.IsNothing() || !set_maybe.FromMaybe(false)) {
-        return napi_set_last_error(napi_generic_failure);
+      if (!set_maybe.FromMaybe(false)) {
+        return napi_set_last_error(napi_invalid_arg);
       }
     } else {
       v8::Local<v8::Value> value = v8impl::V8LocalValueFromJsValue(p->value);
 
       auto define_maybe =
           obj->DefineOwnProperty(context, name, value, attributes);
 
-      // IsNothing seems like a serious failure,
-      // should we return a different error code if the define failed?
-      if (define_maybe.IsNothing() || !define_maybe.FromMaybe(false)) {
-        return napi_set_last_error(napi_generic_failure);
+      if (!define_maybe.FromMaybe(false)) {
+        return napi_set_last_error(napi_invalid_arg);
       }
     }
   }
@@ -1430,21 +1424,17 @@ napi_status napi_call_function(napi_env env,
   NAPI_PREAMBLE(env);
   CHECK_ARG(result);
 
-  std::vector<v8::Local<v8::Value>> args(argc);
   v8::Isolate* isolate = v8impl::V8IsolateFromJsEnv(env);
   v8::Local<v8::Context> context = isolate->GetCurrentContext();
 
   v8::Local<v8::Value> v8recv = v8impl::V8LocalValueFromJsValue(recv);
 
-  for (size_t i = 0; i < argc; i++) {
-    args[i] = v8impl::V8LocalValueFromJsValue(argv[i]);
-  }
-
   v8::Local<v8::Value> v8value = v8impl::V8LocalValueFromJsValue(func);
   RETURN_STATUS_IF_FALSE(v8value->IsFunction(), napi_invalid_arg);
 
   v8::Local<v8::Function> v8func = v8value.As<v8::Function>();
-  auto maybe = v8func->Call(context, v8recv, argc, args.data());
+  auto maybe = v8func->Call(context, v8recv, argc,
+    reinterpret_cast<v8::Local<v8::Value>*>(const_cast<napi_value*>(argv)));
 
   if (try_catch.HasCaught()) {
     return napi_set_last_error(napi_pending_exception);
@@ -1809,8 +1799,9 @@ napi_status napi_unwrap(napi_env env, napi_value js_object, void** result) {
   CHECK_ARG(js_object);
   CHECK_ARG(result);
 
-  v8::Local<v8::Object> obj =
-      v8impl::V8LocalValueFromJsValue(js_object).As<v8::Object>();
+  v8::Local<v8::Value> value = v8impl::V8LocalValueFromJsValue(js_object);
+  RETURN_STATUS_IF_FALSE(value->IsObject(), napi_invalid_arg);
+  v8::Local<v8::Object> obj = value.As<v8::Object>();
 
   // Only objects that were created from a NAPI constructor's prototype
   // via napi_define_class() can be (un)wrapped.
@@ -2014,17 +2005,14 @@ napi_status napi_new_instance(napi_env env,
   v8::Isolate* isolate = v8impl::V8IsolateFromJsEnv(env);
   v8::Local<v8::Context> context = isolate->GetCurrentContext();
 
-  std::vector<v8::Local<v8::Value>> args(argc);
-  for (size_t i = 0; i < argc; i++) {
-    args[i] = v8impl::V8LocalValueFromJsValue(argv[i]);
-  }
-
   v8::Local<v8::Value> v8value = v8impl::V8LocalValueFromJsValue(constructor);
   RETURN_STATUS_IF_FALSE(v8value->IsFunction(), napi_invalid_arg);
 
   v8::Local<v8::Function> ctor = v8value.As<v8::Function>();
 
-  auto maybe = ctor->NewInstance(context, argc, args.data());
+  auto maybe = ctor->NewInstance(context, argc,
+    reinterpret_cast<v8::Local<v8::Value>*>(const_cast<napi_value*>(argv)));
+
   CHECK_MAYBE_EMPTY(maybe, napi_generic_failure);
 
   *result = v8impl::JsValueFromV8LocalValue(maybe.ToLocalChecked());
@@ -2095,19 +2083,19 @@ napi_status napi_instanceof(napi_env env,
   v8::Local<v8::String> prototype_string;
   CHECK_NEW_FROM_UTF8(isolate, prototype_string, "prototype");
 
-  auto maybe = ctor->Get(context, prototype_string);
-
-  CHECK_MAYBE_EMPTY(maybe, napi_generic_failure);
-
-  v8::Local<v8::Value> prototype_property = maybe.ToLocalChecked();
+  auto maybe_prototype = ctor->Get(context, prototype_string);
+  CHECK_MAYBE_EMPTY(maybe_prototype, napi_generic_failure);
 
+  v8::Local<v8::Value> prototype_property = maybe_prototype.ToLocalChecked();
   if (!prototype_property->IsObject()) {
-    napi_throw_type_error(env, "constructor prototype must be an object");
+    napi_throw_type_error(env, "constructor.prototype must be an object");
 
     return napi_set_last_error(napi_object_expected);
   }
 
-  ctor = prototype_property->ToObject();
+  auto maybe_ctor = prototype_property->ToObject(context);
+  CHECK_MAYBE_EMPTY(maybe_ctor, napi_generic_failure);
+  ctor = maybe_ctor.ToLocalChecked();
 
   v8::Local<v8::Value> current_obj = v8impl::V8LocalValueFromJsValue(object);
   if (!current_obj->StrictEquals(ctor)) {
@@ -2143,13 +2131,10 @@ napi_status napi_make_callback(napi_env env,
       v8impl::V8LocalValueFromJsValue(recv).As<v8::Object>();
   v8::Local<v8::Function> v8func =
       v8impl::V8LocalValueFromJsValue(func).As<v8::Function>();
-  std::vector<v8::Local<v8::Value>> args(argc);
-  for (size_t i = 0; i < argc; i++) {
-    args[i] = v8impl::V8LocalValueFromJsValue(argv[i]);
-  }
 
   *result = v8impl::JsValueFromV8LocalValue(
-    node::MakeCallback(isolate, v8recv, v8func, argc, args.data()));
+    node::MakeCallback(isolate, v8recv, v8func, argc,
+      reinterpret_cast<v8::Local<v8::Value>*>(const_cast<napi_value*>(argv))));
 
   return GET_RETURN_STATUS();
 }

src/node_api.h

@@ -71,10 +71,10 @@ typedef struct {
 #ifdef __cplusplus
 #define EXTERN_C_START extern "C" {
 #define EXTERN_C_END }
-#else /* ndef __cplusplus */
+#else
 #define EXTERN_C_START
 #define EXTERN_C_END
-#endif /* def __cplusplus */
+#endif
 
 #define NAPI_MODULE_X(modname, regfunc, priv, flags)                  \
   EXTERN_C_START                                                      \
@@ -185,7 +185,7 @@ NAPI_EXTERN napi_status napi_get_value_string_utf16(napi_env env,
                                                     size_t* result);
 
 // Methods to coerce values
-// These APIs may execute user script
+// These APIs may execute user scripts
 NAPI_EXTERN napi_status napi_coerce_to_bool(napi_env env,
                                             napi_value value,
                                             napi_value* result);
@@ -203,9 +203,9 @@ NAPI_EXTERN napi_status napi_coerce_to_string(napi_env env,
 NAPI_EXTERN napi_status napi_get_prototype(napi_env env,
                                            napi_value object,
                                            napi_value* result);
-NAPI_EXTERN napi_status napi_get_propertynames(napi_env env,
-                                               napi_value object,
-                                               napi_value* result);
+NAPI_EXTERN napi_status napi_get_property_names(napi_env env,
+                                                napi_value object,
+                                                napi_value* result);
 NAPI_EXTERN napi_status napi_set_property(napi_env env,
                                           napi_value object,
                                           napi_value key,

src/node_api_types.h

@@ -43,8 +43,6 @@ typedef struct {
   void* data;
 } napi_property_descriptor;
 
-#define DEFAULT_ATTR 0, 0, 0, napi_default, 0
-
 typedef enum {
   // ES6 types (corresponds to typeof)
   napi_undefined,

test/addons-napi/test_object/test_object.c

@@ -195,7 +195,7 @@ void Inflate(napi_env env, napi_callback_info info) {
   napi_value obj = args[0];
 
   napi_value propertynames;
-  status = napi_get_propertynames(env, obj, &propertynames);
+  status = napi_get_property_names(env, obj, &propertynames);
   if (status != napi_ok) return;
 
   uint32_t i, length;