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
update calls to use contexts and convert Maybes into Checked values, … #3
Conversation
…for compatibility with node 12.6.0 and v8 >= 7.5.288.22
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.
Thanks for doing the research! Couple small nitpicks here. Might as well get the deprecated stuff over with too.
src/zlib_sync.cc
Outdated
@@ -65,15 +65,15 @@ class ZlibSyncInflate : public ObjectWrap { | |||
|
|||
static NAN_METHOD(Push) { | |||
ZlibSyncInflate* self = ObjectWrap::Unwrap<ZlibSyncInflate>(info.This()); | |||
|
|||
auto context = info.GetIsolate()->GetCurrentContext(); |
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.
Since we're using nan
anyway, let's stick with Nan::GetCurrentContext()
for this and the other context.
Also, Local<Context>
is a bit more readable than auto
.
Local<Object> buffer = Local<Object>::Cast(info[0]); | ||
int flush = Z_NO_FLUSH; | ||
if(info[1]->IsBoolean()) { | ||
if(info[1]->BooleanValue()) { | ||
if(info[1]->BooleanValue(context).ToChecked()) { |
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.
Since we can use the magic of nan
, Nan::To<bool>().FromJust()
would be preferred. Some of these *Value()
calls are deprecated anyway.
@@ -201,7 +201,7 @@ class ZlibSyncInflate : public ObjectWrap { | |||
Nan::SetAccessor(itpl, Nan::New<String>("result").ToLocalChecked(), GetResult); | |||
Nan::SetAccessor(itpl, Nan::New<String>("windowBits").ToLocalChecked(), GetWindowBits); | |||
|
|||
target->Set(Nan::New<String>("Inflate").ToLocalChecked(), tpl->GetFunction()); | |||
target->Set(Nan::New<String>("Inflate").ToLocalChecked(), tpl->GetFunction(context).ToLocalChecked()); |
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.
This thing is deprecated too, could add context
as first arg and a ToChecked()
at the end.
You're welcome. Thanks for making this package! Sorry for the late reply. I have been busy working on a cancellable tasks async addon for ES2019. I will make the requested changes and push a new pull request. Is that still something you'd still like me to do? I'm only asking because I don't know if you have already made the changes yourself. Cheers |
Go ahead. Just commit to the branch, no need to make a new PR |
Though, if this is going to take another 8 days, I'll just make the changes here. |
…for compatibility with node 12.6.0 and v8 >= 7.5.288.22