napi: Property & class definition improvements (nodejs#97)

- 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
boingoing · Feb 10, 2017 · 784e07c · 784e07c
1 parent 758fde2
commit 784e07c
Show file tree

Hide file tree

Showing 23 changed files with 149 additions and 128 deletions.
diff --git a/src/node_jsvmapi.cc b/src/node_jsvmapi.cc
@@ -596,24 +596,23 @@ napi_status napi_create_function(
   return GET_RETURN_STATUS();
 }
 
-napi_status napi_create_constructor(
+napi_status napi_define_class(
     napi_env e,
     const char* utf8name,
-    napi_callback cb,
+    napi_callback constructor,
     void* data,
     int property_count,
-    napi_property_descriptor* properties,
+    const napi_property_descriptor* properties,
     napi_value* result) {
   NAPI_PREAMBLE(e);
   CHECK_ARG(result);
 
   v8::Isolate *isolate = v8impl::V8IsolateFromJsEnv(e);
   v8::Local<v8::Context> context = isolate->GetCurrentContext();
-  v8::Local<v8::Object> retval;
 
   v8::EscapableHandleScope scope(isolate);
   v8::Local<v8::Object> cbdata =
-    v8impl::CreateFunctionCallbackData(e, cb, data);
+    v8impl::CreateFunctionCallbackData(e, constructor, data);
 
   RETURN_STATUS_IF_FALSE(!cbdata.IsEmpty(), napi_generic_failure);
 
@@ -627,8 +626,16 @@ napi_status napi_create_constructor(
   CHECK_NEW_FROM_UTF8(isolate, namestring, utf8name);
   tpl->SetClassName(namestring);
 
+  int staticPropertyCount = 0;
   for (int i = 0; i < property_count; i++) {
-    napi_property_descriptor* p = properties + i;
+    const napi_property_descriptor* p = properties + i;
+
+    if ((p->attributes & napi_static_property) != 0) {
+      // Static properties are handled separately below.
+      staticPropertyCount++;
+      continue;
+    }
+
     v8::Local<v8::String> propertyname;
     CHECK_NEW_FROM_UTF8(isolate, propertyname, p->utf8name);
 
@@ -671,8 +678,23 @@ napi_status napi_create_constructor(
     }
   }
 
-  retval = scope.Escape(tpl->GetFunction());
-  *result = v8impl::JsValueFromV8LocalValue(retval);
+  *result = v8impl::JsValueFromV8LocalValue(scope.Escape(tpl->GetFunction()));
+
+  if (staticPropertyCount > 0) {
+    std::vector<napi_property_descriptor> staticDescriptors;
+    staticDescriptors.reserve(staticPropertyCount);
+
+    for (int i = 0; i < property_count; i++) {
+      const napi_property_descriptor* p = properties + i;
+      if ((p->attributes & napi_static_property) != 0) {
+        staticDescriptors.push_back(*p);
+      }
+    }
+
+    napi_status status = napi_define_properties(
+      e, *result, staticDescriptors.size(), staticDescriptors.data());
+    if (status != napi_ok) return status;
+  }
 
   return GET_RETURN_STATUS();
 }
@@ -851,77 +873,80 @@ napi_status napi_get_element(napi_env e,
   return GET_RETURN_STATUS();
 }
 
-napi_status napi_define_property(napi_env e, napi_value o,
-                                 napi_property_descriptor* p) {
+napi_status napi_define_properties(napi_env e, napi_value o,
+    int property_count, const napi_property_descriptor* properties) {
   NAPI_PREAMBLE(e);
 
   v8::Isolate *isolate = v8impl::V8IsolateFromJsEnv(e);
   v8::Local<v8::Context> context = isolate->GetCurrentContext();
-  v8::Local<v8::Object> obj;
-  CHECK_TO_OBJECT(context, obj, o);
+  v8::Local<v8::Object> obj = v8impl::V8LocalValueFromJsValue(o).As<v8::Object>();
 
-  v8::Local<v8::Name> name;
-  CHECK_NEW_FROM_UTF8(isolate, name, p->utf8name);
+  for (int i = 0; i < property_count; i++) {
+    const napi_property_descriptor* p = &properties[i];
 
-  v8::PropertyAttribute attributes =
-    static_cast<v8::PropertyAttribute>(p->attributes);
+    v8::Local<v8::Name> name;
+    CHECK_NEW_FROM_UTF8(isolate, name, p->utf8name);
 
-  if (p->method) {
-    v8::Local<v8::Object> cbdata = v8impl::CreateFunctionCallbackData(
-      e, p->method, p->data);
+    v8::PropertyAttribute attributes =
+      static_cast<v8::PropertyAttribute>(p->attributes & ~napi_static_property);
 
-    RETURN_STATUS_IF_FALSE(!cbdata.IsEmpty(), napi_generic_failure);
+    if (p->method) {
+      v8::Local<v8::Object> cbdata = v8impl::CreateFunctionCallbackData(
+        e, p->method, p->data);
 
-    v8::Local<v8::FunctionTemplate> t = v8::FunctionTemplate::New(
-      isolate, v8impl::FunctionCallbackWrapper::Invoke, cbdata);
+      RETURN_STATUS_IF_FALSE(!cbdata.IsEmpty(), napi_generic_failure);
 
-    auto define_maybe = obj->DefineOwnProperty(
-      context,
-      name,
-      t->GetFunction(),
-      attributes);
+      v8::Local<v8::FunctionTemplate> t = v8::FunctionTemplate::New(
+        isolate, v8impl::FunctionCallbackWrapper::Invoke, cbdata);
 
-    // 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);
+      auto define_maybe = obj->DefineOwnProperty(
+        context,
+        name,
+        t->GetFunction(),
+        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);
+      }
     }
-  }
-  else if (p->getter || p->setter) {
-    v8::Local<v8::Object> cbdata = v8impl::CreateAccessorCallbackData(
-      e, p->getter, p->setter, p->data);
-
-    auto set_maybe = obj->SetAccessor(
-      context,
-      name,
-      v8impl::GetterCallbackWrapper::Invoke,
-      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);
+    else if (p->getter || p->setter) {
+      v8::Local<v8::Object> cbdata = v8impl::CreateAccessorCallbackData(
+        e, p->getter, p->setter, p->data);
+
+      auto set_maybe = obj->SetAccessor(
+        context,
+        name,
+        v8impl::GetterCallbackWrapper::Invoke,
+        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);
+      }
     }
-  }
-  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);
+    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);
+      }
     }
   }
 

diff --git a/src/node_jsvmapi.h b/src/node_jsvmapi.h
@@ -185,8 +185,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);
@@ -262,13 +264,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,

diff --git a/src/node_jsvmapi_types.h b/src/node_jsvmapi_types.h
@@ -21,7 +21,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 {

diff --git a/test/addons-abi/1_hello_world/binding.cc b/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;
 }
 

diff --git a/test/addons-abi/2_function_arguments/binding.cc b/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;
 }
 

diff --git a/test/addons-abi/3_callbacks/binding.cc b/test/addons-abi/3_callbacks/binding.cc
@@ -24,7 +24,7 @@ void RunCallback(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", RunCallback };
-  status = napi_define_property(env, module, &desc);
+  status = napi_define_properties(env, module, 1, &desc);
   if (status != napi_ok) return;
 }
 

diff --git a/test/addons-abi/4_object_factory/binding.cc b/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;
 }
 

diff --git a/test/addons-abi/5_function_factory/binding.cc b/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;
 }
 

diff --git a/test/addons-abi/6_object_wrap/myobject.cc b/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);

diff --git a/test/addons-abi/7_factory_wrap/binding.cc b/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;
 }
 

diff --git a/test/addons-abi/7_factory_wrap/myobject.cc b/test/addons-abi/7_factory_wrap/myobject.cc
@@ -16,7 +16,7 @@ napi_status MyObject::Init(napi_env env) {
   };
 
   napi_value cons;
-  status = napi_create_constructor(env, "MyObject", New, nullptr, 1, properties, &cons);
+  status = napi_define_class(env, "MyObject", New, nullptr, 1, properties, &cons);
   if (status != napi_ok) return status;
 
   status = napi_create_persistent(env, cons, &constructor);

diff --git a/test/addons-abi/8_passing_wrapped/binding.cc b/test/addons-abi/8_passing_wrapped/binding.cc
@@ -42,12 +42,11 @@ void Init(napi_env env, napi_value exports, napi_value module) {
 
   MyObject::Init(env);
 
-  napi_property_descriptor desc = { "createObject", CreateObject };
-  status = napi_define_property(env, exports, &desc);
-  if (status != napi_ok) return;
-
-  napi_property_descriptor desc2 = { "add", Add };
-  status = napi_define_property(env, exports, &desc2);
+  napi_property_descriptor desc[] = {
+    { "createObject", CreateObject },
+    { "add", Add },
+  };
+  status = napi_define_properties(env, exports, sizeof(desc) / sizeof(*desc), desc);
   if (status != napi_ok) return;
 }
 

diff --git a/test/addons-abi/8_passing_wrapped/myobject.cc b/test/addons-abi/8_passing_wrapped/myobject.cc
@@ -14,7 +14,7 @@ napi_status MyObject::Init(napi_env env) {
   napi_status status;
 
   napi_value cons;
-  status = napi_create_constructor(env, "MyObject", New, nullptr, 0, nullptr, &cons);
+  status = napi_define_class(env, "MyObject", New, nullptr, 0, nullptr, &cons);
   if (status != napi_ok) return status;
 
   status = napi_create_persistent(env, cons, &constructor);

diff --git a/test/addons-abi/test_array/test_array.cc b/test/addons-abi/test_array/test_array.cc
@@ -128,10 +128,9 @@ void Init(napi_env env, napi_value exports, napi_value module) {
     { "New", New },
   };
 
-  for (int i = 0; i < sizeof(descriptors) / sizeof(*descriptors); i++) {
-    status = napi_define_property(env, exports, &descriptors[i]);
-    if (status != napi_ok) return;
-  }
+  status = napi_define_properties(
+    env, exports, sizeof(descriptors) / sizeof(*descriptors), descriptors);
+  if (status != napi_ok) return;
 }
 
 NODE_MODULE_ABI(addon, Init)
diff --git a/test/addons-abi/test_constructor/test_constructor.cc b/test/addons-abi/test_constructor/test_constructor.cc
@@ -91,7 +91,7 @@ void Init(napi_env env, napi_value exports, napi_value module) {
   };
 
   napi_value cons;
-  status = napi_create_constructor(env, "MyObject", New,
+  status = napi_define_class(env, "MyObject", New,
     nullptr, sizeof(properties)/sizeof(*properties), properties, &cons);
   if (status != napi_ok) return;
 

diff --git a/test/addons-abi/test_exception/test_exception.cc b/test/addons-abi/test_exception/test_exception.cc
@@ -65,10 +65,9 @@ NAPI_MODULE_INIT(Init) {
     { "wasPending", wasPending },
   };
 
-  for (int i = 0; i < sizeof(descriptors) / sizeof(*descriptors); i++) {
-    status = napi_define_property(env, exports, &descriptors[i]);
-    if (status != napi_ok) return;
-  }
+  status = napi_define_properties(
+    env, exports, sizeof(descriptors) / sizeof(*descriptors), descriptors);
+  if (status != napi_ok) return;
 }
 
 NODE_MODULE_ABI(addon, Init)