fix: check V8 version directly instead of inferring from NMV

[MarshallOfSound]

The changes introduced in d113c02 make the (historically correct) assumption that it's impossible for an older version of node (and therefore an older version of V8) to have a higher NODE_MODULE_VERSION.

As of Electron 4.1.0 however this is not the case, it embeds node 10.11 with v8 v6.9.427.24 but has the NMV of 69 which is higher than node 11's NMV of 68. As Electron continuously gets their own NMV numbers (refs nodejs/TSC#651) we can't make this assumption any more. Currently native modules are failing to build against Electron 4 because of this issue.

This PR updates these checks to check what actually matters here (the V8 version).

cc @kkoopa @Flarna

[Flarna]

Hmm, NodeJs 10.11.0 uses v8 6.8.275.32 (see https://github.com/nodejs/node/blob/v10.11.0/deps/v8/include/v8-version.h) so it seems electron uses something else then 10.11.0.

Is there any define available to use to distinguish between node.js and electron?

Node.JS updates v8 within a major release cycle a few times. Using checks for v8 versions makes it easy to get binaries not consistent across a major node.js version. Maybe a check on NODE_MAJOR_VERSION should be used here? I expect not as v8 version in electron differs from that v8 version in node.
So checks for NODE_MAJOR_VERSION and NODE_MODULE_VERSION should be avoided then...

[MarshallOfSound]

So checks for NODE_MAJOR_VERSION and NODE_MODULE_VERSION should be avoided then...

I believe so, my thinking here is if something truly depends on a V8 API being available we should probably be checking the V8 version as that strictly dictates the APIs available rather than inferring from the node version / NMV

Node.JS updates v8 within a major release cycle a few times. Using checks for v8 versions makes it easy to get binaries not consistent across a major node.js version.

Node major releases have to remain ABI compatible though right? If an API is removed inside V8 I don't think they could update to that V8 version inside a major release. I.e. It's an impossible situation for Node 12.0 to have a V8 API that 12.2 does not have. I'm not super familiar with how that works though so that's based only on my understanding as an observer.

EDIT:

so it seems electron uses something else then 10.11.0.

We shipped node 10.11 but using the V8 version that Chromium 69 shipped with. We are tied to the Chromium V8 version rather than the node V8 version. We try to match them but in some cases it's not possible

[Flarna]

Yes, major node versions are abi stable, not just for v8 also for libuv, openssl,... and the node module version is intended to represent an ABI version.

And yes, v8 can't remove an API. The solution chosen by node is to agree with v8 team on a release schedule and pull in ABI changes for future v8 versions into that one used in first major release. see nodejs/node#25082 if you are interested in more details in this.

[MarshallOfSound]

@Flarna I think we are OK using V8 versions here then, because any patches that node pulls in as extras on top of their V8 version we also float on top of our embedded version of node so they will be consistent here.

This definitely fixes the current issue, and moving forward it would probably make sense for nan to run some Electron versions during CI (I can set this up later if you're 👍 )

[kkoopa]

This definitely fixes the current issue, and moving forward it would probably make sense for nan to run some Electron versions during CI (I can set this up later if you're +1 )

Please do. This discrepancy between Node.js and Electron has been popping up for years.

[bnoordhuis]

FWIW, I don't think these checks are going to be very robust. Node.js can (and has) upgraded V8 in stable releases. Checking for V8. 7.0 breaks when Node.js upgrades to V8 7.1.

Node major releases have to remain ABI compatible though right? If an API is removed inside V8 I don't think they could update to that V8 version inside a major release.

It's usually possible to maintain API and ABI compatibility with only minor fixes to Node's fork of V8 and so that is what we do. We still bump the minor and/or major version number.

[Flarna]

Concrete NodeJs 10.0.0 used 6.6.346.24 and current 10.15.3 uses 6.8.275.32.
The change in this PR should be ok as NodeJs 11 started with 7.0.276.28 and uses now 7.0.276.38 so no change in minor version.

[fivdi]

A table with all the version number details from the past can be found at https://nodejs.org/en/download/releases/

[bnoordhuis]

For Node.js 11 it probably doesn't matter since it's short-lived but Node.js 12 (coming out next month) will be a LTS release and it's almost certainly going to see V8 upgrades. It'll ship V8 7.3 out of the gate.

I'm pretty sure NAN will emit warnings with that version after this PR because Value::BooleanValue(Local<Context>) const is deprecated, you're supposed to use the Isolate overload again.

[Flarna]

I think it works as the isolate version is used for all v8 versions >7.0 which matches to the previous check to Node.JS 11. I assume v11 will not get an V8 update anymore.
I fully agree that checks on v8 version are not the best choice in general.

I gave it a fast try with Node 12 nightly which uses 7.3 and the only warnings I get (from node and NAN) are because v8::PersistentBase<v8::Object>::IsNearDeath() has been deprecated. But this is not related to this change here.