From ff42f9d2755ad6e2749395f223f029c251eb609b Mon Sep 17 00:00:00 2001
From: Jason Ginchereau <jasongin@users.noreply.github.com>
Date: Thu, 18 May 2017 16:08:03 -0700
Subject: [PATCH] n-api: Fix test-addons-napi failures (#249)

- Reimplement napi_make_callback to call node::MakeCallback
   using V8 (chakrashim) types. We always knew the direct call to
   JsCallFunction() was incorrect there.
 - Re-enable the two tests that were failing due to napi_make_callback

Fixes: https://github.com/nodejs/node-chakracore/issues/246
---
 src/node_api_jsrt.cc                          | 28 +++++++++----------
 test/addons-napi/test_async/test.js           |  6 ----
 .../test_make_callback_recurse/test.js        |  5 ----
 3 files changed, 14 insertions(+), 25 deletions(-)

diff --git a/src/node_api_jsrt.cc b/src/node_api_jsrt.cc
index bf94299c0be..ef4275374bf 100644
--- a/src/node_api_jsrt.cc
+++ b/src/node_api_jsrt.cc
@@ -1623,22 +1623,22 @@ napi_status napi_make_callback(napi_env env,
                                size_t argc,
                                const napi_value* argv,
                                napi_value* result) {
-  JsValueRef object = reinterpret_cast<JsValueRef>(recv);
-  JsValueRef function = reinterpret_cast<JsValueRef>(func);
-  std::vector<JsValueRef> args(argc + 1);
-  args[0] = object;
-  for (size_t i = 0; i < argc; i++) {
-    args[i + 1] = reinterpret_cast<JsValueRef>(argv[i]);
-  }
-  JsValueRef returnValue;
-  CHECK_JSRT(JsCallFunction(
-    function,
-    args.data(),
-    static_cast<uint16_t>(argc + 1),
-    &returnValue));
+  v8::Isolate* isolate = v8impl::V8IsolateFromJsEnv(env);
+  v8::Local<v8::Object> v8recv =
+    v8impl::V8LocalValueFromJsValue(recv).As<v8::Object>();
+  v8::Local<v8::Function> v8func =
+    v8impl::V8LocalValueFromJsValue(func).As<v8::Function>();
+  v8::Local<v8::Value>* v8argv =
+    reinterpret_cast<v8::Local<v8::Value>*>(const_cast<napi_value*>(argv));
+
+  // TODO(jasongin): Expose JSRT or N-API version of node::MakeCallback?
+  v8::Local<v8::Value> v8result =
+    node::MakeCallback(isolate, v8recv, v8func, argc, v8argv);
+
   if (result != nullptr) {
-    *result = reinterpret_cast<napi_value>(returnValue);
+    *result = v8impl::JsValueFromV8LocalValue(v8result);
   }
+
   return napi_ok;
 }
 
diff --git a/test/addons-napi/test_async/test.js b/test/addons-napi/test_async/test.js
index f6287b9d6b5..bd93dd814f3 100644
--- a/test/addons-napi/test_async/test.js
+++ b/test/addons-napi/test_async/test.js
@@ -3,12 +3,6 @@ const common = require('../../common');
 const assert = require('assert');
 const child_process = require('child_process');
 
-if (common.isChakraEngine) {
-  common.skip('Skipped for node-chakracore till #246 is fixed.');
-  return;
-}
-
-
 const test_async = require(`./build/${common.buildType}/test_async`);
 
 const testException = 'test_async_cb_exception';
diff --git a/test/addons-napi/test_make_callback_recurse/test.js b/test/addons-napi/test_make_callback_recurse/test.js
index 2fdb0733aad..895769bc330 100644
--- a/test/addons-napi/test_make_callback_recurse/test.js
+++ b/test/addons-napi/test_make_callback_recurse/test.js
@@ -6,11 +6,6 @@ const domain = require('domain');
 const binding = require(`./build/${common.buildType}/binding`);
 const makeCallback = binding.makeCallback;
 
-if (common.isChakraEngine) {
-  common.skip('Skipped for node-chakracore till #246 is fixed.');
-  return;
-}
-
 // Make sure this is run in the future.
 const mustCallCheckDomains = common.mustCall(checkDomains);