N-API: Remove reference to node internals

[gabrielschulhof]

We must keep src/node_api.cc free of node internals because it must
build cleanly outside of node as well - for example in node_addon_api.

Checklist

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

Affected core subsystem(s)

[mhdawson]

LGTM

[mhdawson]

CI run: https://ci.nodejs.org/job/node-test-pull-request/8830/

[digitalinfinity]

Super-nitpicky- consider pointing to node-addon-api as an example here

[digitalinfinity]

Is there a way to validate this in CI somehow so it doesn't break in the future? I'm not sure of the best way other than having some bizarre CI job that clones node-addon-api, replaces node_api.cc in it and then runs the node-addon-api test- surely there's a better way?

[mhdawson]

@digitalinfinity thats probably a good idea to try for ci validation. We should likely open a separate issue for that. It would not necessarily need node-addon-api, unless it provides some elements needed to compile node_api.cc

[bnoordhuis]

If it's going to live in the node source tree, it should be idiomatic code.

If you want to make it build outside of the tree, provide facsimiles in out-of-tree builds.

On that topic: the use of assert() should be replaced by CHECK macros.

[mhdawson]

@bnoordhuis I'm not sure being quite that black/white makes sense to me in this case. It's a small delta in order to avoid extra work to keep a separate copy which then needs to be kept in sync. Is it really that big a deal in this case ?

[gabrielschulhof]

@bnoordhuis especially since we're only talking about no more than one reference to node::arraysize()

…

On Wed, Jun 28, 2017 at 10:21 PM, Michael Dawson ***@***.***> wrote: @bnoordhuis <https://github.com/bnoordhuis> I'm not sure being quite that black/white makes sense to me in this case. It's a small delta in order to avoid extra work to keep a separate copy which then needs to be kept in sync. Is it really that big a deal in this case ? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#13892 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA7k0YnVmRZIWGnEqFm6CZ1xC_xtX2Szks5sIqfUgaJpZM4OEAu5> .

[bnoordhuis]

I'm not sympathetic to the "it's more work" argument, that's the price of upstreaming. The out-of-tree project will become irrelevant over time whereas the in-tree version will stay around for many years to come.

we're only talking about no more than one reference to node::arraysize()

Also that calls to assert() should be replaced by CHECK macros, and I'm sure there is more. And if not now, then future refactorings are sure to introduce them.

n-api gets no special treatment. That's demanding everyone else do more work in order that the n-api people can get away with doing less.

[bnoordhuis]

After seeing how #13971 copy/pastes whole functions from node.cc to node_api.cc I'm even more convinced the rules for n-api need to be tightened, not loosened. It's an unacceptable lowering of the code quality.

[gabrielschulhof]

We've decided to use our own version of node_internals.h in node-addon-api along with possibly additional source files, in which we grab whatever we need from node. This should allow us to leave node_api.cc identical between the two repos.