upgrade to Node 12

[rwalle61]

(These changes break Node 6. See below)

Fix for Node 12:

General information about the V8 API changes between Node 10 and 12

• Summary https://docs.google.com/document/d/1g8JFi8T_oAE_7uAri7Njtig7fKaPDfotU6huOa1alds/edit
• "v8/node deprecated APIs and how to handle them" https://github.com/bcoin-org/bcrypto/issues/7

These fixes frequently involve the following:

• the isolate object - get via Isolate* isolate = v8::Isolate::GetCurrent()
• the context object - get via Local<Context> context = Nan::GetCurrentContext()
  • https://github.com/nodejs/nan/blob/master/doc/v8_misc.md#nangetcurrentcontext
• converting MaybeLocal types returned by many Nan functions to Local using maybeLocalObj.ToLocalChecked()

Specific error fixes and my specific sources of information

• from s->WriteUtf8(buf) to s->WriteUtf8(isolate, buf)
  • https://github.com/nodejs/node/pull/22531/files#diff-f77b9fed8a3d7f7899ae2316e2bb3230R24
• from X->ToString() to Nan::To<String>(X).ToLocalChecked()
  • https://github.com/astro/node-expat/pull/196/files#diff-2b6f822b34553a6cadd4627d4dc9e84fR29
• from X->ToObject() to Nan::To<Object>(X).ToLocalChecked()
  • https://github.com/astro/node-expat/pull/196/files#diff-2b6f822b34553a6cadd4627d4dc9e84fR29
• from X->ToInteger() to Nan::To<Integer>(X).ToLocalChecked()
  • following the pattern of the above
  • see also https://github.com/nodejs/nan/blob/master/test/cpp/converters.cpp#L37
• from X->GetOwnPropertyNames() to Nan::GetOwnPropertyNames(cache).ToLocalChecked()
  • https://github.com/nodejs/nan/blob/master/doc/maybe_types.md#api_nan_get_own_property_names
  • or, to X->GetOwnPropertyNames(context).ToLocalChecked()
    • https://stackoverflow.com/questions/40533193/v8-c-how-to-get-object-key-values-provided-as-arguments
    • https://v8docs.nodesource.com/node-8.11/db/d85/classv8_1_1_object.html#a79a6e4d66049b9aa648ed4dfdb23e6eb
• from X->GetFunction() to X->GetFunction(context).ToLocalChecked()
  • https://www.nearform.com/page/7/?cat=-1
  • https://nodejs.org/api/addons.html
  • see alternative https://github.com/astro/node-expat/pull/196/files#diff-2b6f822b34553a6cadd4627d4dc9e84fR29
• from String::Utf8Value X(Y) to Nan::Utf8Value X(Y)
  • https://github.com/nodejs/nan/blob/master/doc/v8_misc.md#api_nan_utf8_string
  • https://github.com/microsoft/node-pty/pull/288/files#diff-8fc148e2d2217506b0e485a3033310feR174
  • https://v8docs.nodesource.com/node-8.11/d4/d1b/classv8_1_1_string_1_1_utf8_value.html
• from X->Int32Value() to X->Int32Value(context).FromJust()
  • https://github.com/microsoft/node-pty/pull/288/files#diff-8fc148e2d2217506b0e485a3033310feR174
• from X->IntegerValue() to X->IntegerValue(context).FromJust()
  • https://github.com/microsoft/node-pty/pull/288/files#diff-32a8618c58cb849e9b5535ba19ffacc5R186
• from kApiPointerSize to kApiSystemPointerSize
  • https://github.com/v8/v8/commit/81a654e7a0d8dc86896dd94773ad839b6ffc2383#diff-4637d1e4b9c1cbab90cbfd580aceff15R29
• from isolate->GetCpuProfiler() to ... (larger fix, because whereas the V8 engine previously had just 1 CpuProfiler, it now has multiple)
  • https://github.com/nodejs/node/pull/18959/files
  • https://v8docs.nodesource.com/node-10.6/d2/d34/classv8_1_1_cpu_profiler.html
• from const Handle<String> X to const Local<String> X
  • https://electronjs.org/blog/nodejs-native-addons-and-electron-5
  • https://github.com/microsoft/node-pty/pull/288/files#diff-8fc148e2d2217506b0e485a3033310feR174
• update node-gyp from v3 to v4
  • https://github.com/nodejs/node-gyp/issues/792
  • tbh I don't know in detail how this solves the problem

Specific deprecation warning fixes and my specific sources of information

• from X->Get(Y) to Nan::Get(X, Y).ToLocalChecked()

  • https://github.com/nodejs/nan/blob/master/doc/maybe_types.md#nanget

• from X->Set(Y, Z) to Nan::SetMethod(X, Y2, Z2)

  • (this is for for setting methods. To convert other forms of X->Set(..) to their Nan equivalents, see Nan docs https://github.com/nodejs/nan/blob/master/doc/methods.md#nansetprototypemethod and example https://github.com/astro/node-expat/pull/196/files#diff-2b6f822b34553a6cadd4627d4dc9e84fR29)
  • Nan docs https://github.com/nodejs/nan/blob/master/doc/methods.md#api_nan_set_method
  • at https://github.com/RuntimeTools/appmetrics/pull/576/files#diff-2419e406d80ff8bfd9b4fc942cc6b491R741, we are using NAN_METHOD macros https://github.com/RuntimeTools/appmetrics/pull/576/files#diff-2419e406d80ff8bfd9b4fc942cc6b491R358

Notes

• other forms of 'X->Set' will soon be deprecated - we should update our code

These changes break Node 6:

Problem:

Error output from `npm i` on Node 6

``` /Users/richard.waller@ibm.com/.node-gyp/6.17.1/include/node/v8.h:21:10: fatal error: 'utility' file not found #include ^~~~~~~~~ 1 warning and 1 error generated. make: *** [Release/obj.target/appmetrics/geni/appmetrics.o] Error 1 ```

This can be resolved simply following https://github.com/nodejs/node-gyp/issues/1564 and https://github.com/nodejs/node-gyp/issues/1574. However, that reveals that my upgrade to Node 12 is incompatible with Node 6:

Error output from `CXXFLAGS="-mmacosx-version-min=10.9" LDFLAGS="-mmacosx-version-min=10.9" npm i` on Node 6

``` In file included from ../src/plugins/node/prof/watchdog.h:21: ../src/plugins/node/prof/compat-inl.h:308:38: error: no member named 'New' in 'v8::CpuProfiler' cpu_profiler_ = v8::CpuProfiler::New(isolate); ~~~~~~~~~~~~~~~~~^ Release/obj.target/appmetrics/geni/appmetrics.cpp:467:29: warning: 'Call' is deprecated [-Wdeprecated-declarations] listener->callback->Call(argc, argv); ^ ../node_modules/nan/nan.h:1673:3: note: 'Call' has been explicitly marked deprecated here NAN_DEPRECATED inline v8::Local ^ ../node_modules/nan/nan.h:103:40: note: expanded from macro 'NAN_DEPRECATED' # define NAN_DEPRECATED __attribute__((deprecated)) ^ Release/obj.target/appmetrics/geni/appmetrics.cpp:530:27: error: no matching constructor for initialization of 'String::Utf8Value' String::Utf8Value str(isolate, Nan::To(info[0]).ToLocalChecked()); ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /Users/richard.waller@ibm.com/.node-gyp/6.17.1/include/node/v8.h:2428:14: note: candidate constructor not viable: requires single argument 'obj', but 2 arguments were provided explicit Utf8Value(Local obj); ^ /Users/richard.waller@ibm.com/.node-gyp/6.17.1/include/node/v8.h:2438:5: note: candidate constructor not viable: requires 1 argument, but 2 were provided Utf8Value(const Utf8Value&); ^ Release/obj.target/appmetrics/geni/appmetrics.cpp:542:27: error: no matching constructor for initialization of 'String::Utf8Value' String::Utf8Value str(isolate, Nan::To(info[1]).ToLocalChecked()); ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /Users/richard.waller@ibm.com/.node-gyp/6.17.1/include/node/v8.h:2428:14: note: candidate constructor not viable: requires single argument 'obj', but 2 arguments were provided explicit Utf8Value(Local obj); ^ /Users/richard.waller@ibm.com/.node-gyp/6.17.1/include/node/v8.h:2438:5: note: candidate constructor not viable: requires 1 argument, but 2 were provided Utf8Value(const Utf8Value&); ^ Release/obj.target/appmetrics/geni/appmetrics.cpp:570:27: error: no matching constructor for initialization of 'String::Utf8Value' String::Utf8Value topicArg(isolate, Nan::To(info[0]).ToLocalChecked()); ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /Users/richard.waller@ibm.com/.node-gyp/6.17.1/include/node/v8.h:2428:14: note: candidate constructor not viable: requires single argument 'obj', but 2 arguments were provided explicit Utf8Value(Local obj); ^ /Users/richard.waller@ibm.com/.node-gyp/6.17.1/include/node/v8.h:2438:5: note: candidate constructor not viable: requires 1 argument, but 2 were provided Utf8Value(const Utf8Value&); ^ Release/obj.target/appmetrics/geni/appmetrics.cpp:572:27: error: no matching constructor for initialization of 'String::Utf8Value' String::Utf8Value commandArg(isolate, Nan::To(info[1]).ToLocalChecked()); ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /Users/richard.waller@ibm.com/.node-gyp/6.17.1/include/node/v8.h:2428:14: note: candidate constructor not viable: requires single argument 'obj', but 2 arguments were provided explicit Utf8Value(Local obj); ^ /Users/richard.waller@ibm.com/.node-gyp/6.17.1/include/node/v8.h:2438:5: note: candidate constructor not viable: requires 1 argument, but 2 were provided Utf8Value(const Utf8Value&); ^ 1 warning and 5 errors generated. make: *** [Release/obj.target/appmetrics/geni/appmetrics.o] Error 1 ```

Solution 1

Drop Node 6. This PR achieves this.

Solution 2

Support Node 6 alongside 8, 10, and 12. We would need to spend considerable time converting probably ~60% of the fixes in this PR to use NAN and if blocks.

Ideal long-term solution

Rewrite much of appmetrics using N-API, but that is beyond the scope of this issue.

[AndyRWatson]

I think option 1 is ok given that Node 6 is no longer in LTS. Agree that a longer term plan to support N-Api would be the right one to avoid issues in the future. We should document that Node 6 is no longer supported and the last known working appmetrics level

[sam-github]

Dropping 6.x support sounds like a good idea to me, but it requires bumping the appmetrics major (because dropping 6 support is a breaking change for people using node 6). That's NBD, but don't forget to do the book keeping: it eans finding all the other runtimes packages (prometheus, etc) that depend on appmetrics, and publishing an updated major of them, that depends on the new appmetrics major.

[tobespc]

The issues raised in the comments will be dealt with as separate PR's

[rwalle61]

Travis problem

Build intermittently failing on Windows (various versions of Node) with seemingly the same error in a test in http-outbound-probe-test.js.

Error output:

``` [Thu May 30 09:47:02 2019] com.ibm.diagnostics.healthcenter.mqtt INFO: Connecting to broker localhost:1883 Error: connect ECONNREFUSED 127.0.0.1:49860 at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1191:14) { name: 'TAP', autoend: true, errno: 'ECONNREFUSED', code: 'ECONNREFUSED', syscall: 'connect', address: '127.0.0.1', port: 49860, tapCaught: 'uncaughtException', test: 'TAP' } Error: socket hang up at createHangUpError (_http_client.js:342:15) at Socket.socketCloseListener (_http_client.js:377:23) at emitOne (events.js:121:20) at Socket.emit (events.js:211:7) at TCP._handle.close [as _onclose] (net.js:561:12) { name: 'TAP', autoend: true, code: 'ECONNRESET', tapCaught: 'uncaughtException', test: 'TAP' } # time=92.67ms not ok 3 - tests/probes/http-outbound-probe-test.js # time=5743.445ms --- timeout: 120000 file: tests/probes/http-outbound-probe-test.js childId: 2 command: 'C:\ProgramData\nvs\node\8.16.0\x64\node.exe' args: - '-r' - 'C:\Users\travis\build\RuntimeTools\appmetrics\node_modules\esm\esm.js' - tests/probes/http-outbound-probe-test.js stdio: - 0 - pipe - 2 cwd: 'C:\Users\travis\build\RuntimeTools\appmetrics' exitCode: 1 ... ```

Solution

• On Travis, Appmetrics passes tests ~60% time on Windows with Node 8, 10, and 12.
• We have manually verified that Appmetrics passes tests on Windows with Node 8, 10, and 12.
• Travis says "Windows builds are in early access stage"
• other people have reported Travis errors due to intermittent network issues https://travis-ci.community/t/nvs-add-6-download-error/3050

Therefore, we will keep running the Windows tests on Travis but flag them as allowed to fail.

• That way we will at least be able to monitor whether the Windows tests pass more consistently when Windows builds are no longer just early access.
• If the tests still fail intermittently at that point, then we should investigate these failures further

[rwalle61]

I've updated the docs, so this PR also resolves https://github.com/RuntimeTools/appmetrics/issues/571