-
-
Notifications
You must be signed in to change notification settings - Fork 211
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
Added support for Node 13 #348
Conversation
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 the fix!
Could you add Node 13 to the list of node versions in travis.yml
so we can test this fix on CI?
Thanks for adding it to |
There is also the Nan::Get() helper which might be a safe alternative? |
As I said, there might be better ways to fix it. I just needed it to work and figured I'd have a look, rather than downgrading Node, so I went with the least possible changes and stopped as soon as it worked. Otherwise, lately I've been using node-addon-api rather than N-API, NAN or V8. Anyway, I guess it's between Local<Object> buf = batch->Get(Nan::GetCurrentContext(), i).ToLocalChecked().As<Object>(); and Local<Object> buf = Nan::Get(batch, i).ToLocalChecked().As<Object>(); which is shorter and probably more stable, so might indeed be better. |
@rolftimmermans What do you think, can we merge this, or should we use I am not very familiar with this, but I can do a patch release once this PR is merged. |
Let’s just merge for now, we can always improve later! |
👍 @giovannicalo Thanks for the PR. Let's ship it |
As mentioned in #346, this library fails to install with Node 13.0.1, due to compilation errors.
After a quick investigation, the issue seems to be
which has already been deprecated for a while and seems to have been removed in favour of
in Node 13 / V8 7.8.
This seems to fix the issue.
It's only been tested with Node 12.13.0 and 13.0.1, on Windows.
This might break compatibility with older versions of Node, however the new method was already present (and the old one was already deprecated) in Node 6 / V8 5.1, so it shouldn't be the case.
It might not be the best way to fix it, but works for me.