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.

Sign up for GitHub

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

Jump to bottom

n-api: update documentation #19078

Closed
wants to merge 1 commit into from
Closed

n-api: update documentation #19078

wants to merge 1 commit into from

Conversation

gabrielschulhof
Copy link
Contributor

Document which APIs work while an exception is pending.

Fixes: nodejs/abi-stable-node#257

Checklist
Affected core subsystem(s)

n-api

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. dont-land-on-v4.x node-api Issues and PRs related to the Node-API. labels Mar 1, 2018
@gabrielschulhof gabrielschulhof force-pushed the document-exception-state-apis branch from 9ccb573 to 2a071a9 Compare March 1, 2018 22:53
@mhdawson
Copy link
Member

mhdawson commented Mar 1, 2018

I wonder if we want to lock ourselves into documenting all of these. They may be ok now but I'm not sure how easy it would be to ensure that remains the case.

Instead it might be better to just document the ones we think people "should" be able to use while an exception is pending.

@gabrielschulhof
Copy link
Contributor Author

@mhdawson I've broken down the functions into groups that I think might be relevant, and placed them in order from least likely to most likely to require NAPI_PREAMBLE() in the future.

# No V8 involved
napi_get_last_error_info
napi_get_version
napi_get_node_version
napi_create_async_work
napi_delete_async_work
napi_get_uv_event_loop
napi_queue_async_work
napi_cancel_async_work

# Should not execute JS - even in the future
napi_get_cb_info
napi_get_new_target
napi_create_reference
napi_delete_reference
napi_reference_ref
napi_reference_unref
napi_get_reference_value
napi_open_handle_scope
napi_close_handle_scope
napi_open_escapable_handle_scope
napi_close_escapable_handle_scope
napi_escape_handle
napi_open_callback_scope
napi_close_callback_scope
napi_async_init
napi_async_destroy
napi_is_exception_pending
napi_get_and_clear_last_exception
napi_get_buffer_info
napi_get_arraybuffer_info
napi_get_typedarray_info
napi_get_dataview_info
napi_adjust_external_memory

# New instances of JS types
napi_create_object
napi_create_array
napi_create_array_with_length
napi_create_string_latin1
napi_create_string_utf8
napi_create_string_utf16
napi_create_double
napi_create_int32
napi_create_uint32
napi_create_int64
napi_get_boolean
napi_create_symbol
napi_create_error
napi_create_type_error
napi_create_range_error
napi_get_undefined
napi_get_null

#Type check functions
napi_is_array
napi_is_error
napi_is_buffer
napi_is_arraybuffer
napi_is_typedarray
napi_is_dataview
napi_is_promise

# JS type to native type
napi_get_value_double
napi_get_value_int32
napi_get_value_uint32
napi_get_value_int64
napi_get_value_bool
napi_get_value_string_latin1
napi_get_value_string_utf8
napi_get_value_string_utf16
napi_get_value_external

napi_typeof

Of course, I cannot predict how the language will evolve, and where JS execution hooks will be added - such as with napi_instanceof() and the hasInstance symbol.

Which do you think we should document?

@mhdawson
Copy link
Member

mhdawson commented Mar 2, 2018

I think these are needed to handle exceptions:

napi_get_last_error_info
napi_is_exception_pending
napi_get_and_clear_last_exception

These are probably useful for cleanup:

napi_delete_async_work
napi_cancel_async_work
napi_delete_reference
napi_close_handle_scope
napi_close_escapable_handle_scope
napi_escape_handle
napi_close_callback_scope
napi_async_destroy

I'm thinking maybe we just leave it at those. What do you think?

@gabrielschulhof
Copy link
Contributor Author

@mhdawson Maybe we should add napi_fatal_error to the list.

n-api: update documentation
5480afe 
Document which APIs work while an exception is pending. This is the
list of all APIs which do not start with `NAPI_PREAMBLE(env)`.

Fixes: nodejs/abi-stable-node#257
@gabrielschulhof gabrielschulhof force-pushed the document-exception-state-apis branch from 2a071a9 to 5480afe Compare March 6, 2018 19:32
@gabrielschulhof
Copy link
Contributor Author

@mhdawson I have reduced the list to the one you proposed + napi_fatal_error.

mhdawson
mhdawson approved these changes Mar 6, 2018
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@gabrielschulhof
Copy link
Contributor Author

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

@gabrielschulhof
Copy link
Contributor Author

I don't believe the failures are related.

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Mar 7, 2018
n-api: update documentation
48b5c11 
Document which APIs work while an exception is pending. This is the
list of all APIs which do not start with `NAPI_PREAMBLE(env)`.

Fixes: nodejs/abi-stable-node#257
PR-URL: nodejs#19078
Reviewed-By: Michael Dawson <[email protected]>
@gabrielschulhof gabrielschulhof deleted the document-exception-state-apis branch March 7, 2018 17:10
@gabrielschulhof
Copy link
Contributor Author

Landed in 48b5c11.

targos pushed a commit that referenced this pull request Mar 17, 2018
@targos
n-api: update documentation
39b9215 
Document which APIs work while an exception is pending. This is the
list of all APIs which do not start with `NAPI_PREAMBLE(env)`.

Fixes: nodejs/abi-stable-node#257
PR-URL: #19078
Reviewed-By: Michael Dawson <[email protected]>
@targos targos mentioned this pull request Mar 18, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
@MylesBorins
n-api: update documentation
12f19a6 
Document which APIs work while an exception is pending. This is the
list of all APIs which do not start with `NAPI_PREAMBLE(env)`.

Fixes: nodejs/abi-stable-node#257
PR-URL: #19078
Reviewed-By: Michael Dawson <[email protected]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 12, 2018
n-api: update documentation
de2d3a6 
Document which APIs work while an exception is pending. This is the
list of all APIs which do not start with `NAPI_PREAMBLE(env)`.

Fixes: nodejs/abi-stable-node#257
PR-URL: nodejs#19078
Reviewed-By: Michael Dawson <[email protected]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 16, 2018
n-api: update documentation
ab4763d 
Document which APIs work while an exception is pending. This is the
list of all APIs which do not start with `NAPI_PREAMBLE(env)`.

Fixes: nodejs/abi-stable-node#257
PR-URL: nodejs#19078
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@MylesBorins
n-api: update documentation
a8237ef 
Document which APIs work while an exception is pending. This is the
list of all APIs which do not start with `NAPI_PREAMBLE(env)`.

Fixes: nodejs/abi-stable-node#257
Backport-PR-URL: #19447
PR-URL: #19078
Reviewed-By: Michael Dawson <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
Merged
MylesBorins pushed a commit that referenced this pull request May 1, 2018
@MylesBorins
n-api: update documentation
df63adf 
Document which APIs work while an exception is pending. This is the
list of all APIs which do not start with `NAPI_PREAMBLE(env)`.

Fixes: nodejs/abi-stable-node#257
Backport-PR-URL: #19265
PR-URL: #19078
Reviewed-By: Michael Dawson <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@MayaLekova
n-api: update documentation
36caa60 
Document which APIs work while an exception is pending. This is the
list of all APIs which do not start with `NAPI_PREAMBLE(env)`.

Fixes: nodejs/abi-stable-node#257
PR-URL: nodejs#19078
Reviewed-By: Michael Dawson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. node-api Issues and PRs related to the Node-API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Document list of functions that are safe to call in "pending exception" state
3 participants
@gabrielschulhof @mhdawson @nodejs-github-bot