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.

Sign up for GitHub

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

Jump to bottom

Fix deprecation warnings on node 10.12 #811

Closed
wants to merge 1 commit into from
Closed

Fix deprecation warnings on node 10.12 #811

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions nan.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@
# error This version of node/NAN/v8 requires a C++11 compiler
#endif

#if NODE_MAJOR_VERSION == 10 && NODE_MINOR_VERSION >= 12
# define NAN_HAS_V8_7_DEPRECATIONS
#endif

#include <uv.h>
#include <node.h>
#include <node_buffer.h>
Expand Down Expand Up @@ -1060,8 +1064,9 @@ class Utf8String {
length_(0), str_(str_st_) {
HandleScope scope;
if (!from.IsEmpty()) {
#if V8_MAJOR_VERSION >= 7
v8::Local<v8::String> string = from->ToString(v8::Isolate::GetCurrent());
#if V8_MAJOR_VERSION >= 7 || defined(NAN_HAS_V8_7_DEPRECATIONS)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use #if NODE_MODULE_VERSION >= NODE_10_0_MODULE_VERSION to use the same API independent of the minor node version.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I checked the node module version and it didn't change between these two versions unless I'm missing something here? It might be possible for the cases where the new API already existed but there's many solutions for those. :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new variants of ToString() and WriteUtf8() are available already in 10.0.0 (or even before) only the StringObject is problematic as the new variant is only available since 10.12.0.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, exactly. :) But see above - I haven't updated this PR yet because there didn't seem to be agreement on how the real problem (the 3rd API) should be solved. I'm not a huge fan of churn when there's no clear target.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok. My understanding was the agreement is that users of StringObject via NAN have bad luck if they have to distribute precompiled binaries.
But maybe you are right and we should give this some more time and votes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since nobody saw it fit to increase the module version when rolling out breaking changes in a minor update and there is no nice way of resolving the issue, I'd say it is not our problem. NAN never promised ABI compatibility, Node says the ABI has not changed, bug reports go upstream and in time this will be a footnote in history.

v8::Local<v8::String> string = from->ToString(
v8::Isolate::GetCurrent()->GetCurrentContext()).FromMaybe(v8::Local<v8::String>());
#else
v8::Local<v8::String> string = from->ToString();
#endif
Expand All @@ -1074,7 +1079,7 @@ class Utf8String {
}
const int flags =
v8::String::NO_NULL_TERMINATION | imp::kReplaceInvalidUtf8;
#if V8_MAJOR_VERSION >= 7
#if V8_MAJOR_VERSION >= 7 || defined(NAN_HAS_V8_7_DEPRECATIONS)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use #if NODE_MODULE_VERSION >= NODE_10_0_MODULE_VERSION to use the same API independent of the minor node version.

length_ = string->WriteUtf8(v8::Isolate::GetCurrent(), str_, static_cast<int>(len), 0, flags);
#else
length_ = string->WriteUtf8(str_, static_cast<int>(len), 0, flags);
Expand Down
2 changes: 1 addition & 1 deletion nan_implementation_12_inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,7 @@ Factory<v8::String>::New(ExternalOneByteStringResource * value) {

Factory<v8::StringObject>::return_t
Factory<v8::StringObject>::New(v8::Local<v8::String> value) {
#if V8_MAJOR_VERSION >= 7
#if V8_MAJOR_VERSION >= 7 || defined(NAN_HAS_V8_7_DEPRECATIONS)
return v8::StringObject::New(v8::Isolate::GetCurrent(), value).As<v8::StringObject>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ai, I raised this in nodejs/node#23159 (comment) - it means you can't distribute a pre-compiled add-on and have it work with older Node.js v10.x releases. That seems bad. Maybe we should just accept this warning as unavoidable.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, for me it would mean that I would drop nan in my project. Because I know what lengths some of the users inside of Groupon will go to if they try to "fix" the warnings unfortunately. :(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it means you can't distribute a pre-compiled add-on and have it work with older Node.js v10.x releases.

As you mentioned in your linked comment – this would only affect people who actually use StringObject::New(), of which there aren’t many to begin with, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I haven't seen it used much. Small consolation if you're one of the unhappy few, though.

#else
return v8::StringObject::New(value).As<v8::StringObject>();
Expand Down