nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
107.25k stars 29.41k forks source link

`N-API` and/or `node-addon-api` Docs Clarification and Submission Offer #47504

Closed matthewkeil closed 6 months ago

matthewkeil commented 1 year ago

Affected URL(s)

No response

Description of the problem

N-API and/or node-addon-api Documentation Offer and Question

Overview

Hello fellow open source contributors!!

My name is Matthew Keil and I am part of the Lodestar team at ChainSafe. I wrote extensive documentation for developing node addons, that will serve as open-source instruction for future contributors. I have a few goals for writing this issue:

In particular, I am writing you today to ask for some clarification on how HandleScope works. Two-Fold I want to make sure the documentation I am writing is 1000% accurate and the code is equally stable. This Issue (node-addon-api#1267) is a great example of what I am talking about. There is a bit of muddiness in the existing docs surrounding values and handle scopes. I read through the nodejs/node codebase, and several surrounding pieces of information, to give my best effort to represent things correctly here. That markdown document speaks to how Objects, Values, Handles and HandleScopes all work together under-the-hood with a goal of using Napi::HandleScope correctly.

I was wondering if you would like to take a look at it. My hopes is that you can help review what is written there so that it is 100% accurate. Once it is I would like to offer it as an addition to the existing body of documentation in any way that the node team feels is appropriate. I will be happy to change any wording or the tone of the document so that it fits in its new home. No obligation though in any way. If that is not something that is interesting, the docs will all be open-source in perpetuity and I will be very glad to simply say thank you on behalf of the community for any help you can provide.

What Started This Process

@chainsafe/blst-ts is a dependency for Lodestar which is one of the officially sponsored Ethereum consensus clients. Blockchain requires a lot of heavy math, and I just finished one set of bindings and am actively building another (first and second). Because of the seriousness, and need for absolute correctness, I sought to more deeply understand N-API and node-addon-api as well as how they interact with the runtime for architectural purposes.

The docs started as implementation notes and primer for team-members that are not familiar with the native layer. They ended up having a lot of good info and may be useful to others not contributing to the @chainsage/blst-ts repo. You can find the docs here and an open PR here for revising/merging. I assume there will be other math libraries developed by the Ethereum Foundation and want to add my learning lessons for all contributors that follow.

More Info on the Documentation

The docs are written (from a JS dev perspective) as a resource to fill in some gaps beyond basic syntax and the existing napi docs and blog posts. My team members will review a C/C++ PR and they are all ninja JS devs (also many can code in several other languages) but I have the most experience with this particular kind of stuff. The goal was to talk about my learning lessons prior to PR to improve the quality of the merged code. It can also serve as a contributors guide and as a resource for team members that want to build other native addons.

I added some "hows-and-whys" with addon development, helpful native code patterns, how the native layer of node works (roughly), understanding of how the N-API and node-addon-api relate to each other, how to debug effectively, effective workflows and how all of those pieces come into focus during active development. There is also a reference section for context, details on workflow, DX considerations and nice-to-haves like setting up and using Valgrind.

The table of contents can be found here. If there is anything else you would like to use for the documentation of N-API, node-addon-api or node-addon-examples it will be my sincere pleasure to see it in the docs.

legendecas commented 1 year ago

Improvements on the document are always welcome. Please feel free to submit PRs to both repositories following the contribution guideline.

However, the documents hosted in the nodejs/node and nodejs/node-addon-api repositories are scoped to the APIs provided. Expanding on topics not maintained in the repo may not be accepted as it would increase the maintenance burden. Still, documents that help people start building add-ons can be listed at https://github.com/nodejs/abi-stable-node/blob/doc/node-api-media.md.

There is also a reference section for context, details on workflow, DX considerations and nice-to-haves like setting up and using Valgrind.

@KevinEady is working on a document to illustrate how to use Valgrind with add-ons too: https://github.com/nodejs/abi-stable-node/issues/440.

matthewkeil commented 1 year ago

Thank you very much @legendecas. I am working through the project that started this thread and will circle back shortly to put up a PR in nodejs/node, nodejs/node-addon-api, and nodejs/node-addon-examples as time allows.

gabrielschulhof commented 1 year ago

@matthewkeil have you had a chance to identify material you might be able to contribute?

matthewkeil commented 1 year ago

Hi @gabrielschulhof I am working on some stuff now and will post it in the node-addon-examples repo. I have been moving house and been busy with life/work since the last napi contributor call I participated in about a month ago with @mhdawson. I will attempt to join next week as this week I am at a conference with work today.

The heart of what I am working on in the background is cleaning up the organization of the node-addon-examples and adding some docs and examples that explain how the examples are structured and some implementation details for using node api. I look forward to putting up a PR for discussion and feel free to drop a line in there if you have any questions.

mhdawson commented 6 months ago

I think the work was done in the examples repo, @matthewkeil can this issue be closed or is it still needed to surface future work?

matthewkeil commented 6 months ago

I think the work was done in the examples repo, @matthewkeil can this issue be closed or is it still needed to surface future work?

Hi there @mhdawson. Thanks for checking in.

Yes did this work over there and have a bunch more docs I can add. Am on SEAsian time since the holidays and haven't been able to join the napi meetings recently.

Just merged a bunch to one of the other repos I support and will look to get the relevant pieces moved over in the near future.

Feel free to close this for now and I'll open a PR as soon as I get a few minutes.

mhdawson commented 6 months ago

Closing as suggested