address review comments

- add `NAPI_VERSION` guard
- add `IsEmpty`

doc/env.md

@@ -168,3 +168,29 @@ called first.
 
 Returns an `Env::CleanupHook` object, which can be used to remove the hook via
 its `Remove()` method.
+
+# Env::CleanupHook
+
+The `Env::CleanupHook` object allows removal of the hook added via
+`Env::AddCleanupHook()`
+
+## Methods
+
+### IsEmpty
+
+```cpp
+bool IsEmpty();
+```
+
+Returns `true` if the cleanup hook was **not** successfully registered.
+
+### Remove
+
+```cpp
+bool Remove(Env env);
+```
+
+Unregisters the hook from running once the current Node.js environment exits.
+
+Returns `true` if the hook was successfully removed from the Node.js
+environment.

napi-inl.h

@@ -5709,6 +5709,7 @@ Addon<T>::DefineProperties(Object object,
 }
 #endif  // NAPI_VERSION > 5
 
+#if NAPI_VERSION > 2
 template <typename Hook, typename Arg>
 Env::CleanupHook<Hook, Arg> Env::AddCleanupHook(Hook hook, Arg* arg) {
   return CleanupHook<Hook, Arg>(*this, hook, arg);
@@ -5724,30 +5725,36 @@ Env::CleanupHook<Hook, Arg>::CleanupHook(Napi::Env env, Hook hook)
     : wrapper(Env::CleanupHook<Hook, Arg>::Wrapper) {
   data = new CleanupData{std::move(hook), nullptr};
   napi_status status = napi_add_env_cleanup_hook(env, wrapper, data);
-  NAPI_FATAL_IF_FAILED(status,
-                       "Env::CleanupHook<Hook, Arg>::CleanupHook Wrapper",
-                       "napi_add_env_cleanup_hook");
+  if (status != napi_ok) {
+    delete data;
+    data = nullptr;
+  }
 }
 
 template <typename Hook, typename Arg>
 Env::CleanupHook<Hook, Arg>::CleanupHook(Napi::Env env, Hook hook, Arg* arg)
     : wrapper(Env::CleanupHook<Hook, Arg>::WrapperWithArg) {
   data = new CleanupData{std::move(hook), arg};
   napi_status status = napi_add_env_cleanup_hook(env, wrapper, data);
-  NAPI_FATAL_IF_FAILED(
-      status,
-      "Env::CleanupHook<Hook, Arg>::CleanupHook WrapperWithArg",
-      "napi_add_env_cleanup_hook");
+  if (status != napi_ok) {
+    delete data;
+    data = nullptr;
+  }
 }
 
 template <class Hook, class Arg>
-void Env::CleanupHook<Hook, Arg>::Remove(Env env) {
+bool Env::CleanupHook<Hook, Arg>::Remove(Env env) {
   napi_status status = napi_remove_env_cleanup_hook(env, wrapper, data);
-  NAPI_FATAL_IF_FAILED(status,
-                       "Env::CleanupHook<Hook, Arg>::Remove",
-                       "napi_remove_env_cleanup_hook");
   delete data;
+  data = nullptr;
+  return status == napi_ok;
+}
+
+template <class Hook, class Arg>
+bool Env::CleanupHook<Hook, Arg>::IsEmpty() const {
+  return data == nullptr;
 }
+#endif  // NAPI_VERSION > 2
 
 } // namespace Napi
 

napi.h

@@ -203,11 +203,13 @@ namespace Napi {
     Value RunScript(const std::string& utf8script);
     Value RunScript(String script);
 
+#if NAPI_VERSION > 2
     template <typename Hook>
     CleanupHook<Hook> AddCleanupHook(Hook hook);
 
     template <typename Hook, typename Arg>
     CleanupHook<Hook, Arg> AddCleanupHook(Hook hook, Arg* arg);
+#endif  // NAPI_VERSION > 2
 
 #if NAPI_VERSION > 5
     template <typename T> T* GetInstanceData();
@@ -228,12 +230,14 @@ namespace Napi {
   private:
     napi_env _env;
 
+#if NAPI_VERSION > 2
     template <typename Hook, typename Arg>
     class CleanupHook {
      public:
       CleanupHook(Env env, Hook hook, Arg* arg);
       CleanupHook(Env env, Hook hook);
-      void Remove(Env env);
+      bool Remove(Env env);
+      bool IsEmpty() const;
 
      private:
       static inline void Wrapper(void* data) NAPI_NOEXCEPT;
@@ -246,6 +250,7 @@ namespace Napi {
       } * data;
     };
   };
+#endif  // NAPI_VERSION > 2
 
   /// A JavaScript value of unknown type.
   ///

test/env_cleanup.cc

@@ -58,7 +58,18 @@ Value AddHooks(const CallbackInfo& info) {
     hook6.Remove(env);
   }
 
-  return env.Undefined();
+  int added = 0;
+
+  added += !hook1.IsEmpty();
+  added += !hook1b.IsEmpty();
+  added += !hook2.IsEmpty();
+  added += !hook3.IsEmpty();
+  added += !hook3b.IsEmpty();
+  added += !hook4.IsEmpty();
+  added += !hook5.IsEmpty();
+  added += !hook6.IsEmpty();
+
+  return Number::New(env, added);
 }
 
 Object InitEnvCleanup(Env env) {

test/env_cleanup.js

@@ -7,7 +7,9 @@ if (process.argv[2] === 'runInChildProcess') {
     const remove_hooks = process.argv[4] === 'true';
 
     const binding = require(binding_path);
-    binding.env_cleanup.addHooks(remove_hooks);
+    const actualAdded = binding.env_cleanup.addHooks(remove_hooks);
+    const expectedAdded = remove_hooks === true ? 0 : 8;
+    assert(actualAdded === expectedAdded, 'Incorrect number of hooks added');
 }
 else {
     module.exports = require('./common').runTestWithBindingPath(test);