-
Notifications
You must be signed in to change notification settings - Fork 501
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 v10.12.0 deprecation warnings. #825
Conversation
nan_implementation_12_inl.h
Outdated
@@ -337,7 +345,23 @@ Factory<v8::StringObject>::New(v8::Local<v8::String> value) { | |||
#if V8_MAJOR_VERSION >= 7 |
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.
At the other locations this #if
was replaced by #if NODE_MAJOR_VERSION >= 10
. Is it kept intentionally here?
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.
Yes. Should I add a comment?
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.
Up to you.
Personally I find the #ifs on V8 version not nice as it indicates that it breaks the API/ABI stability within a major nodejs version.
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.
Yeah, I agree. I've pushed a new commit that removes it, PTAL.
Rebased onto master. The CI should be passing now that #826 is merged. |
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.
🚢 🚢 🚢
Nice job! LGTM. |
This commit was brought to you by `perl -i -pe` (well, mostly - I did some manual fixups.) PR-URL: #825 Reviewed-By: Benjamin Byholm <bbyholm@abo.fi>
Thanks for the review, Benjamin. Landed in e6ef6a4...e222068. |
v10.12.0 turns on a number of V8 deprecation warnings. This commit fixes
them in NAN.
Fixes: #810
Refs: #811