Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify Project Organization #282

Closed
webern opened this issue Jun 17, 2018 · 16 comments
Closed

Clarify Project Organization #282

webern opened this issue Jun 17, 2018 · 16 comments

Comments

@webern
Copy link

webern commented Jun 17, 2018

I spend a lot of time trying to find things when working with the napi.h API.

Finding things is quite confusing because there is a website with (outdated?) Doxygen documentation here https://nodejs.github.io/node-addon-api/napi_8h_source.html.

The main, up-to-date sourcecode seems to be here https://github.com/nodejs/node-addon-api.

But the usage examples seem to be here https://github.com/nodejs/abi-stable-node-addon-examples/tree/master/1_hello_world/node-addon-api.

And the name of the project, node-addon-api is not very distinguishable from the the built-in node project named abi-stable-node which it wraps.

The usage examples for napi.h code are in the repo named abi-stable-node-addon-examples. This name would imply that usage examples should be of the C N-API API found under abi-stable-node, but instead we find usage examples there of the node-addon-api.

All of this is confusing to a new user of the API. Some explanation up-front in the main readme.md as to the different projects, names, urls, statuses and relationships, as well as either an update or a takedown of https://nodejs.github.io/node-addon-api/napi_8h_source.html would be helpful.

Ultimately, folding this project into Node.js directly would make a lot of sense to me, and the Node.js documentation itself should say, here's the ABI stable C API named 'N-API' and heres the C++ wrapper for 'N-API' named 'N-API-CPP.

Here is a Stack Overflow question I opened, which got tumbleweeds, that illustrates the challenges.

@rivertam
Copy link
Contributor

I concur. I've been trying to build a binding for functionality that needs to be in C++. The main node docs start by using V8 directly, which I learned is a bad idea for multiple reasons. Then the N-API uses node_api.h which appears to be entirely a C API, but I'm running into all kinds of issues ("cannot create a handle without a HandleScope" woes), and now I'm learning everyone (for some definition of everyone) is using napi.h, which is for C++ but also called N-API and has very little documentation. I feel like I'm barely exaggerating when I say this could not be more confusing. It reminds me of this xkcd.

@NickNaso
Copy link
Member

NickNaso commented Jun 18, 2018

Hi everyone,
thanks for your feedbacks they are very important for us. In this period we are working on documentation and we hope to complete it very soon.

@mhdawson
Copy link
Member

We do agree that documentation needs to be improved and as NickNaso mention we are working on that. If you have some specific wording that you think would improve things and where that wording would go to best clarify things please suggest it either here or through a PR.

I do have in mind that we should merge the abi-stable-node-node-addon-examples back into node-addon-examples, we've just not had time to do that yet.

@rivertam
Copy link
Contributor

For my needs, I'm confused about the relationship between N-API and... N-API. I don't know exactly how to improve the documentation because I don't actually know the answer to the question. 😛

There's <napi.h> and there's <node_api.h> and it's been simply unclear to me what the relationship is. It looks like the former is "node-addon-api" which is also known as "N-API" whereas the latter is just known as "N-API" and is (for some reason unknown to me, to be honest) compatible with C.

I'm just seeing now the following line in the official N-API docs:

The N-API is a C API that ensures ABI stability across Node.js versions and different compiler levels. However, we also understand that a C++ API can be easier to use in many cases. To support these cases we expect there to be one or more C++ wrapper modules that provide an inlineable C++ API. Binaries built with these wrapper modules will depend on the symbols for the N-API C based functions exported by Node.js. These wrappers are not part of N-API, nor will they be maintained as part of Node.js. One such example is: node-addon-api.

I guess I think the vast, vast majority of users are using C++ for addons, so it's unclear to me why node-addon-api is briefly mentioned while the C N-API is covered extensively in the docs. It seems to me the C++ wrapper should be the one covered by default (more-so even than the V8 docs, which take an even higher precedence in the docs) with a footnote saying "by the way, we also have a lower level header called node_api.h if you need C compatibility".

I also think one very quick win in this regard would be renaming napi.h -> napi.hpp to make it clear that one is for C++ and one is for C, though I understand this poses other issues (which could be solved by having a napi.h that's just #include <napi.hpp> or vice versa).

@mhdawson
Copy link
Member

I'll think about this suggestion a bit. Officially N-API is the C API and is the API provided by Node.js. node-addon-api is just a convenience for those using C++ similar to nan. I'll have to re-look to see how well covered nan is in the core documentation as a comparison.

One thing we should do is to make sure we don't refer to node-addon-api as N-API, but instead as a module supporting the use of N-API.

The only way you get napi.h is after having installed node-addon-api, so I'm thinking improving the docs for node-addon-api might be better than renaming napi.h.

@rivertam
Copy link
Contributor

I see. I think maybe the most confusing thing here is the main README of this project. "Node.js API (N-API) Package" is the main title/header and "This package contains header-only C++ wrapper classes for the ABI-stable Node.js API also known as N-API" is extremely ambiguous.

I also definitely think covering the distinction much more thoroughly in the main node docs would be very fruitful, as I'd be willing to bet a very small portion of the people on that page would be better served by the N-API than by this wrapper. Maybe I should be bringing that up elsewhere though. I definitely think when the docs here are more thorough, they should have their own page on the official node docs.

@mhdawson
Copy link
Member

@rivertam once we have completed the docs here , I was going to consider if we might integrate more closely with the core documentation in some way. Do want to get them into a more complete state first.

I'll take a look at improving the readme for this project as a first step.

@mhdawson
Copy link
Member

Here is a PR that tries to clarify the positioning #288

@rivertam @webern can you two take a look and see if it helps?

@mhdawson
Copy link
Member

Landed #288, @rivertam, @webern what would be the next most helpful thing to try to address?

@rivertam
Copy link
Contributor

IMO getting the API docs up to snuff with what's offered for N-API should be the next priority as once that occurs there will be no need to ever reference the N-API docs.

@webern
Copy link
Author

webern commented Jun 22, 2018

I would put the usage examples into this repo instead of abi-stable-node-addon-examples.
More usage examples would be very helpful. One thing that I spent about 40 hours trying to do before giving up, was trying to hang on to a JavaScript function object during the lifetime of an AsyncWorker.

I discovered with your help pointing me to an open pull request, that you cannot touch the JavaScript objects during Execute(). But I thought maybe I could hold a function and use it in OnOK() since we're back in the main JS thread at that point. But I couldn't get it to work.

I strove to understand scopes, and HandleScope, read about it many times but found comprehension impenetrable, both in the N-API and in this C++ API. I tried various flailing random attempts at placing a HandleScope here, or there, or holding one as member data of the AsyncWorker. I built Node from source so that I could step through with lldb in Xcode trying to understand how and why my function object was being destroyed, etc.

I hope this gives some insight into the travails of trying to learn this domain. Thanks!

@betomoretti
Copy link

betomoretti commented Jun 25, 2018

@webern I gave a talk related to performance improvements using addons and I fought with the same thing. Here's the repo with the code https://github.com/betomoretti/async-addon. Check the worker files and maybe it will help you :)

@mhdawson
Copy link
Member

@rivertam @webern thanks for the comments. I think it confirms our current priority on Docs make sense. Once we get the first complete pass we can then go back to see what we can do to add more info on Scopes and Async usage.

@gabrielschulhof
Copy link
Contributor

@rivertam @webern we have now revived the node-addon-examples repo and have added an example using Napi::AsyncWrap. We've also added a lot of documentation to this repo.

@mhdawson
Copy link
Member

mhdawson commented Oct 9, 2018

We should probably remove the outdated Doxygen doc, created this issue for that: #368

@mhdawson
Copy link
Member

mhdawson commented Dec 7, 2020

Going to close this out as there was been a good amount of work completed on the docs, and #368 is there to cover Doxygen. At this point I think we should open a new issue if there is more specific work to do. Going to close, please let us know if this is not the right thing to do.

@mhdawson mhdawson closed this as completed Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants