-
Notifications
You must be signed in to change notification settings - Fork 144
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
Fix Node.js 12 support #499
Conversation
v8::Handle has been deprecated in favor of v8::Local since V8 4.5 and has been removed somehwere after V8 7.0. Node.js 12 (current stable) is upgrading V8 engine to 7.4+ which does not support v8::Handle. Mass-replace v8::Handle -> v8::Local to avoid that issue. It's supported even by the ancient Node versions packaged in older Ubuntu releases.
v8::Object::Set() and v8::FunctionTemplate::GetFunction() have had their API changed between V8 versions. Upgrading Node.js to 12 breaks them completely. Instead of using this API directly we should be using NAN helpers that encapsulate the changes and provide common API for Maybe types.
but can we add checks for different node versions to CircleCI later? or check them in BuildBot 🤔 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sometimes js looks like magic
// Prepare constructor template | ||
v8::Local<v8::FunctionTemplate> tpl = Nan::New<v8::FunctionTemplate>(SecureCellContextImprint::New); | ||
tpl->SetClassName(Nan::New("SecureCellContextImprint").ToLocalChecked()); | ||
tpl->SetClassName(className); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch (here and below)
Sure thing. I'll add them, just in a separate PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Latest stable release of Node.js 12 includes updated V8 engine which has finally removed some of the deprecated API that we are using. JavaScript ecosystem has relatively low half-life periods for versions so we need to upgrade.
v8::Handle
has been deprecated in favor ofv8::Local
since V8 4.5 and has been removed somewhere after V8 7.0. Node.js 12 (current stable) is upgrading V8 engine to 7.4+ which does not support v8::Handle.Mass-replace v8::Handle → v8::Local to avoid that issue. It's supported even by the ancient Node versions packaged in older Ubuntu releases.
v8::Object::Set()
andv8::FunctionTemplate::GetFunction()
have had their API changed between V8 versions. Upgrading Node.js to 12 breaks them completely.Instead of using this API directly we should be using NAN helpers that encapsulate the changes and provide common API for Maybe types.
I have manually verified these changes with the following Node.js versions:
Currently CircleCI checks only Node.js v4. This PR does not update the build system so you'll have to trust me that it actually builds, for now.