AsyncWorker: add GetResult() method

Adds an overridable `GetResult()` method, providing arguments
to the callback invoked in `OnOK()`.

Refs: #231 (comment)
PR-URL: #512
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: NickNaso <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>

doc/async_worker.md

@@ -107,11 +107,21 @@ virtual void Napi::AsyncWorker::Execute() = 0;
 
 This method is invoked when the computation in the `Execute` method ends.
 The default implementation runs the Callback optionally provided when the AsyncWorker class
-was created.
+was created. The callback will by default receive no arguments. To provide arguments,
+override the `GetResult()` method.
 
 ```cpp
 virtual void Napi::AsyncWorker::OnOK();
 ```
+### GetResult
+
+This method returns the arguments passed to the Callback invoked by the default
+`OnOK()` implementation. The default implementation returns an empty vector,
+providing no arguments to the Callback.
+
+```cpp
+virtual std::vector<napi_value> Napi::AsyncWorker::GetResult(Napi::Env env);
+```
 
 ### OnError
 

napi-inl.h

@@ -3686,7 +3686,7 @@ inline void AsyncWorker::SuppressDestruct() {
 
 inline void AsyncWorker::OnOK() {
   if (!_callback.IsEmpty()) {
-    _callback.Call(_receiver.Value(), std::initializer_list<napi_value>{});
+    _callback.Call(_receiver.Value(), GetResult(_callback.Env()));
   }
 }
 
@@ -3700,7 +3700,14 @@ inline void AsyncWorker::SetError(const std::string& error) {
   _error = error;
 }
 
-inline void AsyncWorker::OnExecute(napi_env /*env*/, void* this_pointer) {
+inline std::vector<napi_value> AsyncWorker::GetResult(Napi::Env /*env*/) {
+  return {};
+}
+// The OnExecute method receives an napi_env argument. However, do NOT
+// use it within this method, as it does not run on the main thread and must
+// not run any method that would cause JavaScript to run. In practice, this
+// means that almost any use of napi_env will be incorrect.
+inline void AsyncWorker::OnExecute(napi_env /*DO_NOT_USE*/, void* this_pointer) {
   AsyncWorker* self = static_cast<AsyncWorker*>(this_pointer);
 #ifdef NAPI_CPP_EXCEPTIONS
   try {

napi.h

@@ -1812,6 +1812,7 @@ namespace Napi {
     virtual void OnOK();
     virtual void OnError(const Error& e);
     virtual void Destroy();
+    virtual std::vector<napi_value> GetResult(Napi::Env env);
 
     void SetError(const std::string& error);
 

test/asyncworker.cc

@@ -29,6 +29,38 @@ class TestWorker : public AsyncWorker {
   bool _succeed;
 };
 
+class TestWorkerWithResult : public AsyncWorker {
+public:
+  static void DoWork(const CallbackInfo& info) {
+    bool succeed = info[0].As<Boolean>();
+    Object resource = info[1].As<Object>();
+    Function cb = info[2].As<Function>();
+    Value data = info[3];
+
+    TestWorkerWithResult* worker = new TestWorkerWithResult(cb, "TestResource", resource);
+    worker->Receiver().Set("data", data);
+    worker->_succeed = succeed;
+    worker->Queue();
+  }
+
+protected:
+  void Execute() override {
+    if (!_succeed) {
+      SetError("test error");
+    }
+  }
+
+  std::vector<napi_value> GetResult(Napi::Env env) override {
+    return {Boolean::New(env, _succeed),
+            String::New(env, _succeed ? "ok" : "error")};
+  }
+
+private:
+  TestWorkerWithResult(Function cb, const char* resource_name, const Object& resource)
+      : AsyncWorker(cb, resource_name, resource) {}
+  bool _succeed;
+};
+
 class TestWorkerNoCallback : public AsyncWorker {
 public:
   static Value DoWork(const CallbackInfo& info) {
@@ -65,5 +97,6 @@ Object InitAsyncWorker(Env env) {
   Object exports = Object::New(env);
   exports["doWork"] = Function::New(env, TestWorker::DoWork);
   exports["doWorkNoCallback"] = Function::New(env, TestWorkerNoCallback::DoWork);
+  exports["doWorkWithResult"] = Function::New(env, TestWorkerWithResult::DoWork);
   return exports;
 }

test/asyncworker.js

@@ -66,6 +66,14 @@ function test(binding) {
       assert.strictEqual(typeof this, 'object');
       assert.strictEqual(this.data, 'test data');
     }, 'test data');
+
+    binding.asyncworker.doWorkWithResult(true, {}, function (succeed, succeedString) {
+      assert(arguments.length == 2);
+      assert(succeed);
+      assert(succeedString == "ok");
+      assert.strictEqual(typeof this, 'object');
+      assert.strictEqual(this.data, 'test data');
+    }, 'test data');
     return;
   }
 
@@ -91,6 +99,30 @@ function test(binding) {
     }).catch(common.mustNotCall());
   }
 
+  {
+    const hooks = installAsyncHooksForTest();
+    const triggerAsyncId = async_hooks.executionAsyncId();
+    binding.asyncworker.doWorkWithResult(true, { foo: 'foo' }, function (succeed, succeedString) {
+      assert(arguments.length == 2);
+      assert(succeed);
+      assert(succeedString == "ok");
+      assert.strictEqual(typeof this, 'object');
+      assert.strictEqual(this.data, 'test data');
+    }, 'test data');
+
+    hooks.then(actual => {
+      assert.deepStrictEqual(actual, [
+        { eventName: 'init',
+          type: 'TestResource',
+          triggerAsyncId: triggerAsyncId,
+          resource: { foo: 'foo' } },
+        { eventName: 'before' },
+        { eventName: 'after' },
+        { eventName: 'destroy' }
+      ]);
+    }).catch(common.mustNotCall());
+  }
+
   {
     const hooks = installAsyncHooksForTest();
     const triggerAsyncId = async_hooks.executionAsyncId();