broken on Node master

[mcollina]

I think the goal is to have it in core by Node v12, right? So possibly this is a won't fix.

[richardlau]

I had started looking at this but am caught up with end of year red tape at work. One of the things we need to figure out is if fixing this requires us to drop support for older LTS releases of Node.js and thus require a semver major bump. (In the case that it does, it should be fine to drop support for Node.js 6 in the semver major as it will be EOL when 12 is released).

[richardlau]

In theory this is fixed in the PR to merge node-report into core, but that PR has quite significant changes that it wasn't obvious at first glance what needs to change here.

[richardlau]

For reference, here's a build.log from the node-report CI job (https://ci.nodejs.org/job/nodereport-continuous-integration/259/) showing the compilation failures: https://gist.github.com/richardlau/e8728b278f5ce53ec002cf46d6af012d

[richardlau]

Some of the errors are from nan:

e.g.

12:10:21 In file included from ../node_modules/nan/nan_new.h:189:0,
12:10:21                  from ../node_modules/nan/nan.h:223,
12:10:21                  from ../src/node_report.h:4,
12:10:21                  from ../src/utilities.cc:1:
12:10:21 ../node_modules/nan/nan_implementation_12_inl.h: In static member function ‘static Nan::imp::FactoryBase<v8::StringObject>::return_t Nan::imp::Factory<v8::StringObject>::New(v8::Local<v8::String>)’:
12:10:21 ../node_modules/nan/nan_implementation_12_inl.h:356:37: error: no matching function for call to ‘v8::StringObject::New(v8::Local<v8::String>&)’
12:10:21 return v8::StringObject::New(value).As<v8::StringObject>();

others, e.g.

12:10:21 ../src/node_report.cc: In function ‘void nodereport::PrintJavaScriptErrorStack(std::ostream&, v8::Isolate*, v8::MaybeLocal<v8::Value>)’:
12:10:21 ../src/node_report.cc:512:52: error: no matching function for call to ‘v8::StackTrace::GetFrame(int&)’
12:10:21 PrintStackFrame(out, isolate, stack->GetFrame(i), i, nullptr);

are fixable, but the new function (that takes an isolate parameter) doesn't exist in neither Node.js 6 nor 8.

[richardlau]

I'm comfortable in a semver major dropping support for Node.js 6 to support Node.js 12 as it will become EOL when 12 is released but Node.js 12 overlaps Node.js 8.

Hopefully Node.js 12 will include equivalent functionality to this module, and we'd just redirect people coming here to use the built-in functions and just keep this module around to support older still supported Node.js releases.

[richardlau]

nodejs/nan#831 fixes the nan errors (it's in nan's master branch but not in a release). Until there is a new release of nan, node-report will be broken when compiling against Node.js master (12.0.0-pre).

For the non-nan errors I guess we can attempt to narrow down the V8 versions the replacement function was introduced in and have ifdef's (or maybe be less accurate and gate on the V8 versions in Node.js 6/8/10).

[richardlau]

I've opened #119 to fix the errors that originate from this project. To fully resolve we need a nan release containing nodejs/nan#831.

[MylesBorins]

Looks like this is the PR that broke node-report

nodejs/node#24861

Just backed it out of 11.x until the nan fix lands

[richardlau]

There's a new release of nan which contains nodejs/nan#831. I missed one deprecated method, but once that's fixed we should be good: #124