From 325f92b64e8bffff3cf8cbba752453c863786725 Mon Sep 17 00:00:00 2001
From: Michael Price <mikepricedev@gmail.com>
Date: Thu, 18 Jul 2019 18:16:02 -0600
Subject: [PATCH 1/6] Adds Napi::ObjectWrap::OverrideFinalizeCallback and
 documentation.  Allows for user defined napi_finalize finalzier callbacks;
 addressing advanced cleanup senarios with opt-in complexity closer to the
 N-API lib.

---
 doc/object_wrap.md | 20 ++++++++++++++++++++
 napi-inl.h         | 15 +++++++++++++--
 napi.h             |  4 +++-
 3 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/doc/object_wrap.md b/doc/object_wrap.md
index 31be140f0..630afd38b 100644
--- a/doc/object_wrap.md
+++ b/doc/object_wrap.md
@@ -190,6 +190,26 @@ property of the `Napi::CallbackInfo`.
 
 Returns a `Napi::Function` representing the constructor function for the class.
 
+### OverrideFinalizeCallback
+
+Overrides default `Napi::ObjectWrap::FinalizeCallback` with a user defined finalizer.
+
+```cpp
+static void Napi::ObjectWrap::OverrideFinalizeCallback(T* instance,
+                  napi_finalize finalizeCallback);
+```
+
+- `[in] instance`: `this` pointer from the native instance.
+- `[in] finalizeCallback`: function that implements [napi_finalize](https://nodejs.org/api/n-api.html#n_api_napi_finalize "N-API documentation for napi_finalize.").
+
+`Napi::ObjectWrap::OverrideFinalizeCallback` is protected and
+intended to be called from a native instance method; for example, in the native
+constructor.
+
+NOTE: The default `Napi::ObjectWrap::FinalizeCallback` frees
+the native instance.  A user defined finalzier is, therefore, 
+responsible for freeing the native instance.
+
 ### StaticMethod
 
 Creates property descriptor that represents a static method of a JavaScript class.
diff --git a/napi-inl.h b/napi-inl.h
index 0db3c9830..d6db0a642 100644
--- a/napi-inl.h
+++ b/napi-inl.h
@@ -3259,6 +3259,13 @@ inline ClassPropertyDescriptor<T> ObjectWrap<T>::InstanceValue(
   return desc;
 }
 
+template <typename T>
+inline void ObjectWrap<T>::OverrideFinalizeCallback(T* instance,
+    napi_finalize finalizeCallback) {
+  ObjectWrap<T>* base = instance;
+  base->_finalizeCallbackOverride = finalizeCallback;
+}
+
 template <typename T>
 inline napi_value ObjectWrap<T>::ConstructorCallbackWrapper(
     napi_env env,
@@ -3400,9 +3407,13 @@ inline napi_value ObjectWrap<T>::InstanceSetterCallbackWrapper(
 }
 
 template <typename T>
-inline void ObjectWrap<T>::FinalizeCallback(napi_env /*env*/, void* data, void* /*hint*/) {
+inline void ObjectWrap<T>::FinalizeCallback(napi_env env, void* data, void* hint) {
   T* instance = reinterpret_cast<T*>(data);
-  delete instance;
+  if(instance->_finalizeCallbackOverride == nullptr){
+    delete instance;
+  } else {
+    (*(instance->_finalizeCallbackOverride))(env, data, hint);
+  }
 }
 
 ////////////////////////////////////////////////////////////////////////////////
diff --git a/napi.h b/napi.h
index c1946413b..393f693ea 100644
--- a/napi.h
+++ b/napi.h
@@ -1657,8 +1657,10 @@ namespace Napi {
     static PropertyDescriptor InstanceValue(Symbol name,
                                             Napi::Value value,
                                             napi_property_attributes attributes = napi_default);
-
+  protected:
+    static void OverrideFinalizeCallback(T* instance, napi_finalize finalizeCallback);
   private:
+    napi_finalize _finalizeCallbackOverride = nullptr;
     static napi_value ConstructorCallbackWrapper(napi_env env, napi_callback_info info);
     static napi_value StaticVoidMethodCallbackWrapper(napi_env env, napi_callback_info info);
     static napi_value StaticMethodCallbackWrapper(napi_env env, napi_callback_info info);

From d4f99f74261cde8d2014f2fd3a5c261632d11ec7 Mon Sep 17 00:00:00 2001
From: Michael Price <mikepricedev@gmail.com>
Date: Tue, 23 Jul 2019 21:47:09 -0600
Subject: [PATCH 2/6] Updates PR #515 to implement virutal ObjectWrap::Finalize
 outlined
 https://github.com/nodejs/node-addon-api/pull/515#issuecomment-513977222

Co-authored-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
---
 doc/object_wrap.md | 19 +++++--------------
 napi-inl.h         | 15 ++++-----------
 napi.h             |  4 +---
 3 files changed, 10 insertions(+), 28 deletions(-)

diff --git a/doc/object_wrap.md b/doc/object_wrap.md
index 630afd38b..13c38695a 100644
--- a/doc/object_wrap.md
+++ b/doc/object_wrap.md
@@ -190,25 +190,16 @@ property of the `Napi::CallbackInfo`.
 
 Returns a `Napi::Function` representing the constructor function for the class.
 
-### OverrideFinalizeCallback
+### Finalize
 
-Overrides default `Napi::ObjectWrap::FinalizeCallback` with a user defined finalizer.
+Hooks into `Napi::ObjectWrap::FinalizeCallback` giving access to `Napi::Env`
+before the native instance is freed.
 
 ```cpp
-static void Napi::ObjectWrap::OverrideFinalizeCallback(T* instance,
-                  napi_finalize finalizeCallback);
+virtual void Finalize(Napi::Env env);
 ```
 
-- `[in] instance`: `this` pointer from the native instance.
-- `[in] finalizeCallback`: function that implements [napi_finalize](https://nodejs.org/api/n-api.html#n_api_napi_finalize "N-API documentation for napi_finalize.").
-
-`Napi::ObjectWrap::OverrideFinalizeCallback` is protected and
-intended to be called from a native instance method; for example, in the native
-constructor.
-
-NOTE: The default `Napi::ObjectWrap::FinalizeCallback` frees
-the native instance.  A user defined finalzier is, therefore, 
-responsible for freeing the native instance.
+- `[in] env`: `Napi::Env`.
 
 ### StaticMethod
 
diff --git a/napi-inl.h b/napi-inl.h
index d6db0a642..4e619a87d 100644
--- a/napi-inl.h
+++ b/napi-inl.h
@@ -3260,11 +3260,7 @@ inline ClassPropertyDescriptor<T> ObjectWrap<T>::InstanceValue(
 }
 
 template <typename T>
-inline void ObjectWrap<T>::OverrideFinalizeCallback(T* instance,
-    napi_finalize finalizeCallback) {
-  ObjectWrap<T>* base = instance;
-  base->_finalizeCallbackOverride = finalizeCallback;
-}
+inline void ObjectWrap<T>::Finalize(Napi::Env env) {}
 
 template <typename T>
 inline napi_value ObjectWrap<T>::ConstructorCallbackWrapper(
@@ -3407,13 +3403,10 @@ inline napi_value ObjectWrap<T>::InstanceSetterCallbackWrapper(
 }
 
 template <typename T>
-inline void ObjectWrap<T>::FinalizeCallback(napi_env env, void* data, void* hint) {
+inline void ObjectWrap<T>::FinalizeCallback(napi_env env, void* data, void* /*hint*/) {
   T* instance = reinterpret_cast<T*>(data);
-  if(instance->_finalizeCallbackOverride == nullptr){
-    delete instance;
-  } else {
-    (*(instance->_finalizeCallbackOverride))(env, data, hint);
-  }
+  instance->Finalize(Napi::Env(env));
+  delete instance;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
diff --git a/napi.h b/napi.h
index 393f693ea..ff4968e2e 100644
--- a/napi.h
+++ b/napi.h
@@ -1657,10 +1657,8 @@ namespace Napi {
     static PropertyDescriptor InstanceValue(Symbol name,
                                             Napi::Value value,
                                             napi_property_attributes attributes = napi_default);
-  protected:
-    static void OverrideFinalizeCallback(T* instance, napi_finalize finalizeCallback);
+    virtual void Finalize(Napi::Env env);
   private:
-    napi_finalize _finalizeCallbackOverride = nullptr;
     static napi_value ConstructorCallbackWrapper(napi_env env, napi_callback_info info);
     static napi_value StaticVoidMethodCallbackWrapper(napi_env env, napi_callback_info info);
     static napi_value StaticMethodCallbackWrapper(napi_env env, napi_callback_info info);

From 03a235be733ad7ac048c8754349cf879e6b51dfb Mon Sep 17 00:00:00 2001
From: Michael Price <mikepricedev@gmail.com>
Date: Wed, 24 Jul 2019 15:52:07 -0600
Subject: [PATCH 3/6] Updates ObjectWrap::Finalize documention and fixes
 formatting.

Todo: Add ObjectWrap::Finalize test.

Co-authored-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
---
 doc/object_wrap.md | 4 ++--
 napi.h             | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/doc/object_wrap.md b/doc/object_wrap.md
index 13c38695a..24ff7463d 100644
--- a/doc/object_wrap.md
+++ b/doc/object_wrap.md
@@ -192,8 +192,8 @@ Returns a `Napi::Function` representing the constructor function for the class.
 
 ### Finalize
 
-Hooks into `Napi::ObjectWrap::FinalizeCallback` giving access to `Napi::Env`
-before the native instance is freed.
+Provides an opportunity to run cleanup code that requires access to the `Napi::Env` 
+before the wrapped native object instance is freed.  Override to implement. 
 
 ```cpp
 virtual void Finalize(Napi::Env env);
diff --git a/napi.h b/napi.h
index ff4968e2e..3140cd3ef 100644
--- a/napi.h
+++ b/napi.h
@@ -1658,6 +1658,7 @@ namespace Napi {
                                             Napi::Value value,
                                             napi_property_attributes attributes = napi_default);
     virtual void Finalize(Napi::Env env);
+
   private:
     static napi_value ConstructorCallbackWrapper(napi_env env, napi_callback_info info);
     static napi_value StaticVoidMethodCallbackWrapper(napi_env env, napi_callback_info info);

From cf5a57e5a034d93ce0c2a5655d651a3119451568 Mon Sep 17 00:00:00 2001
From: Michael Price <mikepricedev@gmail.com>
Date: Wed, 24 Jul 2019 17:30:47 -0600
Subject: [PATCH 4/6] Adds ObjectWrap::Finalize test.

Co-authored-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
---
 test/objectwrap.cc | 17 +++++++++++++++++
 test/objectwrap.js | 21 ++++++++++++++++++++-
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/test/objectwrap.cc b/test/objectwrap.cc
index ef1cfa73e..70809421d 100644
--- a/test/objectwrap.cc
+++ b/test/objectwrap.cc
@@ -24,6 +24,11 @@ class Test : public Napi::ObjectWrap<Test> {
 public:
   Test(const Napi::CallbackInfo& info) :
     Napi::ObjectWrap<Test>(info) {
+    
+    if(info.Length() > 0){
+      finalizerCb_ = Napi::Persistent(info[0].As<Napi::Function>());
+    }
+
   }
 
   void Setter(const Napi::CallbackInfo& /*info*/, const Napi::Value& value) {
@@ -105,8 +110,20 @@ class Test : public Napi::ObjectWrap<Test> {
     }));
   }
 
+  void Finalize(Napi::Env env){
+    
+    if(finalizerCb_.IsEmpty()){
+      return;
+    }
+
+    finalizerCb_.Call(env.Global(), {Napi::Boolean::New(env, true)});
+    finalizerCb_.Unref();
+
+  }
+
 private:
   std::string value_;
+  Napi::FunctionReference finalizerCb_;
 };
 
 Napi::Object InitObjectWrap(Napi::Env env) {
diff --git a/test/objectwrap.js b/test/objectwrap.js
index aa6d8d3cb..663006b00 100644
--- a/test/objectwrap.js
+++ b/test/objectwrap.js
@@ -182,6 +182,24 @@ const test = (binding) => {
     }
   };
 
+  const testFinalizer = (clazz) => {
+    
+    let finalizerCalled = false;
+    const finalizerCb = function(called){
+      finalizerCalled = called;
+    };
+
+    //Scope Test instance so that it can be gc'd
+    (function(){
+      new Test(finalizerCb);
+    })();
+
+    global.gc();
+
+    assert.strictEqual(finalizerCalled, true);
+
+  };
+
   const testObj = (obj, clazz) => {
     testValue(obj, clazz);
     testAccessor(obj, clazz);
@@ -197,7 +215,8 @@ const test = (binding) => {
     testStaticAccessor(clazz);
     testStaticMethod(clazz);
 
-    testStaticEnumerables(clazz);
+    testStaticEnumerables(clazz);    
+    testFinalizer(clazz);
   };
 
   // `Test` is needed for accessing exposed symbols

From 9dcc3244751f3edb125fe57a2832c8ae3945e667 Mon Sep 17 00:00:00 2001
From: Michael Price <mikepricedev@gmail.com>
Date: Wed, 24 Jul 2019 21:17:25 -0600
Subject: [PATCH 5/6] Fixes style and minor semantic variations.

Co-authored-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
---
 test/objectwrap.cc | 14 +++++++-------
 test/objectwrap.js | 18 +++++++++---------
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/test/objectwrap.cc b/test/objectwrap.cc
index 70809421d..9eadca7a5 100644
--- a/test/objectwrap.cc
+++ b/test/objectwrap.cc
@@ -25,8 +25,8 @@ class Test : public Napi::ObjectWrap<Test> {
   Test(const Napi::CallbackInfo& info) :
     Napi::ObjectWrap<Test>(info) {
     
-    if(info.Length() > 0){
-      finalizerCb_ = Napi::Persistent(info[0].As<Napi::Function>());
+    if(info.Length() > 0) {
+      finalizeCb_ = Napi::Persistent(info[0].As<Napi::Function>());
     }
 
   }
@@ -110,20 +110,20 @@ class Test : public Napi::ObjectWrap<Test> {
     }));
   }
 
-  void Finalize(Napi::Env env){
+  void Finalize(Napi::Env env) {
     
-    if(finalizerCb_.IsEmpty()){
+    if(finalizeCb_.IsEmpty()) {
       return;
     }
 
-    finalizerCb_.Call(env.Global(), {Napi::Boolean::New(env, true)});
-    finalizerCb_.Unref();
+    finalizeCb_.Call(env.Global(), {Napi::Boolean::New(env, true)});
+    finalizeCb_.Unref();
 
   }
 
 private:
   std::string value_;
-  Napi::FunctionReference finalizerCb_;
+  Napi::FunctionReference finalizeCb_;
 };
 
 Napi::Object InitObjectWrap(Napi::Env env) {
diff --git a/test/objectwrap.js b/test/objectwrap.js
index 663006b00..beb84168e 100644
--- a/test/objectwrap.js
+++ b/test/objectwrap.js
@@ -182,21 +182,21 @@ const test = (binding) => {
     }
   };
 
-  const testFinalizer = (clazz) => {
+  const testFinalize = (clazz) => {
     
-    let finalizerCalled = false;
-    const finalizerCb = function(called){
-      finalizerCalled = called;
+    let finalizeCalled = false;
+    const finalizeCb = function(called) {
+      finalizeCalled = called;
     };
 
-    //Scope Test instance so that it can be gc'd
-    (function(){
-      new Test(finalizerCb);
+    //Scope Test instance so that it can be gc'd.
+    (function() {
+      new Test(finalizeCb);
     })();
 
     global.gc();
 
-    assert.strictEqual(finalizerCalled, true);
+    assert.strictEqual(finalizeCalled, true);
 
   };
 
@@ -216,7 +216,7 @@ const test = (binding) => {
     testStaticMethod(clazz);
 
     testStaticEnumerables(clazz);    
-    testFinalizer(clazz);
+    testFinalize(clazz);
   };
 
   // `Test` is needed for accessing exposed symbols

From d19ba0f25df576d389855d864c1f3a45fb36498f Mon Sep 17 00:00:00 2001
From: Michael Price <mikepricedev@gmail.com>
Date: Thu, 25 Jul 2019 17:56:54 -0600
Subject: [PATCH 6/6] Adds virtual dtor to ObjectWrap and updates
 implementation of ObjectWrap::Finalize env param outlinted here:
 https://github.com/nodejs/node-addon-api/pull/515#issuecomment-515212700

Co-authored-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Co-authored-by: Michael Dawson <michael_dawson@ca.ibm.com>>
---
 napi-inl.h | 5 ++++-
 napi.h     | 3 ++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/napi-inl.h b/napi-inl.h
index 4e619a87d..4b32117b2 100644
--- a/napi-inl.h
+++ b/napi-inl.h
@@ -2886,6 +2886,9 @@ inline ObjectWrap<T>::ObjectWrap(const Napi::CallbackInfo& callbackInfo) {
   *instanceRef = Reference<Object>(env, ref);
 }
 
+template<typename T>
+inline ObjectWrap<T>::~ObjectWrap() {}
+
 template<typename T>
 inline T* ObjectWrap<T>::Unwrap(Object wrapper) {
   T* unwrapped;
@@ -3260,7 +3263,7 @@ inline ClassPropertyDescriptor<T> ObjectWrap<T>::InstanceValue(
 }
 
 template <typename T>
-inline void ObjectWrap<T>::Finalize(Napi::Env env) {}
+inline void ObjectWrap<T>::Finalize(Napi::Env /*env*/) {}
 
 template <typename T>
 inline napi_value ObjectWrap<T>::ConstructorCallbackWrapper(
diff --git a/napi.h b/napi.h
index 3140cd3ef..4d2d1ad99 100644
--- a/napi.h
+++ b/napi.h
@@ -1570,7 +1570,8 @@ namespace Napi {
   class ObjectWrap : public Reference<Object> {
   public:
     ObjectWrap(const CallbackInfo& callbackInfo);
-
+    virtual ~ObjectWrap();
+    
     static T* Unwrap(Object wrapper);
 
     // Methods exposed to JavaScript must conform to one of these callback signatures.