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

NAN compatibility issues when moving from Node 10 -> 12 -> 14 #2722

Closed
sregger opened this issue May 23, 2020 · 4 comments
Closed

NAN compatibility issues when moving from Node 10 -> 12 -> 14 #2722

sregger opened this issue May 23, 2020 · 4 comments

Comments

@sregger
Copy link

sregger commented May 23, 2020

  • Node.js Version: 14.2.0, 12.16.3, 10.19.0
  • OS: Windows
  • Scope (install, code, runtime, meta, other?): install
  • Module (and version) (if relevant): NaN

I'm using an out of date nodejs module (the sspi-client) which has forced my project to be stuck on Node 10. I'd like to solve that by contributing a PR to this library. The error I see, if we move past Node 10, is

error C2660: 'v8::Value::BooleanValue': function does not take 0 arguments

Looking into the issue the 0 arg BooleanValue function was removed. It was replaced by the Context versions as detailed in this issue.

With this info in hand I forked and upgraded the module. That worked well for Node 12. However I ran into numerous issues running the modules tests and got side tracked / lost interest. Now with a renewed interest I got the tests to an okay (sort of) state. But I noticed that last time I only tried the stable release of Node (12). Assuming the latest, Node 14, would be fine I checked it out and ran an install. I saw the following error

error C2664: 'v8::Localv8::Boolean v8::Value::ToBoolean(v8::Isolate *) const': cannot convert argument 1 from 'v8::Localv8::Context' to 'v8::Isolate *'

Re-checking Node 12 I also see a warning

warning C4996: 'v8::Value::ToBoolean': was declared deprecated

I was able to "fix" the code for Node 14 using

info[0]->BooleanValue(Nan::GetCurrentContext()->GetIsolate())

But that, in turn, causes errors with Node 10. e.g.

error C2664: 'bool v8::Value::BooleanValue(void) const': cannot convert argument 1 from 'v8::Isolate *' to 'v8::Local<v8::Context>'

What is the correct upgrade path here?

@devsnek devsnek changed the title NaN compatibility issues when moving from Node 10 -> 12 -> 14 NAN compatibility issues when moving from Node 10 -> 12 -> 14 May 23, 2020
@bnoordhuis
Copy link
Member

Try Nan::To<bool>(info[0]).FromJust(). Or better yet:

bool on;
if (!Nan::To<bool>(info[0]).To(&on)) {
  // exception pending
} else if (on) {
  // ...
} else {
  // ....
}

@sregger
Copy link
Author

sregger commented May 24, 2020

Spot on @bnoordhuis. That works perfectly across all 3 versions of Node. Thank you.

PS: Any chance you know the upgrade path for

callback->Call(4, argv);

which produces

warning C4996: 'Nan::Callback::Call': was declared deprecated

It's only a warning but figure if I can I should tidy it up too.

@bnoordhuis
Copy link
Member

You're probably looking for Nan::Call(). It also works with a Nan::Callback:

Local<Value> retval;
if (Nan::Call(callback, recv, 4, argv).To(&retval)) {
  // ...
} else {
  // exception pending
}

recv is the receiver, the this object. It needs to be something but you can always use Nan::GetCurrentContext()->Global() to use the global object (i.e., globalThis.)

@sregger
Copy link
Author

sregger commented May 25, 2020

Thanks for all the help @bnoordhuis. That answers my question.

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

No branches or pull requests

2 participants