n-api: reimplement instanceof using V8 API

[hashseed]

This uses the v8::Value::InstanceOf API exposed in V8 6.0 and should be performance neutral, if not improve it. By using V8's API, there is no longer the need to cache Symbol.hasInstance.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

N-API

[hashseed]

@addaleax @TimothyGu do you mind taking a look?

[addaleax]

@nodejs/n-api This is a patch that won’t be applicable to previous versions of Node.js. I think that’s okay, at some point that would happen anyway, it just means that updating other copies can’t be done through verbatim copies anymore.

[hashseed]

To be fair src/node_api.cc has existed only since March 2017. Neither Node 6 nor Node 7 has it.

[gabrielschulhof]

Might we be able to wing it with node_internals.h?

[gabrielschulhof]

They have it, just not as part of the tree. src/node_api.cc *is* being built against both as part of node-addon-api. We've expected though that we'd eventually have to do without verbatim copying.

…

On Wed, Oct 11, 2017 at 4:09 PM, Yang Guo ***@***.***> wrote: To be fair src/node_api.cc has existed only since March 2017. Neither Node 6 nor Node 7 has it. — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#16143 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA7k0dxwtMmE_zaQgRLLEYgaOTbzoanbks5srL4CgaJpZM4P1Xos> .

[addaleax]

@gabrielschulhof Sorry, I don’t quite follow … how would using node_internals.h make a difference here?

[gabrielschulhof]

We might be able to do something like https://github.com/nodejs/node-addon-api/blob/master/src/node_internals.h#L79 - although if InstanceOf is an instance method rather than a static one, it's nigh-on impossible to avoid introducing a delta.

…

On Wed, Oct 11, 2017 at 4:21 PM, Anna Henningsen ***@***.***> wrote: @gabrielschulhof <https://github.com/gabrielschulhof> Sorry, I don’t quite follow … how would using node_internals.h make a difference here? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16143 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA7k0YwuKVhyZEfDQ31KOGNv_QArEPaKks5srMDxgaJpZM4P1Xos> .

[addaleax]

Ah, I see – yeah, I wouldn’t bother. Just applying patches individually to the node-addon-api copy should be fine.

[hashseed]

I feel like I'm missing some requirements. Why does it matter whether this patch applies to older versions? What copies are there outside of node core? What role does node_internals.h play?

My impression is that node_api.cc is supposed to be the glue between a stable API and V8. As such, it would change as V8 does. I feel like I'm mistaken?

[gabrielschulhof]

You're not mistaken. We have a copy of src/node_api.cc in https://github.com/nodejs/node-addon-api/blob/master/src/node_api.cc. That package claims to support all versions of node between 4.0.0 (inclusive) and the latest. So far we've gotten away with having a verbatim copy of src/node_api.cc in that package, however, some things like v8::Private and FunctionCallbackInfo::NewTarget are unavailable on older versions, so we've monkey-patched those via node_internals.{h,cc}, available in the same repo. However, the two copies of src/node_api.cc are inexorably (and justifiably) drifting apart so, sooner or later we'll have to introduce a delta.

…

On Wed, Oct 11, 2017 at 5:45 PM, Yang Guo ***@***.***> wrote: I feel like I'm missing some requirements. Why does it matter whether this patch applies to older versions? What copies are there outside of node core? What role does node_internals.h play? My impression is that the node_api.cc is supposed to be the glue between a stable API and V8. As such, it would change as V8 does. I feel like I'm mistaken? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16143 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA7k0eyaTgskv2BouGZ9GI9p90w2R5C6ks5srNSUgaJpZM4P1Xos> .

[hashseed]

I see. Thanks for the explanation. Wouldn't you be better served to simply keep a new copy of src/node_api.cc for every V8 version in node-addon-api and ifdef between them?

[hashseed]

Somebody merge this?

[addaleax]

CI: https://ci.nodejs.org/job/node-test-commit/13136/

[hashseed]

Is CI stuck somehow? Looks like every one of these checks finished with SUCCESS, except for windows-fanned, which finished with UNSTABLE.

[TimothyGu]

@hashseed The GitHub integration for CI stopped working a while ago.

[fhinkel]

Thanks! Landed in c81fd7c

[hashseed]

Awesome. Thanks!