Fix deprecation warnings on node 10.12 #811

jkrems · 2018-10-11T15:28:19Z

node commit: nodejs/node@46c7d0d

Fixes #810

jkrems · 2018-10-11T15:29:28Z

I started out trying to make this based on V8_*_VERSION but only the embedder string changed and the previous node 10 builds didn't have the APIs needed to support the post-deprecation code.

jkrems · 2018-10-11T15:34:20Z

@addaleax Is there a better way to detect support for the new APIs?

node commit: nodejs/node@46c7d0d Fixes nodejs#810

kkoopa · 2018-10-11T16:43:35Z

I started out trying to make this based on V8_*_VERSION but only the embedder string changed and the previous node 10 builds didn't have the APIs needed to support the post-deprecation code.

That seems a bit fishy. Was there some #define changed in Node which caused existing deprecations to become visible?

jkrems · 2018-10-11T16:59:39Z

Afaict it was introduced in the node commit linked above. It's a patch on top of V8 floated by node if I'm reading it correctly.

kkoopa · 2018-10-11T17:04:10Z

Ah, that explains it. I must have missed the commit link.

addaleax · 2018-10-11T17:25:48Z

@jkrems I think this is fine – StringObject should be the only one where this is actually necessary, the others should all work on V8 6.9 already?

jkrems · 2018-10-11T17:36:01Z

the others should all work on V8 6.9 already?

Totally possible. I got lazy once I finally found a working guard for the StringObject case and just blindly reused it.

Flarna · 2018-10-11T17:42:07Z

If I understand this change correct it turns compilation warnings into incompatibility.
E.g. a native add-on using NAN compiled with node 10.12.0 would not work with 10.11.0 as the v8 APIs used are not available there. The other way around should work.
If an add-on is using v8 directly it doesn't face this issue.
I know the target of NAN is not to avoid such problems but I think the price to get rid of warnings is high here.
To my understanding the node change was done to give add-on owners more time to adapt for node 11 and NAN is already prepared for this.
Maybe just silence the warnings within the NAN files?

addaleax · 2018-10-11T17:45:53Z

E.g. a native add-on using NAN compiled with node 10.12.0 would not work with 10.11.0 as the v8 APIs used are not available there. The other way around should work.

That’s correct, yes; but only for the StringObject construction case. I doubt there is more than a handful of people who have ever used it in addons.

Flarna · 2018-10-11T17:53:52Z

But then the ifdefs should be adapted to select the new APIs for all node 10 versions.

bnoordhuis · 2018-10-11T20:34:01Z

nan_implementation_12_inl.h

@@ -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>();


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.

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. :(

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?

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

refack · 2018-10-11T22:23:04Z

I've cobbled up this monstrosity:

#pragma warning(push)
#pragma warning(disable : 4996)
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wdeprecated-declarations"
#include "nan.h"
#pragma clang diagnostic pop
#pragma GCC diagnostic pop
#pragma warning(pop)

and this to binding.gyp

'defines': [ '-Wno-unknown-pragmas', ],

So at least you don't get warning on the declarations.

or

You could add this to your binding.gyp

'defines': [ '-Wno-deprecated-declarations', ],

jkrems · 2018-10-12T14:40:54Z

For now I'll go with something like this:

      "conditions": [
        [
          '"<!(echo $V)" != "1"',
          {
            "cflags": [
              "-Wno-deprecated-declarations",
            ],
            "xcode_settings": {
              "OTHER_CFLAGS": [
                "-Wno-deprecated-declarations",
              ],
            },
          },
        ],
      ],

Not really windows compatible but allows seeing deprecations in verbose mode while not leaking them into npm install of downstream consumers. 👍

jkrems · 2018-10-12T14:42:10Z

Unless there's a way to fix this without breaking compatibility - should this PR be closed?

refack · 2018-10-12T15:36:29Z

For windows comapt add

            'msvs_disabled_warnings': [4996],

like:

      "conditions": [
        [
          '"<!(echo $V)" != "1"',
          {
            "cflags": [
              "-Wno-deprecated-declarations",
            ],
            "xcode_settings": {
              "OTHER_CFLAGS": [
                "-Wno-deprecated-declarations",
              ],
            },
            'msvs_disabled_warnings': [4996],
          },
        ],
      ],

Flarna · 2018-10-22T18:27:19Z

nan.h

@@ -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)


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

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. :)

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.

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.

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.

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.

Flarna · 2018-10-22T18:27:30Z

nan.h

@@ -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)


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

* while waiting for nodejs/nan#811

Hide deprecation warnings - see nodejs/nan#811

erikdubbelboer · 2018-11-13T04:57:15Z

Any update on this? It seems like a lot of modules are fixing this themselves by adding -Wno-deprecated-declarations to their binding.gyp but a fix in NAN itself would obviously be a lot better!

bnoordhuis · 2018-11-26T00:21:34Z

Superseded by #825. Thanks anyway for the pull request, @jkrems.

Hide deprecation warnings - see nodejs/nan#811

v10.12.0 turns on a number of V8 deprecation warnings. This commit fixes them in NAN. Fixes: nodejs/nan#810 PR-URL: nodejs/nan#825 Refs: nodejs/nan#811 Reviewed-By: Benjamin Byholm <[email protected]>

Fix deprecation warnings on node 10.12

d63a550

node commit: nodejs/node@46c7d0d Fixes nodejs#810

jkrems force-pushed the feature/master/node-10-12-deprecations branch from 44eaa1c to d63a550 Compare October 11, 2018 15:35

bnoordhuis reviewed Oct 11, 2018

View reviewed changes

addaleax mentioned this pull request Oct 11, 2018

build: add common defines nodejs/node#23426

Merged

3 tasks

Flarna reviewed Oct 22, 2018

View reviewed changes

jhecking mentioned this pull request Nov 1, 2018

Fix deprecation warnings in Node 10.12 & 11 builds aerospike/aerospike-client-nodejs#287

Merged

3 tasks

Flarna mentioned this pull request Nov 3, 2018

deps: adapt to V8 7.0 #808

Merged

chjj mentioned this pull request Nov 3, 2018

v8/node deprecated APIs and how to handle them bcoin-org/bcrypto#7

Closed

royaltm mentioned this pull request Nov 9, 2018

AppVeyor builds are very slow royaltm/node-murmurhash-native#16

Open

royaltm added a commit to royaltm/node-murmurhash-native that referenced this pull request Nov 9, 2018

suppress deprecation warnings from nan

77ad8f0

* while waiting for nodejs/nan#811

lovell added a commit to lovell/sharp that referenced this pull request Nov 11, 2018

Add Node 11 to CI, experimental only, no prebuild

1c4f6f7

Hide deprecation warnings - see nodejs/nan#811

koooge mentioned this pull request Nov 12, 2018

deprecation warnings on node 10.13.0 erikdubbelboer/node-sleep#93

Closed

This was referenced Nov 13, 2018

node.js 10.13 [email protected] error during installation bnoordhuis/node-iconv#190

Closed

Fix Node.js v10.12.0 deprecation warnings. #825

Merged

bnoordhuis closed this Nov 26, 2018

jkrems deleted the feature/master/node-10-12-deprecations branch November 26, 2018 16:18

webgurucan added a commit to webgurucan/node-image-handler that referenced this pull request Mar 27, 2022

Add Node 11 to CI, experimental only, no prebuild

7e4a59b

Hide deprecation warnings - see nodejs/nan#811

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix deprecation warnings on node 10.12 #811

Fix deprecation warnings on node 10.12 #811

jkrems commented Oct 11, 2018 •

edited

Loading

jkrems commented Oct 11, 2018

jkrems commented Oct 11, 2018

kkoopa commented Oct 11, 2018

jkrems commented Oct 11, 2018

kkoopa commented Oct 11, 2018

addaleax commented Oct 11, 2018

jkrems commented Oct 11, 2018

Flarna commented Oct 11, 2018

addaleax commented Oct 11, 2018

Flarna commented Oct 11, 2018

bnoordhuis Oct 11, 2018

jkrems Oct 11, 2018

addaleax Oct 11, 2018

bnoordhuis Oct 11, 2018

refack commented Oct 11, 2018 •

edited

Loading

jkrems commented Oct 12, 2018 •

edited

Loading

jkrems commented Oct 12, 2018

refack commented Oct 12, 2018

Flarna Oct 22, 2018

jkrems Oct 22, 2018

Flarna Oct 22, 2018

jkrems Oct 22, 2018

Flarna Oct 22, 2018

kkoopa Oct 22, 2018

Flarna Oct 22, 2018

erikdubbelboer commented Nov 13, 2018

bnoordhuis commented Nov 26, 2018

Fix deprecation warnings on node 10.12 #811

Fix deprecation warnings on node 10.12 #811

Conversation

jkrems commented Oct 11, 2018 • edited Loading

jkrems commented Oct 11, 2018

jkrems commented Oct 11, 2018

kkoopa commented Oct 11, 2018

jkrems commented Oct 11, 2018

kkoopa commented Oct 11, 2018

addaleax commented Oct 11, 2018

jkrems commented Oct 11, 2018

Flarna commented Oct 11, 2018

addaleax commented Oct 11, 2018

Flarna commented Oct 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Oct 11, 2018 • edited Loading

jkrems commented Oct 12, 2018 • edited Loading

jkrems commented Oct 12, 2018

refack commented Oct 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikdubbelboer commented Nov 13, 2018

bnoordhuis commented Nov 26, 2018

jkrems commented Oct 11, 2018 •

edited

Loading

refack commented Oct 11, 2018 •

edited

Loading

jkrems commented Oct 12, 2018 •

edited

Loading