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

node-api: Add API to check safety of calling JS #43805

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions doc/api/n-api.md
Original file line number Diff line number Diff line change
Expand Up @@ -6295,6 +6295,24 @@ node_api_get_module_file_name(napi_env env, const char** result);
`result` may be an empty string if the add-on loading process fails to establish
the add-on's file name during loading.

#### `napi_can_call_into_js`

<!-- YAML
added: REPLACEME
-->

> Stability: 1 - Experimental

```c
NAPI_EXTERN napi_status napi_can_call_into_js(napi_env env);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is missing the bool* result parameter.

```

* `[in] env`: The environment that the API is invoked under.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.


Returns `napi_ok` if the API succeeded.

This API is used to check if it is safe to call into JavaScript.

[ABI Stability]: https://nodejs.org/en/docs/guides/abi-stability/
[AppVeyor]: https://www.appveyor.com
[C++ Addons]: addons.md
Expand Down
5 changes: 5 additions & 0 deletions src/js_native_api.h
Original file line number Diff line number Diff line change
Expand Up @@ -566,6 +566,11 @@ NAPI_EXTERN napi_status NAPI_CDECL napi_object_seal(napi_env env,
napi_value object);
#endif // NAPI_VERSION >= 8

#ifdef NAPI_EXPERIMENTAL
NAPI_EXTERN napi_status NAPI_CDECL napi_can_call_into_js(napi_env env,
Copy link
Member

Choose a reason for hiding this comment

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

From dicussion in Node-api team meeting today new methods should use node_api -> node_api_can_call_into_js in this case.

Overall there was agreement that adding a method makes sense.

The other suggestion was that we need a unit test.

bool* result);
#endif

EXTERN_C_END

#endif // SRC_JS_NATIVE_API_H_
7 changes: 7 additions & 0 deletions src/js_native_api_v8.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3252,3 +3252,10 @@ napi_status NAPI_CDECL napi_is_detached_arraybuffer(napi_env env,

return napi_clear_last_error(env);
}

napi_status NAPI_CDECL napi_can_call_into_js(napi_env env, bool* result) {
CHECK_ENV(env);
CHECK_ARG(env, result);
*result = (env)->can_call_into_js();
return napi_clear_last_error(env);
}