Skip to content

Conversation

@basti1302
Copy link
Contributor

@basti1302 basti1302 commented Aug 31, 2018

The node-gyp build breaks on Node.js 10 with

../netlinkwrapper.cc:58:41: error: no matching member function for call to 'NewInstance'
        args.GetReturnValue().Set(cons->NewInstance(argc, argv));
                                  ~~~~~~^~~~~~~~~~~

This change fixes this, so the module can also be used with the latest Node.js version.

Note that the build in Node.js 8 also emitted a corresponding deprecation warning:

../netlinkwrapper.cc:58:41: warning: 'NewInstance' is deprecated [-Wdeprecated-declarations]
        args.GetReturnValue().Set(cons->NewInstance(argc, argv));
                                        ^
/Users/bastian/.node-gyp/8.11.3/include/node/v8.h:3846:3: note: 'NewInstance' has been explicitly marked deprecated here
  V8_DEPRECATED("Use maybe version",

This warning is now also gone when building with Node 8.

I also added a test case for the TCP client functionality and tested the library for all the Node versions from 4 to 10 (latest patch version for each major version) successfully via rm -rf node_modules && npm install && rm -rf package-lock.json && node-gyp rebuild && npm test, so I'm quite confident that the change in the production code doesn't break the library for older versions.

@basti1302
Copy link
Contributor Author

I realize it has been a while since you last touched the library. If you are no longer maintaining this, please give me a ping.

Local<Value> argv[argc] = { args[0] };
Local<Function> cons = Local<Function>::New(isolate, constructor);
args.GetReturnValue().Set(cons->NewInstance(argc, argv));
args.GetReturnValue().Set(Nan::NewInstance(cons, argc, argv).ToLocalChecked());
Copy link
Contributor Author

@basti1302 basti1302 Aug 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JacobFischer
Copy link
Owner

Great work, I'll review this PR this weekend, and merge it if everything looks 👌.

I was planning on fixing the build for v10 via N-API, but was still wrapping my head around it (there's an old branch here on GitHub with the beginning of my inquiries to fix it). I fully intend to still maintain this repo, I just genuinely thought no one besides my own work used this so I could fix it at my leisure.

Thanks again for this.

@JacobFischer
Copy link
Owner

Reviewed this today. Excellent work. The module still works with older node versions (I tested on v9), while also now working on v10 as you stated. My own tests all passed with no changes to the TCP communications, and you added mocha test is a much needed improvement!

So, I'm merging this, and then I want to do some house cleaning (such as #5). Then I'll publish these changes as a new minor patch (v1.1.0).

Thanks for the excellent work!

@JacobFischer JacobFischer merged commit f99bda9 into JacobFischer:master Sep 1, 2018
@basti1302
Copy link
Contributor Author

Thanks for reviewing and merging. If you'd publish a new release that includes this in a somewhat timely fashion, that would be awesome.

@JacobFischer
Copy link
Owner

Planning to publish it today 😄

@basti1302 basti1302 deleted the fix-node-gyp-build-node-10 branch September 1, 2018 17:00
@JacobFischer
Copy link
Owner

@basti1302 V1.1.0 is live now:

https://www.npmjs.com/package/netlinkwrapper/v/1.1.0

Thanks again for your work.

@JacobFischer JacobFischer mentioned this pull request Jun 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants