Skip to content

node-api: minimal C node embedding API function #58207

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alshdavid
Copy link

Currently, this PR is raised to aid in the discussion of #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:

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels May 7, 2025
@vmoroz vmoroz added the node-api Issues and PRs related to the Node-API. label May 9, 2025
@mhdawson mhdawson moved this from Need Triage to In Progress in Node-API Team Project May 9, 2025

#include "node.h"

#ifdef _WIN32
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This header file must follow the design patterns used by Node-API code:

  • 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.
  • For the __cdecl there is a macro NAPI_CDECL. It must help to avoid the code duplication.
  • We must use the EXTERN_C_START and EXTERN_C_END macros around the declaration of functions to enable that header to be used from C++.

@vmoroz
Copy link
Member

vmoroz commented May 11, 2025

@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 node-api: use c-based api for libnode embedding #54660.

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

@alshdavid
Copy link
Author

alshdavid commented May 11, 2025

@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.

@vmoroz
Copy link
Member

vmoroz commented May 12, 2025

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
Copy link
Member

vmoroz commented May 12, 2025

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

@alshdavid alshdavid force-pushed the alsh/node_embedding_api branch from f76b323 to 8b4dfe3 Compare May 12, 2025 02:54
@alshdavid alshdavid force-pushed the alsh/node_embedding_api branch from 8b4dfe3 to 732c46c Compare May 12, 2025 02:56
@alshdavid
Copy link
Author

alshdavid commented May 12, 2025

@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 alshdavid changed the title RFC: Minimal C node_embedding_api function Minimal C node_embedding_api function May 12, 2025
@vmoroz
Copy link
Member

vmoroz commented May 13, 2025

@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 vmoroz changed the title Minimal C node_embedding_api function node-api: minimal C node embedding API function May 13, 2025
@vmoroz vmoroz added the embedding Issues and PRs related to embedding Node.js in another project. label May 13, 2025
Copy link

codecov bot commented May 13, 2025

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.18%. Comparing base (92102c0) to head (732c46c).
Report is 85 commits behind head on main.

Files with missing lines Patch % Lines
src/node_embedding_api.cc 0.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main   #58207    +/-   ##
========================================
  Coverage   90.18%   90.18%            
========================================
  Files         630      632     +2     
  Lines      186473   186692   +219     
  Branches    36612    36669    +57     
========================================
+ Hits       168169   168373   +204     
  Misses      11116    11116            
- Partials     7188     7203    +15     
Files with missing lines Coverage Δ
src/node_embedding_api.cc 0.00% <0.00%> (ø)

... and 70 files with indirect coverage changes

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. embedding Issues and PRs related to embedding Node.js in another project. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API.
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants