Add NAPI support

[mmomtchev]

NAPI support is the future of JavaScript bindings - it is a coordinated effort across the industry to produce a universal interface for speaking to a JS engine. At the moment, it is still fully implemented only by Node.js/V8, but there is ongoing work to support it in Hermes.

NAPI modules present a number of advantages:

• Full binary compatibility across Node.js/V8 versions using only a plain C interface at the link level (node-addon-api is a set of C++ templates on top of the C API), meaning that binary modules can be published to npm
• Full multi-threading support
• Electron compatibility
• Much better abstractions than raw V8
• Full native C++ support - including member functions, exceptions and even inheritance (through a kludge of mine)

[mmomtchev]

This is not completely finished, it is a starting point for discussions. There are still a couple of unit tests that do not work (I have created separate issues for each one of them) and there is no testing infrastructure set up.

This support comes in two flavors: with or without exceptions. I see that testing is always done with exceptions enabled - maybe there should be separate testing run without exceptions - but one will have to identify the tests that do not work in this case - there are quite a few.

Also, the multi-threading is still not tested.

[ojwb]

BTW, there's an long-open issue requesting this: #1243

[ojwb]

Does it make sense to support compiling Node modules in two ways?

#1243 implies this was "experimental" before Node 10, but unless people are realistically still using older versions this seems an argument for bumping the minimum we aim to support.

[mmomtchev]

Alas, JavaScript has become infamous for its competing standards, modes, dialects and interfaces. After all, it is a language that grew from a quick hack to the most-widely used language in the decade after the web exploded. The problem is recognized by the community, so each time there is a new standard, they try very hard for it to be the very last one. N-API has been very successful in this aspect without a single compatibility-breaking change since it was introduced in 2017.

[ojwb]

I think I didn't make my point clearly.

With NAPI support is there a good reason to keep supporting compiling node modules without NAPI?

We already support javascript variants for V8 without node, node and jsc and people are working on others (#2334). That increase maintenance effort, and the manual testing required before pushing any change. We also really need CI testing for variants or else we can't have any confidence they'll continue to work, and the time CI takes to run is already sometimes a problem.

It's possible this would mean dropping support for the oldest version(s) on node we currently claim to support, but we need to review the minimum supported versions periodically anyway. I suspect those versions are simply not used by anyone at this point (I'm not in the node community, but my impression was that in the earlier days of node everyone was upgrading to new versions almost eagerly). https://en.wikipedia.org/wiki/Node.js says they're all years past upstream end-of-life, though that's not always the best criteria since for some target languages distros continue to maintain them well after upstream stops.

[ojwb]

I've just finished off and merged #2455 which looks like it overlaps a bit with the changes here.

The key points are that now:

(a) C++ output is turned on by default for node (like happened already for v8-without-node)
(b) hacks in the testsuite and examples to workaround that omission are gone
(c) The engine to use if you don't specify ENGINE is selected based on what configure found

Sorry about the overlap but overall this should make things easier. Let me know if you need help updating for it.

[mmomtchev]

I plan to use this to create good Node.js bindings for ImageMagick-7 - which is a very good example, since it is a very important library that has been absent from the Node.js ecosystem because of its sheer size and complexity despite the demand for it - evident from the number of ersatz packages on npm which are simply wrappers around the CLI tools. This will allow me to ensure the usability of the NAPI support.

[wsfulton]

Hi @mmontchev. I don't really understand the Javascript space, but seems like this could be very useful once complete.

In the meantime, could you please make your next commit to this pull request one where all the napi Javascript tests pass on CI? Meanwhile you can test the GHA CI on your own account. I also suggest you modify ci.yml to only run Javascript while you develop it and fix the GHA CI. We discourage large scale development like this on branches used for pull requests because SWIG uses a lot of compute resource for each CI run.

[mmomtchev]

@wsfulton I am focusing on making a publishable module out of https://github.com/mmomtchev/node-magickwand - because this is the real NAPI support test - and then I will push through the various PRs / add some documentation
Is 4.2.0 already planned?

[mmomtchev]

@wsfulton @ojwb This is ready for review and ready to be used
There is already an npm module based on it: https://www.npmjs.com/package/node-magickwand

Multithreading/async is still missing and it is coming in a separate chunk

[mmomtchev]

@ojwb @wsfulton I am struggling with the napi.h includes - I finally settled on test-suite/javascript/node_template - the same directory which contains the binding.gyp template. However I cannot find any variable in the test suite makefile that references the main source directory when doing an out-of-source-tree build.

Normally, this file is to be included in the node addon sources. It is not part of the main Node.js distribution and it is a set of templates. Most actual addons will install it through npm - in which case it would be integrated into the gyp build system. There is no other official distribution besides npm - downloading a source tarball is the only other option.

[ojwb]

However I cannot find any variable in the test suite makefile that references the main source directory when doing an out-of-source-tree build.

Isn't that $(srcdir)?

[mmomtchev]

However I cannot find any variable in the test suite makefile that references the main source directory when doing an out-of-source-tree build.

Isn't that $(srcdir)?

Yes, but only in the beginning, when setting up the environment, during the build it gets overwritten with the out-of-source directory where the tests are copied one by one. This was creating the confusion, now I am getting the value in the beginning.

[ojwb]

Otherwise, this can go through, but #1957 will have to adapt

@mmomtchev I think you must have the wrong patch number there as 1957 was merged over two years ago...

[wsfulton]

The prerequisites for merging into master as described in Doc/Manual/Extending.html#Extending_prerequisites seem to be met except for the html documentation. Can you please add some napi specific documentation to Doc/Manual/Javascript.html then these will be met. I've run the beautify to make a few minor changes, which I can commit.

When ready, I propose to merge the branch without squashing... are you happy with that or do you wish to do a bit of history rewrite via rebase first? If you do want to rebase, please let me know because I can't handle a rebase after starting the code review!

I noticed a bug in the node implementation when running the class example (not necessary to fix unless you feel inspired). The output with node is

Square instanceof Shape:  false

but is correct with napi:

Square instanceof Shape:  true

[wsfulton]

I've reviewed the patch and it seems quite simple and fits in nicely to the current architecture by simply providing a class derived from JSEmitter in javascript.cxx. While there is quite a lot of copy/paste of files in Lib/javascript/napi from Lib/javascript/v8 that could be addressed, it does seem to be the way the v8 vs jsc was first implemented and not an unreasonable approach. Nice to see the code easily expanding to a new variant of Javascript. Well done to all the Javascript authors!

Here are my comments that need addressing for the patch:

1. Removal of smart_pointer_static from Examples/test-suite/common.mk prevents testing in all languages, can you look at a temporary removal for just NAPI instead?
2. I'd have thought the keywords would be the same across all Javascript implementations, hence, shouldn't
   Lib/javascript/napi/javascriptkw.swg and Lib/javascript/v8/javascriptkw.swg be identical?
3. The Examples/test-suite/javascript/node_template/napi*.h files added... aren't the napi header files installable from somewhere else in some way??
4. According to https://nodejs.org/api/n-api.html#node-api, the headers to use are node_api.h, but napi.h is being used instead. Maybe a minimum version of Node is required before node_api.h was provided?
5. N-API seems to be renamed Node-API. Is this something that should be corrected in the generated code, so use node_api.h instead of napi.h copied into SWIG?
6. Why does .gitignore ignore Examples/test-suite/javascript/node_template? I can't see any reason for this having run the examples and test-suite.

[mmomtchev]

There are two interfaces - Node-API (formerly N-API) which is a plain C API that is part of the core distribution of Node.js. It is defined in node_api.h and it is always installed along Node.js itself, usually in /usr/include/node or /usr/local/include/node.

node-addon-api is a set of C++ templates (only header files) that use Node-API (formely N-API). These are also published by the Node.js team, but are not part of the Node.js distribution. They are available as an NPM package. As most binary Node.js addons are also NPM packages, they simply have to include node-addon-api as a dependency. This also applies to binary addons that are generated by SWIG - node-magickwand includes it. Normally, it is expected that these templates will be part of the client application.

One of the main reasons for this separation is that this way binary addons use only the well-defined plain C linking which works very well across different compilers. This allows a Node.js addon publisher to distribute a binary on NPM which will work with all Node.js binaries that an end-user might have - without regard if one has been built with the g++ runtime, or the clang runtime, or the MSVC runtime.

When it comes to SWIG, these files are only needed when building the examples and test suite. I did hesitate a lot what to do with these and I ended placing them in node_template along the node-gyp templates. Another option would be to always install them from NPM.

[wsfulton]

So to summarise, if I have understood correctly, the generated code is actually targeting node-addon-api, which in turn uses Node-API (formerly N-API). node-addon-api is provided in napi.h and Node-API is provided in node_api.h. Somewhat confusing... this must definitely go in the html docs! Adding to the confusion, the term for the new SWIG Javascript backend is NAPI and I'm still not sure if this is the old name of Node-API or just the new name for node-addon-api now!

As SWIG is actually targeting node-addon-api, users must install this package in order to successfully use the generated code in their own projects. The examples and test-suite should also use the installed node-addon-api npm package as the tests/examples are meant to reflect testing a user's actual 'Javascript environment'. The CI should install the npm package too when testing. Note that Tools/CI-linux-install.sh already uses npm to install node-gyp, so there is a convenient location and precedence for using npm already. configure.sh should only enable testing if napi.h is found. Removing the napi.h versions in Examples/test-suite/javascript/node_template will also ensure SWIG is testing new versions as they are made available

[mmomtchev]

1. Removal of smart_pointer_static from Examples/test-suite/common.mk prevents testing in all languages, can you look at a temporary removal for just NAPI instead?

It is declared as broken only for JS now. However, in reality, this test produces invalid code for most target languages. It is part of the larger problem of having a shared namespace for static and non-static member functions.

2. I'd have thought the keywords would be the same across all Javascript implementations

Indeed, I moved this file out to the Javascript root. The current list includes the union of all words that cannot be used as identifiers in ECMAScript 4 to 6, both in sloppy and in strict mode. The list has had a few new items since it was last updated.

3. The Examples/test-suite/javascript/node_template/napi*.h files added... aren't the napi header files installable from somewhere else in some way??

node-addon-api is now installed during CI

I will wrap up the documentation and this should be ready. The current implementation works out of the box with the yeoman skeleton generator:

npm install -g yo
npm install -g generator-napi-module
mkdir hello-world
cd hello-world
yo napi-module

[mmomtchev]

@wsfulton I think to have addressed all the review suggestions and I updated the documentation section.

As the PR is very sizable and contains a very large number of separate commits, it is natural to squash it to a single commit message in order to keep the blame log tidy and readable. Normally, when squashing, GitHub takes automatically care of attributing the changes to their respective authors. If someone manually merges from the git CLI, the author must be specified with git commit --author <pattern>. <pattern> is then searched in the git history and the last committer matching the pattern is automatically replaced. If not, <pattern> must be a full git identifier such as A U Thor <author@address.com>. This replicates the GitHub behavior where the committer and the author appear separately in the history - the committer being the one specified by git config user.name.

[wsfulton]

• There are a couple of minor points to be addressed which I've added as review comments.
• The initial merge request comments about the advantages should be in the docs. I'd encourage you to add any other interesting aspects regarding the JavaScript wrappers.
• Can you add an appropriate detailed README entry for users.

When these minor points are addressed, I'll squash and merge. Author attribution is important, don't worry, I'll get that right. Can you please also add your name into the list of major contributors / authors in the COPYRIGHT file.

[mmomtchev]

• Can you add an appropriate detailed README entry for users.

@wsfulton, which README are you referring to?

[wsfulton]

@wsfulton, which README are you referring to?

Sorry, my bad, I meant the CHANGES.current file in the root directory.

[wsfulton]

Thanks @mmomtchev, it's been a pleasure reviewing this. Thanks for your prompt responses and getting this patch into a state that fits very well with what is expected for a major contribution to SWIG.

[dot-asm]

Reference to "automatic multi-threading" is misleading. NAPI effectively forces modules to be "context-aware," but it doesn't make the generated code MT-safe. More correct formulation would probably be "provides Javascript Workers-aware environment for module developer to utilize."

For reference, MT-safety has multiple meanings in the context. One is "having multiple module threads interact with the same context," another is "having multiple Javascript Workers interact with the module." NAPI makes the former usable. And in the context of the latter it facilitates per-Worker state maintenance. But both are on the module developer to implement, not on SWIG. In other words there is nothing "automatic" about it, hence the objection. Just in case, it's exactly as "automatic" as #1973. The tests that pass there simply don't have global mutable states, which makes them MT-safe "naturally."

[wsfulton]

Could you post a patch here or a new github issue and I'll get this committed. Thanks.