Fix deprecation warnings from the Nan library

[DavidVujic]

The code in node-zk.cpp causes warnings when installing zookeeper. This should be fixed, before deprecation warnings become errors.

Output from the command npm install zookeeper

../src/node-zk.cpp:547:14: warning: 'MakeCallback' is deprecated [-Wdeprecated-declarations]
        Nan::MakeCallback(thisObj, "emit", 3, argv);
             ^
../../nan/nan.h:1001:3: note: 'MakeCallback' has been explicitly marked deprecated here
  NAN_DEPRECATED inline v8::Local<v8::Value> MakeCallback(
  ^
../../nan/nan.h:103:40: note: expanded from macro 'NAN_DEPRECATED'
# define NAN_DEPRECATED __attribute__((deprecated))
                                       ^
../src/node-zk.cpp:622:9: warning: 'Call' is deprecated [-Wdeprecated-declarations]
        CALLBACK_EPILOG();
        ^
../src/node-zk.cpp:564:19: note: expanded from macro 'CALLBACK_EPILOG'
        callback->Call(sizeof(argv)/sizeof(argv[0]), argv); \
                  ^
../../nan/nan.h:1673:3: note: 'Call' has been explicitly marked deprecated here
  NAN_DEPRECATED inline v8::Local<v8::Value>
  ^
../../nan/nan.h:103:40: note: expanded from macro 'NAN_DEPRECATED'
# define NAN_DEPRECATED __attribute__((deprecated))
                                       ^
../src/node-zk.cpp:651:9: warning: 'Call' is deprecated [-Wdeprecated-declarations]
        CALLBACK_EPILOG();
        ^
../src/node-zk.cpp:564:19: note: expanded from macro 'CALLBACK_EPILOG'
        callback->Call(sizeof(argv)/sizeof(argv[0]), argv); \
                  ^
../../nan/nan.h:1673:3: note: 'Call' has been explicitly marked deprecated here
  NAN_DEPRECATED inline v8::Local<v8::Value>
  ^
../../nan/nan.h:103:40: note: expanded from macro 'NAN_DEPRECATED'
# define NAN_DEPRECATED __attribute__((deprecated))
                                       ^
../src/node-zk.cpp:693:9: warning: 'Call' is deprecated [-Wdeprecated-declarations]
        CALLBACK_EPILOG();
        ^
../src/node-zk.cpp:564:19: note: expanded from macro 'CALLBACK_EPILOG'
        callback->Call(sizeof(argv)/sizeof(argv[0]), argv); \
                  ^
../../nan/nan.h:1673:3: note: 'Call' has been explicitly marked deprecated here
  NAN_DEPRECATED inline v8::Local<v8::Value>
  ^
../../nan/nan.h:103:40: note: expanded from macro 'NAN_DEPRECATED'
# define NAN_DEPRECATED __attribute__((deprecated))
                                       ^
../src/node-zk.cpp:724:9: warning: 'Call' is deprecated [-Wdeprecated-declarations]
        CALLBACK_EPILOG();
        ^
../src/node-zk.cpp:564:19: note: expanded from macro 'CALLBACK_EPILOG'
        callback->Call(sizeof(argv)/sizeof(argv[0]), argv); \
                  ^
../../nan/nan.h:1673:3: note: 'Call' has been explicitly marked deprecated here
  NAN_DEPRECATED inline v8::Local<v8::Value>
  ^
../../nan/nan.h:103:40: note: expanded from macro 'NAN_DEPRECATED'
# define NAN_DEPRECATED __attribute__((deprecated))
                                       ^
../src/node-zk.cpp:748:9: warning: 'Call' is deprecated [-Wdeprecated-declarations]
        WATCHER_CALLBACK_EPILOG();
        ^
../src/node-zk.cpp:568:19: note: expanded from macro 'WATCHER_CALLBACK_EPILOG'
        callback->Call(sizeof(argv)/sizeof(argv[0]), argv); \
                  ^
../../nan/nan.h:1673:3: note: 'Call' has been explicitly marked deprecated here
  NAN_DEPRECATED inline v8::Local<v8::Value>
  ^
../../nan/nan.h:103:40: note: expanded from macro 'NAN_DEPRECATED'
# define NAN_DEPRECATED __attribute__((deprecated))
                                       ^
../src/node-zk.cpp:789:9: warning: 'Call' is deprecated [-Wdeprecated-declarations]
        CALLBACK_EPILOG();
        ^
../src/node-zk.cpp:564:19: note: expanded from macro 'CALLBACK_EPILOG'
        callback->Call(sizeof(argv)/sizeof(argv[0]), argv); \
                  ^
../../nan/nan.h:1673:3: note: 'Call' has been explicitly marked deprecated here
  NAN_DEPRECATED inline v8::Local<v8::Value>
  ^
../../nan/nan.h:103:40: note: expanded from macro 'NAN_DEPRECATED'
# define NAN_DEPRECATED __attribute__((deprecated))
                                       ^
../src/node-zk.cpp:826:9: warning: 'Call' is deprecated [-Wdeprecated-declarations]
        CALLBACK_EPILOG();
        ^
../src/node-zk.cpp:564:19: note: expanded from macro 'CALLBACK_EPILOG'
        callback->Call(sizeof(argv)/sizeof(argv[0]), argv); \
                  ^
../../nan/nan.h:1673:3: note: 'Call' has been explicitly marked deprecated here
  NAN_DEPRECATED inline v8::Local<v8::Value>
  ^
../../nan/nan.h:103:40: note: expanded from macro 'NAN_DEPRECATED'
# define NAN_DEPRECATED __attribute__((deprecated))
                                       ^
../src/node-zk.cpp:949:9: warning: 'Call' is deprecated [-Wdeprecated-declarations]
        CALLBACK_EPILOG();
        ^
../src/node-zk.cpp:564:19: note: expanded from macro 'CALLBACK_EPILOG'
        callback->Call(sizeof(argv)/sizeof(argv[0]), argv); \
                  ^
../../nan/nan.h:1673:3: note: 'Call' has been explicitly marked deprecated here
  NAN_DEPRECATED inline v8::Local<v8::Value>
  ^
../../nan/nan.h:103:40: note: expanded from macro 'NAN_DEPRECATED'
# define NAN_DEPRECATED __attribute__((deprecated))
                                       ^
9 warnings generated.

[jbienkowski311]

Hi!
I might have working solution for the deprecation warnings for Node v8.x. There are 2 methods that need fixing:

1. Nan::MakeCallback should be replaced with Nan::AsyncResource::runInAsyncScope
2. callback->Call should take Nan::AsyncResource* as an additional parameter.

However, there are more deprecation warnings for Node v10.x. I could submit PR with fixes for Node v8.x or I could try and fix the warnings for Node v10.x as well. Which approach should I take?

[DavidVujic]

Hi! Oh, I',m sorry. I forgot to assign myself to the issue. I have ongoing work for this, maybe we could collaborate? My C++ skills are not great. Do you have any feedback on this?
master...DavidVujic:node_12_support

[DavidVujic]

It is also a Node.js 12 support feature branch.

[jbienkowski311]

I will take a look and get back to you 😉

[jbienkowski311]

@DavidVujic I have taken a look and it looks promising!

There was one minor thing that I think could be done better. We are using namespace v8;, however we still use namespaced references like v8::Value instead of just Value. Moreover, in some places, the two are mixed together: static v8::Local<Value> toLocalVal(Local<Object> arg, Local<String> propertyName).
Other than that it's looking great! I've tested it using Docker and it was working fine (maybe I'm wrong?). Is there anything left to do?

[DavidVujic]

Thank you @jbienkowski311 for feedback and suggestions!

I will fix according to your suggestions, and am also planning to write some sort of tests to make sure the functionality is the same as before.

When ready, I will make a Pull Request and would appreciate if you could have a last look at the changes (I can ping you in a comment). I will also ask for feedback from fellow contributors to this project.

[jbienkowski311]

One more thing - I have seen that you have configured Travis CI on your fork. You could add node_js:12 so its automatically checked in future 😉

PS: it would be great if we could configure Travis CI checks for this repo as well.

[DavidVujic]

Thanks! Good catch! I'll add it to the fork. The main repo already has Travis CI enabled, so node_js:12 will also be merged.

[jbienkowski311]

Travis CI does not agree: https://travis-ci.org/yfinkelstein/node-zookeeper

[DavidVujic]

Maybe the other maintainers have done something similar as I do: a travis CI setup at a local fork of the original repo. When I started working with this repo, I was asked to add actions to the existing travis configuration. That is why I thought the main repo had something setup.

Could it also be that the link you posted is not public? But I totally agree it would be very good to have a green "Badge" at the README for quality.

[jbienkowski311]

The link not being public is an option. Green "badge" is one thing, and as long as it is pretty cool feature, more important thing is to setup up automatic checks for the pull requests. This way we can enforce many rules without user intervention.

[DavidVujic]

Yes, I agree. However, I haven't access to the Travis CI setup of the owner of this repo. What I do when verifying a PR, is to create a temporary "verify" branch and merge the changes into that. Then I push the code to my personal GitHub repo and the Travis CI will build and validate the changes.

If all green I will hit the "Merge" button.