Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deprecation warning from Nan::Callback::call #209

Closed
nickdesaulniers opened this issue Jun 30, 2019 · 2 comments
Closed

deprecation warning from Nan::Callback::call #209

nickdesaulniers opened this issue Jun 30, 2019 · 2 comments

Comments

@nickdesaulniers
Copy link
Owner

via npm install:

../src/node_nanomsg.cc: In member function ‘virtual void NanomsgDeviceWorker::HandleOKCallback()’:
../src/node_nanomsg.cc:233:27: warning: ‘v8::Local<v8::Value> Nan::Callback::Call(int, v8::Local<v8::Value>*) const’ is deprecated [-Wdeprecated-declarations]
     callback->Call(1, argv);
                           ^
In file included from ../src/poll_ctx.h:3:0,
                 from ../src/node_nanomsg.cc:7:
../node_modules/nan/nan.h:1740:3: note: declared here
   Call(int argc, v8::Local<v8::Value> argv[]) const {
   ^~~~
  CXX(target) Release/obj.target/node_nanomsg/src/poll_ctx.o
../src/poll_ctx.cc: In member function ‘void PollCtx::invoke_callback(int) const’:
../src/poll_ctx.cc:37:24: warning: ‘v8::Local<v8::Value> Nan::Callback::Call(int, v8::Local<v8::Value>*) const’ is deprecated [-Wdeprecated-declarations]
   callback.Call(1, argv);
                        ^
In file included from ../src/poll_ctx.h:3:0,
                 from ../src/poll_ctx.cc:2:
../node_modules/nan/nan.h:1740:3: note: declared here
   Call(int argc, v8::Local<v8::Value> argv[]) const {
   ^~~~

https://github.com/nodejs/nan/blob/master/doc/node_misc.md#api_nan_make_callback

@nickdesaulniers
Copy link
Owner Author

Looks like those interfaces are deprecated in favor of ones that additionally pass a Nan::AsyncResource.

https://github.com/nodejs/nan/blob/master/doc/callback.md#api_nan_callback
https://github.com/nodejs/nan/blob/master/doc/node_misc.md#api_nan_make_callback

@nickdesaulniers
Copy link
Owner Author

so src/node_nanomsg.cc:233 is easy; we already subclass Nan::AsyncWorker, so just:

diff --git a/src/node_nanomsg.cc b/src/node_nanomsg.cc
index ab66db5..9b40b5e 100644
--- a/src/node_nanomsg.cc
+++ b/src/node_nanomsg.cc
@@ -230,7 +230,7 @@ public:
 
     Local<Value> argv[] = { Nan::New<Number>(err) };
 
-    callback->Call(1, argv);
+    callback->Call(1, argv, async_resource);
   };
 
 private:

is necessary. The change for poll_ctx.cc looks more involved, since we hand rolled a bit.

nickdesaulniers added a commit that referenced this issue Jul 1, 2019
Looks like the Nan::Callback::Call() API deprecated bare calls that
don't pass a new Nan::AsyncResource().  Luckily, in this case we were
already subclassing Nan::AsyncWorker, which has a member named
`async_resource` so all we have to do is explicitly pass this along.

I wonder if Nan should just do this for me? *hmm*

Bug #209
nickdesaulniers added a commit that referenced this issue Jul 2, 2019
* support node v12
- upgrade nan to v2.14
- use  Nan::Utf8String rather than v8::String::Utf8Value

Fixes #206

* fix 1 of the 2 Nan::Callback::Call deprecation warnings

Looks like the Nan::Callback::Call() API deprecated bare calls that
don't pass a new Nan::AsyncResource().  Luckily, in this case we were
already subclassing Nan::AsyncWorker, which has a member named
`async_resource` so all we have to do is explicitly pass this along.

I wonder if Nan should just do this for me? *hmm*

Bug #209
nickdesaulniers added a commit that referenced this issue Jul 4, 2019
This allows PollCtx to leave the lifetime management of the callback to
AsyncWorker, and allows us to use the AsyncWorker's async_resource.

No change in `make perf` was observed, though it's a little noisy.

We must implement the pure virtual function interface for Exectute(),
but I'm not sure what it's for.

Also, I believe that ~AsyncWorker() deletes the passed callback, as all
of the examples show calls to new Nan::Callback().

Fixes #209
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant