Enable V8 deprecation warnings for native modules

[matthewloring]

It will be helpful for native module developers to be aware of any
deprecated apis they are using so that they can update before their
modules break completely. Module developers can override this option
in their binding.gyp if they do not want to see these warnings.

[rvagg]

I'm +1 on this, whatchy'all think @nodejs/addon-api & @nodejs/v8?

[jeisinger]

why not add a block

'variables': {
'v8_deprecation_warnings%': 1,
}

instead of setting the macro directly?

[bnoordhuis]

I'm +1 too but I think it's important to first make sure that node v4-v6 (by which I mean node.h and friends) and nan are V8_DEPRECATION_WARNINGS-clean before releasing it into the wild.

(Node.js is compiled with deprecation warnings enabled but that doesn't extend to files that aren't used by core itself, like src/node_object_wrap.h.)

[bnoordhuis]

@jeisinger V8's *.gypi files aren't included when building add-ons, only node's common.gypi and config.gypi.

[jeisinger]

fair enough, +1 then to your proposal

[kkoopa]

-1, it will unleash an endless stream of useless issues and headache on every level of the dependency chain. It is harder to find an addon that does not use deprecated APIs than one that does not.

[matthewloring]

More issues should alert module authors to upgrade before things start breaking. This is especially important given how widespread the dependencies on deprecated APIs are.

[matthewloring]

Addressing nan deprecations here: nodejs/nan#568

[matthewloring]

nodejs/nan#568 and nodejs/nan#569 should clear up the nan deprecation warnings once they land.

[matthewloring]

nodejs/nan#568 and nodejs/nan#569 have landed and I didn't observe any deprecation warnings when compiling src/node_object_wrap.h. Are there any other files that aren't used by core and should be checked explicitly?

[bnoordhuis]

Landed in 15fd56b, thanks. There are one or two more PRs that I'd like to land before putting out a new release.

[rvagg]

Apparently this prints stack traces for users now, which is likely to get us (and addon authors) a lot of needless attention.

e.g. #991 coming from:

 base::OS::PrintError(
"(node) v8::%sTemplate::Set() with non-primitive values is deprecated\n"
"(node) and will stop working in the next major release.\n",
templ->IsFunctionTemplateInfo() ? "Function" : "Object");
isolate->PrintStack(stderr, i::Isolate::kPrintStackConcise);
base::DumpBacktrace();

in deps/v8/src/api.cc

[bnoordhuis]

@rvagg No, the warning is emitted by a V8 patch that we're floating in node.

[rvagg]

@bnoordhuis is there a more minimal thing we can print cause looking at the output in #991 it's pretty dramatic.

[bnoordhuis]

That's kind of the point. It's a strong incentive to update and it helps with finding the offending call site. If it just printed a warning without a stack trace, you wouldn't know from what dependency it originates.

(I say this as someone who was less than amused by the flood of bogus bug reports he got about a deprecation warning in a dependency. Never again.)

[ofrobots]

Apparently this prints stack traces for users now, which is likely to get us (and addon authors) a lot of needless attention.

Note that this issue is about compile time deprecation warnings whereas #991 is a runtime warning. This doesn't print a stack trace.

[rvagg]

ok, I stand corrected, missed that one obviously!