napi: Property & class definition improvements (#101)

 - Rename napi_create_constructor to napi_define_class and include support for defining static properties on the class at the same time
 - Rename napi_define_property to napi_define_properties to allow for defining multiple properties at the same time
 - Update tests for the changes

Author: jasongin

src/node_jsrtapi.cc

@@ -23,6 +23,7 @@
 #include <node_object_wrap.h>
 #include <env.h>
 #include <vector>
+#include <algorithm>
 #include "ChakraCore.h"
 
 #include "src/jsrtutils.h"
@@ -294,8 +295,13 @@ napi_status napi_create_function(napi_env e, napi_callback cb, void* data, napi_
   return napi_ok;
 }
 
-napi_status napi_create_constructor(napi_env e, const char* utf8name, napi_callback cb,
-  void* data, int property_count, napi_property_descriptor* properties, napi_value* result) {
+napi_status napi_define_class(napi_env e,
+                              const char* utf8name,
+                              napi_callback cb,
+                              void* data,
+                              int property_count,
+                              const napi_property_descriptor* properties,
+                              napi_value* result) {
   CHECK_ARG(result);
 
   napi_value namestring;
@@ -315,8 +321,41 @@ napi_status napi_create_constructor(napi_env e, const char* utf8name, napi_callb
   CHECK_JSRT(JsCreatePropertyIdUtf8("constructor", 12, &pid));
   CHECK_JSRT(JsSetProperty(prototype, pid, constructor, false));
 
+  int instancePropertyCount = 0;
+  int staticPropertyCount = 0;
   for (int i = 0; i < property_count; i++) {
-    CHECK_NAPI(napi_define_property(e, reinterpret_cast<napi_value>(prototype), &properties[i]));
+    if ((properties[i].attributes & napi_static_property) != 0) {
+      staticPropertyCount++;
+    }
+    else {
+      instancePropertyCount++;
+    }
+  }
+
+  std::vector<napi_property_descriptor> descriptors;
+  descriptors.reserve(std::max(instancePropertyCount, staticPropertyCount));
+
+  if (instancePropertyCount > 0) {
+    for (int i = 0; i < property_count; i++) {
+      if ((properties[i].attributes & napi_static_property) == 0) {
+        descriptors.push_back(properties[i]);
+      }
+    }
+
+    CHECK_NAPI(napi_define_properties(
+      e, reinterpret_cast<napi_value>(prototype), descriptors.size(), descriptors.data()));
+  }
+
+  if (staticPropertyCount > 0) {
+    descriptors.clear();
+    for (int i = 0; i < property_count; i++) {
+      if (!(properties[i].attributes & napi_static_property) != 0) {
+        descriptors.push_back(properties[i]);
+      }
+    }
+
+    CHECK_NAPI(napi_define_properties(
+      e, reinterpret_cast<napi_value>(constructor), descriptors.size(), descriptors.data()));
   }
 
   *result = reinterpret_cast<napi_value>(constructor);
@@ -364,69 +403,82 @@ napi_status napi_set_property(napi_env e, napi_value o,
   return napi_ok;
 }
 
-napi_status napi_define_property(napi_env e, napi_value o,
-    napi_property_descriptor* p) {
-  napi_value descriptor;
-  CHECK_NAPI(napi_create_object(e, &descriptor));
-
-  napi_propertyname configurableProperty;
-  CHECK_NAPI(napi_property_name(e, "configurable", &configurableProperty));
-  napi_value configurable;
-  CHECK_NAPI(napi_create_boolean(e, !(p->attributes & napi_dont_delete), &configurable));
-  CHECK_NAPI(napi_set_property(e, descriptor, configurableProperty, configurable));
+napi_status napi_define_properties(napi_env e,
+                                   napi_value o,
+                                   int property_count,
+                                   const napi_property_descriptor* properties) {
+  JsPropertyIdRef configurableProperty;
+  CHECK_JSRT(JsCreatePropertyIdUtf8("configurable", 12, &configurableProperty));
 
-  napi_propertyname enumerableProperty;
-  CHECK_NAPI(napi_property_name(e, "enumerable", &enumerableProperty));
-  napi_value enumerable;
-  CHECK_NAPI(napi_create_boolean(e, !(p->attributes & napi_dont_enum), &enumerable));
-  CHECK_NAPI(napi_set_property(e, descriptor, enumerableProperty, enumerable));
+  JsPropertyIdRef enumerableProperty;
+  CHECK_JSRT(JsCreatePropertyIdUtf8("enumerable", 10, &enumerableProperty));
 
-  if (p->method) {
-    napi_propertyname valueProperty;
-    CHECK_NAPI(napi_property_name(e, "value", &valueProperty));
-    napi_value method;
-    CHECK_NAPI(napi_create_function(e, p->method, p->data, &method));
-    CHECK_NAPI(napi_set_property(e, descriptor, valueProperty, method));
-  }
-  else if (p->getter || p->setter) {
-    if (p->getter) {
-      napi_propertyname getProperty;
-      CHECK_NAPI(napi_property_name(e, "get", &getProperty));
-      napi_value getter;
-      CHECK_NAPI(napi_create_function(e, p->getter, p->data, &getter));
-      CHECK_NAPI(napi_set_property(e, descriptor, getProperty, getter));
+  for (int i = 0; i < property_count; i++) {
+    const napi_property_descriptor* p = properties + i;
+
+    JsValueRef descriptor;
+    CHECK_JSRT(JsCreateObject(&descriptor));
+
+    JsValueRef configurable;
+    CHECK_JSRT(JsBoolToBoolean(!(p->attributes & napi_dont_delete), &configurable));
+    CHECK_JSRT(JsSetProperty(descriptor, configurableProperty, configurable, true));
+
+    JsValueRef enumerable;
+    CHECK_JSRT(JsBoolToBoolean(!(p->attributes & napi_dont_enum), &enumerable));
+    CHECK_JSRT(JsSetProperty(descriptor, enumerableProperty, enumerable, true));
+
+    if (p->method) {
+      JsPropertyIdRef valueProperty;
+      CHECK_JSRT(JsCreatePropertyIdUtf8("value", 5, &valueProperty));
+      JsValueRef method;
+      CHECK_NAPI(napi_create_function(e, p->method, p->data,
+        reinterpret_cast<napi_value*>(&method)));
+      CHECK_JSRT(JsSetProperty(descriptor, valueProperty, method, true));
     }
-
-    if (p->setter) {
-      napi_propertyname setProperty;
-      CHECK_NAPI(napi_property_name(e, "set", &setProperty));
-      napi_value setter;
-      CHECK_NAPI(napi_create_function(e, p->setter, p->data, &setter));
-      CHECK_NAPI(napi_set_property(e, descriptor, setProperty, setter));
+    else if (p->getter || p->setter) {
+      if (p->getter) {
+        JsPropertyIdRef getProperty;
+        CHECK_JSRT(JsCreatePropertyIdUtf8("get", 3, &getProperty));
+        JsValueRef getter;
+        CHECK_NAPI(napi_create_function(e, p->getter, p->data,
+          reinterpret_cast<napi_value*>(&getter)));
+        CHECK_JSRT(JsSetProperty(descriptor, getProperty, getter, true));
+      }
+
+      if (p->setter) {
+        JsPropertyIdRef setProperty;
+        CHECK_JSRT(JsCreatePropertyIdUtf8("set", 5, &setProperty));
+        JsValueRef setter;
+        CHECK_NAPI(napi_create_function(e, p->setter, p->data,
+          reinterpret_cast<napi_value*>(&setter)));
+        CHECK_JSRT(JsSetProperty(descriptor, setProperty, setter, true));
+      }
+    }
+    else {
+      RETURN_STATUS_IF_FALSE(p->value != nullptr, napi_invalid_arg);
+
+      JsPropertyIdRef writableProperty;
+      CHECK_JSRT(JsCreatePropertyIdUtf8("writable", 8, &writableProperty));
+      JsValueRef writable;
+      CHECK_JSRT(JsBoolToBoolean(!(p->attributes & napi_read_only), &writable));
+      CHECK_JSRT(JsSetProperty(descriptor, writableProperty, writable, true));
+
+      JsPropertyIdRef valueProperty;
+      CHECK_JSRT(JsCreatePropertyIdUtf8("value", 5, &valueProperty));
+      CHECK_JSRT(JsSetProperty(descriptor, valueProperty,
+        reinterpret_cast<JsValueRef>(p->value), true));
     }
-  }
-  else {
-    RETURN_STATUS_IF_FALSE(p->value != nullptr, napi_invalid_arg);
-
-    napi_propertyname writableProperty;
-    CHECK_NAPI(napi_property_name(e, "writable", &writableProperty));
-    napi_value writable;
-    CHECK_NAPI(napi_create_boolean(e, !(p->attributes & napi_read_only), &writable));
-    CHECK_NAPI(napi_set_property(e, descriptor, writableProperty, writable));
 
-    napi_propertyname valueProperty;
-    CHECK_NAPI(napi_property_name(e, "value", &valueProperty));
-    CHECK_NAPI(napi_set_property(e, descriptor, valueProperty, p->value));
+    JsPropertyIdRef nameProperty;
+    CHECK_JSRT(JsCreatePropertyIdUtf8(p->utf8name, strlen(p->utf8name), &nameProperty));
+    bool result;
+    CHECK_JSRT(JsDefineProperty(
+      reinterpret_cast<JsValueRef>(o),
+      reinterpret_cast<JsPropertyIdRef>(nameProperty),
+      reinterpret_cast<JsValueRef>(descriptor),
+      &result));
   }
 
-  napi_propertyname nameProperty;
-  CHECK_NAPI(napi_property_name(e, p->utf8name, &nameProperty));
-  bool result;
-  CHECK_JSRT(JsDefineProperty(
-    reinterpret_cast<JsValueRef>(o),
-    reinterpret_cast<JsPropertyIdRef>(nameProperty),
-    reinterpret_cast<JsValueRef>(descriptor),
-    &result));
   return napi_ok;
 }
 
@@ -679,7 +731,7 @@ napi_status napi_get_cb_args(napi_env e, napi_callback_info cbinfo,
   const CallbackInfo *info = reinterpret_cast<CallbackInfo*>(cbinfo);
 
   int i = 0;
-  int min = bufferlength < (info->argc) - 1 ? bufferlength : (info->argc) - 1;
+  int min = std::min(bufferlength, info->argc - 1);
 
   for (; i < min; i++) {
     buffer[i] = info->argv[i + 1];

src/node_jsvmapi.h

@@ -186,8 +186,10 @@ NODE_EXTERN napi_status napi_has_element(napi_env e, napi_value object,
                                   uint32_t i, bool* result);
 NODE_EXTERN napi_status napi_get_element(napi_env e, napi_value object,
                                   uint32_t i, napi_value* result);
-NODE_EXTERN napi_status napi_define_property(napi_env e, napi_value object,
-                                  napi_property_descriptor* property);
+NODE_EXTERN napi_status napi_define_properties(napi_env e,
+                                               napi_value object,
+                                               int property_count,
+                                               const napi_property_descriptor* properties);
 
 // Methods to work with Arrays
 NODE_EXTERN napi_status napi_is_array(napi_env e, napi_value v, bool* result);
@@ -263,13 +265,13 @@ NODE_EXTERN napi_status napi_wrap(napi_env e, napi_value jsObject, void* nativeO
                                   napi_weakref* handle);
 NODE_EXTERN napi_status napi_unwrap(napi_env e, napi_value jsObject, void** result);
 
-NODE_EXTERN napi_status napi_create_constructor(napi_env e,
-                                                const char* utf8name,
-                                                napi_callback cb,
-                                                void* data,
-                                                int property_count,
-                                                napi_property_descriptor* properties,
-                                                napi_value* result);
+NODE_EXTERN napi_status napi_define_class(napi_env e,
+                                          const char* utf8name,
+                                          napi_callback constructor,
+                                          void* data,
+                                          int property_count,
+                                          const napi_property_descriptor* properties,
+                                          napi_value* result);
 
 // Methods to control object lifespan
 NODE_EXTERN napi_status napi_create_persistent(napi_env e, napi_value v,

src/node_jsvmapi_types.h

@@ -1,6 +1,7 @@
 #ifndef SRC_NODE_JSVMAPI_TYPES_H_
 #define SRC_NODE_JSVMAPI_TYPES_H_
 
+#include <stddef.h>
 #include <stdint.h>
 
 // JSVM API types are all opaque pointers for ABI stability
@@ -21,7 +22,11 @@ enum napi_property_attributes {
   napi_default = 0,
   napi_read_only = 1 << 0,
   napi_dont_enum = 1 << 1,
-  napi_dont_delete = 1 << 2
+  napi_dont_delete = 1 << 2,
+
+  // Used with napi_define_class to distinguish static properties
+  // from instance properties. Ignored by napi_define_properties.
+  napi_static_property = 1 << 10,
 };
 
 struct napi_property_descriptor {

test/addons-abi/1_hello_world/binding.cc

@@ -12,7 +12,7 @@ void Method(napi_env env, napi_callback_info info) {
 void Init(napi_env env, napi_value exports, napi_value module) {
   napi_status status;
   napi_property_descriptor desc = { "hello", Method };
-  status = napi_define_property(env, exports, &desc);
+  status = napi_define_properties(env, exports, 1, &desc);
   if (status != napi_ok) return;
 }
 

test/addons-abi/2_function_arguments/binding.cc

@@ -49,7 +49,7 @@ void Add(napi_env env, napi_callback_info info) {
 void Init(napi_env env, napi_value exports, napi_value module) {
   napi_status status;
   napi_property_descriptor addDescriptor = { "add", Add };
-  status = napi_define_property(env, exports, &addDescriptor);
+  status = napi_define_properties(env, exports, 1, &addDescriptor);
   if (status != napi_ok) return;
 }
 

test/addons-abi/3_callbacks/binding.cc

@@ -41,10 +41,9 @@ void Init(napi_env env, napi_value exports, napi_value module) {
     { "RunCallback", RunCallback },
     { "RunCallbackWithRecv", RunCallbackWithRecv }
   };
-  for (int index = 0; index < 2; index++) {
-    status = napi_define_property(env, exports, &desc[index]);
-    if (status != napi_ok) return;
-  }
+  status = napi_define_properties(
+    env, exports, sizeof(desc) / sizeof(napi_property_descriptor), desc);
+  if (status != napi_ok) return;
 }
 
 NODE_MODULE_ABI(addon, Init)

test/addons-abi/4_object_factory/binding.cc

@@ -25,7 +25,7 @@ void CreateObject(napi_env env, const napi_callback_info info) {
 void Init(napi_env env, napi_value exports, napi_value module) {
   napi_status status;
   napi_property_descriptor desc = { "exports", CreateObject };
-  status = napi_define_property(env, module, &desc);
+  status = napi_define_properties(env, module, 1, &desc);
   if (status != napi_ok) return;
 }
 

test/addons-abi/5_function_factory/binding.cc

@@ -33,7 +33,7 @@ void CreateFunction(napi_env env, napi_callback_info info) {
 void Init(napi_env env, napi_value exports, napi_value module) {
   napi_status status;
   napi_property_descriptor desc = { "exports", CreateFunction };
-  status = napi_define_property(env, module, &desc);
+  status = napi_define_properties(env, module, 1, &desc);
   if (status != napi_ok) return;
 }
 

test/addons-abi/6_object_wrap/myobject.cc

@@ -21,7 +21,7 @@ void MyObject::Init(napi_env env, napi_value exports) {
   };
 
   napi_value cons;
-  status = napi_create_constructor(env, "MyObject", New, nullptr, 3, properties, &cons);
+  status = napi_define_class(env, "MyObject", New, nullptr, 3, properties, &cons);
   if (status != napi_ok) return;
 
   status = napi_create_persistent(env, cons, &constructor);

test/addons-abi/7_factory_wrap/binding.cc

@@ -22,7 +22,7 @@ void Init(napi_env env, napi_value exports, napi_value module) {
   if (status != napi_ok) return;
 
   napi_property_descriptor desc = { "exports", CreateObject };
-  status = napi_define_property(env, module, &desc);
+  status = napi_define_properties(env, module, 1, &desc);
   if (status != napi_ok) return;
 }