node icon indicating copy to clipboard operation
node copied to clipboard

node-api: C embedding function "node_rt_main"

Open alshdavid opened this issue 7 months ago • 20 comments

Currently, this PR is raised to aid in the discussion of https://github.com/nodejs/node/pull/54660, which implements the complete API.

This PR only adds a single "unstable" C compatible function for embedders. node_embedding_start which forwards to node::Start.

While this function does not offer a complete embedder API like the aforementioned PR, when combined with some glue code on the JavaScript side and existing n-api functionality, it is sufficient to enable a large portion of use cases for embedders.

Example use cases; Calling into JavaScript plugins that feature Node.js compatibility from a language that can consume a C library (Rust, Go, Zig, C#, etc)

While a rich C API would be amazing, my hope is that in the interim, a smaller change is more likely to be included into Nodejs and unblocks consumers that want to embed it.

Reference consumer embedder implementations:

alshdavid avatar May 07 '25 03:05 alshdavid

Review requested:

  • [ ] @nodejs/gyp

nodejs-github-bot avatar May 07 '25 03:05 nodejs-github-bot

@alshdavid , we had discussed this PR in our last Node-API meeting on 5/9/2025. The overall opinion is that we should proceed with this change. The key points from the discussion are:

  • The API should follow the same patterns as in the Node-API. It should include the Node-API header and use its macros as other Node-API functions do. (See my comment for the header file.)
  • We discussed a bit the function name. It should reflect the purpose and then work together with other upcoming embedding APIs. While node_embedding_start reflects the Node.js function node::Start, its name may sound a bit confusing for the people seeing it the fist time. My intuition is that if we have start, then we should have the end, isn't it? I would propose the name node_embedding_main which IMHO better reflects the purpose. Other candidate names are node_embedding_run, node_embedding_main_run, and node_embedding_run_main. What is your opinion?
  • It would be nice to see the tests or examples. Though they can be added in the follow up PR.
  • We must document this function. You may consider looking at the embedding docs that I started to work on in PR #54660.

Overall, it is a great step towards the ABI-stable embedding API and thank you for doing it!

vmoroz avatar May 11 '25 23:05 vmoroz

@alshdavid , we had discussed this PR in our last Node-API meeting on 5/9/2025. The overall opinion is that we should proceed with this change.

🥳

  • We discussed a bit the function name. [...]I would propose the name node_embedding_main. What is your opinion?

I agree with your intuition here. node_embedding_main feels more appropriate.

  • The API should follow the same patterns as in the Node-API. It should include the Node-API header and use its macros as other Node-API functions do. (See my comment for the header file.)
  • We must document this function. You may consider looking at the embedding docs that I started to work on in PR node-api: use c-based api for libnode embedding #54660.

I will update the PR to reflect these requirements and happy to write the documentation.

  • It must not depend on the node.h. The node.h is a non-ABI stable C++ API. It should rather depend on the node-api.h or js_native_api.h.
  • It would be nice to see the tests or examples. Though they can be added in the follow up PR.

If I'm honest, outside of compsci 101 many years ago, I am very new to real-world C/C++ and would definitely require assistance with writing tests as I'm already treading water just trying to navigate the build system, templating and macros 😅.

As for using node-api.h or js_native_api.h, I will give it a try but I'm a bit out of my depth and am concerned I will miss platform specific conditional templates or something (e.g. __cdecl was a surprise to me).

Overall, it is a great step towards the ABI-stable embedding API and thank you for doing it!

Likewise! Thank you for working with the team to get the ball rolling, obtaining consensus on the approach and helping out with the implementation specifics in comments to this PR.

P.S. I wouldn't be offended if you added commits to this PR or rewrote it/raised a new PR that is idiomatic as I might need some time to get it right.

alshdavid avatar May 11 '25 23:05 alshdavid

P.S. I wouldn't be offended if you added commits to this PR or rewrote it/raised a new PR that is idiomatic as I might need some time to get it right.

@alshdavid , sure, I can do that to streamline the process. Though, in the end, you must squash all changes to redo the first commit. It is important that the name of the first commit and the PR name follow the Node.js rules: be prefixed wth node-api: and followed with a lower case letter, e.g.: node-api: minimal C node_embedding_api function. See: https://github.com/nodejs/node/blob/main/doc/contributing/pull-requests.md#commit-message-guidelines

vmoroz avatar May 12 '25 00:05 vmoroz

@alshdavid , I do not have permissions to update your PR branch directly. This commit has the changes that I would suggest for the PR: https://github.com/vmoroz/node/commit/0642652abbd6546082d1db77752770928c0071ee Feel free to merge/copy them.

vmoroz avatar May 12 '25 01:05 vmoroz

@vmoroz, thanks for the commit. I have applied those changes and amended the commit message to fit the Nodejs rules

I have also added you as a co-author and given you write access to my fork if you'd like to make changes

alshdavid avatar May 12 '25 02:05 alshdavid

@vmoroz, thanks for the commit. I have applied those changes and amended the commit message to fit the Nodejs rules

I have also added you as a co-author and given you write access to my fork if you'd like to make changes

@alshdavid , it looks great! Let me also update the PR title. It would be great to update the embedding.md

@nodejs/node-api and @nodejs/embedders , could you have a look at this PR?

vmoroz avatar May 13 '25 01:05 vmoroz

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 88.56%. Comparing base (7fd3688) to head (da41eec). :warning: Report is 96 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #58207   +/-   ##
=======================================
  Coverage   88.56%   88.56%           
=======================================
  Files         703      704    +1     
  Lines      208254   208256    +2     
  Branches    40156    40168   +12     
=======================================
+ Hits       184430   184433    +3     
  Misses      15828    15828           
+ Partials     7996     7995    -1     
Files with missing lines Coverage Δ
src/node_runtime_api.cc 100.00% <100.00%> (ø)

... and 35 files with indirect coverage changes

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar May 13 '25 03:05 codecov[bot]

@vmoroz @mhdawson , updated the PR to fix the linting error and add some basic documentation. Feel free to modify the documentation to better suit.

I tried to write an example but I'm better at Rust than I am C and I didn't want to write something misleading

alshdavid avatar May 18 '25 02:05 alshdavid

We discussed in the Node-api team meeting today and @vmoroz volunteered to pull the framework for testing from his PR into this one with a basic test.

mhdawson avatar May 30 '25 15:05 mhdawson

Hi @vmoroz, any chance you've had some time to look into this?

alshdavid avatar Jul 24 '25 02:07 alshdavid

Bump. This would be so awesome to have!

noncom avatar Aug 31 '25 11:08 noncom

We are currently using my libnode fork at work and it would be amazing if we could make progress here as there are reservations with using/maintaining a fork. Once this is merged I can add a recipe to nodejs/unofficial-builds so we can have access to libnode.so libnode.dylib libnode.dll from a trusted domain

alshdavid avatar Aug 31 '25 23:08 alshdavid

+1

benpalevsky avatar Sep 06 '25 00:09 benpalevsky

Any C# bindings? (other than PuerTS or EdgeJS, they don't support C++ Native .node)

ImanCol avatar Nov 17 '25 15:11 ImanCol

@ImanCol if you search for a binding for node-api C# you can find to this page https://github.com/nodejs/abi-stable-node/blob/doc/node-api-engine-bindings.md.

NickNaso avatar Nov 17 '25 15:11 NickNaso

@ImanCol if you search for a binding for node-api C# you can find to this page https://github.com/nodejs/abi-stable-node/blob/doc/node-api-engine-bindings.md.

I ran the test using the Microsoft API. These were the results: https://github.com/nodejs/abi-stable-node/issues/471

Same problem with PuertaTA, Edge JS, I can't load .node Natives

ImanCol avatar Nov 17 '25 23:11 ImanCol

@alshdavid , I do not have permissions to update your PR. I did all the edits and added tests in this branch: https://github.com/vmoroz/nodejs-node/tree/alshdavid-node_embedding_api It is just one last commit. The key changes are:

  • Added unit test
  • Updated the embedding tests to report progress and enable multiple "main" functions
  • Changed the function prefix to node_rt_
  • Changed the header file name to node_runtime_api.h

We are going to discuss these changes in the Node-API meeting and then hopefully you can adopt them in your PR.

vmoroz avatar Nov 21 '25 15:11 vmoroz

Hey @vmoroz, I have merged your changes into this PR. I have also squashed the commits and rebased onto main.

alshdavid avatar Nov 22 '25 23:11 alshdavid

Hey @vmoroz, I have merged your changes into this PR. I have also squashed the commits and rebased onto main.

Thank you, @alshdavid! Last week we did not have a Node-API meeting. Hopefully the change can be discussed this week and be reviewed by other team members.

@joyeecheung , @addaleax , @legendecas , @KevinEady , could you have a look at this PR?

vmoroz avatar Nov 23 '25 18:11 vmoroz