node-api: update headers for better wasm support by toyobayashi · Pull Request #49037 · nodejs/node · GitHub

Hi, Node-API team @nodejs/node-api

I'm wondering if emnapi could use node-api-headers package instead of maintaining a modified version of headers.

Just a little modification:

change __wasm32__ to __wasm__ for building wasm64-unknown-emscripten
expand NAPI_MODULE_EXPORT to __attribute__((used)) (EMSCRIPTEN_KEEPALIVE) to export napi_register_wasm_v1 and node_api_module_get_api_version_v1 if build with Emscripten
expand NAPI_EXTERN to __attribute__((__import_module__("env"))) if build with Emscripten. Not sure if this should. Currently there is no way to specify a custom module name in wasm import object for imported function which is implemented in JavaScript, all functions imported from JavaScript are under env module by default. If don't do this change, emnapi must require user to provide a instantiateWasm hook to emscripten and add napi module during this hook call, which isn't what instantiateWasm are supposed to be used for, that force users to write loading logic themselves.
```
import init from './node-api-wasm-module-compiled-by-emcc.js'

await init({
  instantiateWasm (imports, successCallback) {
    imports.napi = imports.env
    WebAssembly.instantiate(..., imports).then(({ instance, module }) => {
      successCallback(instance, module)
    })
    return {}
  }
})
```
remove __wasm32__ guards on async work and TSFN, because they have been implemented and are available in wasm, even in wasm32-unknown-unknown and wasm32-wasi. It is worth mentioning that I created a PR src: define NAPI_HAS_THREADS to make TSFN available on Emscripten node-addon-api#1283 that added NAPI_HAS_THREADS to Napi::AsyncWorker and Napi::ThreadSafeFunction, now maybe just add it to Napi::AsyncProgressWorker is enough because it's using std::mutex, which requires __EMSCRIPTEN_PTHREADS__ or __wasi__ && _REENTRANT

Review requested:

@nodejs/node-api

@toyobayashi what is the likelyhood that this would affect any existing addons?

@cjihrig I'm wondering if you have any thoughts/opinions on the request related to expand NAPI_EXTERN to __attribute__((__import_module__("env"))) if build with Emscripten

@mhdawson I don't think I'm familiar enough with this work to have a good opinion.

expand NAPI_EXTERN to __attribute__((__import_module__("env"))) if build with Emscripten

This very intentionally already expands to __attribute__((__import_module__("napi"))).

What we could do instead is allow you to define your own NAPI_EXTERN with a simple ifndef check. The default namespace would still be napi but you could modify it on a per-build basis.

what is the likelyhood that this would affect any existing addons?

@mhdawson Reverted changes on NAPI_EXTERN. I believe the remaining changes will have no impact on existing addons.

define your own NAPI_EXTERN with a simple ifndef check

Should we add #ifndef NAPI_EXTERN in BUILDING_NODE_EXTENSION? if BUILDING_NODE_EXTENSION is defined (node-gyp default behavior), custom NAPI_EXTERN seems to not work?

https://github.com/nodejs/node/blob/0d03b777151b1d34bcbbc35d6c38c123a009dded/src/node_api.h#L4-L11

- #ifdef BUILDING_NODE_EXTENSION
+ #if defined(BUILDING_NODE_EXTENSION) && !defined(NAPI_EXTERN)

Sharp is using node-gyp, emnapi and emscripten to build wasm port, the wasm version is already available on StackBlitz WebContainer. Though #ifndef NAPI_EXTERN already exists in js_native_api.h, I think adding && !defined(NAPI_EXTERN) to #ifdef BUILDING_NODE_EXTENSION is still necessary here since it comes before including js_native_api.h. Otherwise would request users who use both node-gyp and emscripten to undef BUILDING_NODE_EXTENSION in addition to define their own NAPI_EXTERN.

It makes sense that, if js_native_api.h is included via node_api.h, then NAPI_EXTERN should still be used if already available.

@legendecas is going to take another look at this before we land it.

CI: https://ci.nodejs.org/job/node-test-pull-request/54549/

CI: https://ci.nodejs.org/job/node-test-pull-request/54551/

Landed in b55adfb

PR-URL: #49037 Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

PR-URL: nodejs#49037 Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

PR-URL: nodejs/node#49037 Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

Re-enable ThinLTO on macOS. Add a new patch to ensure N-API entry points aren't stripped by ThinLTO. Copy the `__attribute__((used))` approach from nodejs/node#49037 .

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API. labels Aug 6, 2023

toyobayashi mentioned this pull request Aug 15, 2023

Support custom import namespace for functions implemented in JavaScript emscripten-core/emscripten#20035

Open

gabrielschulhof approved these changes Sep 22, 2023

View reviewed changes

mhdawson requested a review from legendecas September 29, 2023 15:25

toyobayashi added 5 commits October 2, 2023 23:50

node-api: update headers for better wasm support

4dbd893

remove NAPI_HAS_THREADS

1ca52b3

fix lint

d76e0ec

revert emscripten NAPI_EXTERN

d116291

always respect user-defined NAPI_EXTERN

adee653

toyobayashi force-pushed the node-api-headers-wasm branch from 0d4a5ef to adee653 Compare October 2, 2023 15:50

legendecas approved these changes Oct 5, 2023

View reviewed changes

legendecas added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Oct 5, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 5, 2023

legendecas added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 5, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 5, 2023

nodejs-github-bot merged commit b55adfb into nodejs:main Oct 5, 2023

github-actions Bot mentioned this pull request Oct 6, 2023

CI Reliability 2023-10-06 nodejs/reliability#685

Open

10 tasks

toyobayashi mentioned this pull request Oct 6, 2023

sync latest headers toyobayashi/emnapi#84

Merged

This was referenced Oct 7, 2023

CI Reliability 2023-10-07 nodejs/reliability#686

Open

CI Reliability 2023-10-08 nodejs/reliability#687

Open

CI Reliability 2023-10-09 nodejs/reliability#688

Open

RafaelGSS mentioned this pull request Oct 9, 2023

2023-10-17, Version 21.0.0 (Current) #49870

Merged

github-actions Bot mentioned this pull request Oct 10, 2023

CI Reliability 2023-10-10 nodejs/reliability#689

Open

24 tasks

targos pushed a commit that referenced this pull request Nov 11, 2023

node-api: update headers for better wasm support

e2f0ef2

PR-URL: #49037 Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

targos mentioned this pull request Nov 12, 2023

v20.10.0 release proposal #50682

Merged

targos pushed a commit that referenced this pull request Nov 27, 2023

node-api: update headers for better wasm support

f449b22

PR-URL: #49037 Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

targos mentioned this pull request Nov 28, 2023

v18.19.0 release proposal #50953

Merged

juanarbol mentioned this pull request Dec 6, 2023

2023-12-06, N|Solid v5.0.1 Iron Release nodesource/nsolid#41

Merged

debadree25 pushed a commit to debadree25/node that referenced this pull request Apr 15, 2024

node-api: update headers for better wasm support

3941379

PR-URL: nodejs#49037 Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024

node-api: update headers for better wasm support

8df8d89

PR-URL: nodejs/node#49037 Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024

node-api: update headers for better wasm support

00ec256

PR-URL: nodejs/node#49037 Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

ckerr mentioned this pull request May 16, 2026

build: re-enable ThinLTO on macOS electron/electron#51669

Merged

5 tasks

node-api: update headers for better wasm support by toyobayashi · Pull Request #49037 · nodejs/node · GitHub

Uh oh!

Uh oh!

node-api: update headers for better wasm support - #49037

node-api: update headers for better wasm support#49037nodejs-github-bot merged 5 commits intonodejs:mainnodejs/node:mainfrom toyobayashi:node-api-headers-wasmtoyobayashi/node:node-api-headers-wasmCopy head branch name to clipboard

Conversation

toyobayashi commented Aug 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 6, 2023

Uh oh!

mhdawson commented Aug 14, 2023

Uh oh!

mhdawson commented Aug 14, 2023

Uh oh!

cjihrig commented Aug 14, 2023

Uh oh!

devsnek commented Aug 14, 2023

Uh oh!

toyobayashi commented Aug 15, 2023

Uh oh!

toyobayashi commented Aug 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gabrielschulhof commented Sep 22, 2023

Uh oh!

mhdawson commented Sep 29, 2023

Uh oh!

nodejs-github-bot commented Oct 5, 2023

Uh oh!

nodejs-github-bot commented Oct 5, 2023

Uh oh!

nodejs-github-bot commented Oct 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

node-api: update headers for better wasm support#49037
nodejs-github-bot merged 5 commits into
nodejs:mainfrom
toyobayashi:node-api-headers-wasm

toyobayashi commented Aug 6, 2023 •
edited
Loading

toyobayashi commented Aug 15, 2023 •
edited
Loading