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

test: add basic tests and doc for scopes #250

Closed
wants to merge 1 commit into from

Conversation

mhdawson
Copy link
Member

No description provided.

Copy link
Member

@NickNaso NickNaso left a comment

Choose a reason for hiding this comment

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

Hi @mhdawson,
I tried to suggest some corrections and improvements.

As the methods and classes within the node-addon-api are used,
handles to objects in the heap for the underlying
VM may be created. A handle may be created for any new node-addon-api
Value(and subclasses) created or returned. These handles must hold the
Copy link
Member

Choose a reason for hiding this comment

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

A handle may be created when any new node-addon-api Value and its subclasses have been created or returned.

are not collected while native code is using them. A handle may be
created any time a node-addon-api Value(and subclasses) are created
or returned.
Copy link
Member

Choose a reason for hiding this comment

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

A handle may be created when any new node-addon-api Value and its subclasses have been created or returned.


For more details refer to the section title "Object lifetime management".

Copy link
Member

Choose a reason for hiding this comment

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

It could be better add the link to "Object lifetime management"

Value(and subclasses) created or returned. These handles must hold the
objects 'live' until they are no longer required by the native code,
otherwise the objects could be collected before the native code was
finished using them.
Copy link
Member

Choose a reason for hiding this comment

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

otherwise the objects could be collected by the garbage collector before the native code was finished using them.

The EscapableHandleScope class is used to manage the lifetime of object handles
which are created through the use of node-addon-api. These handles
keep an object alive in the heap in order to ensure that the objects
are not collected while native code is using them. A handle may be
Copy link
Member

Choose a reason for hiding this comment

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

are not collected by garbage collector

which allows a single handle to be "promoted" to an outer scope.

For more details refer to the section title "Object lifetime management".

Copy link
Member

Choose a reason for hiding this comment

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

It could be better add the link to "Object lifetime management"

- `[in] scope`: pre-existing napi_handle_scope.

Creates a new EscapableHandleScope instance which wraps the
napi_escapable_handle_scope handle passed in. This can be used
Copy link
Member

Choose a reason for hiding this comment

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

Remove extra space here: in. This can be used

```

Deletes the EscapableHandleScope instance and allows any objects/handles created
in the scope to be collected by the garbage collector. There is no
Copy link
Member

Choose a reason for hiding this comment

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

Remove extra space here: collector. There is no

- `[in] escapee`: Napi::Value or napi_env to promote to the outer scope

Returns Napi:Value which can be used in the outer scope. This method can
be called at most once on a given EscapableHandleScope. If it is called
Copy link
Member

Choose a reason for hiding this comment

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

Remove extra space here: EscapableHandleScope. If


Returns Napi:Value which can be used in the outer scope. This method can
be called at most once on a given EscapableHandleScope. If it is called
more than once and exception will be thrown.
Copy link
Member

Choose a reason for hiding this comment

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

If it is called more than once an exception will be thrown.

@mhdawson
Copy link
Member Author

mhdawson commented May 1, 2018

@NickNaso, thanks for the comments. Pushed commit which should address them.

Creates a new escapable handle scope.

```cpp
EscapableHandleScope::New(napi_env env, napi_handle_scope scope);
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency does it make sense to include return types?


- `[in] Env`: The environment in which to construct the EscapableHandleScope object.

Creates a new EscapableHandleScope
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it makes more sense to use "Returns" for consistency. In working on my documentation it seems like the pattern should be:

  • Description/Remarks
  • Signature
  • Parameters
  • Returns

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed is should say "Returns" will update

@mhdawson
Copy link
Member Author

mhdawson commented May 3, 2018

addressed second set of comments will land.

mhdawson added a commit that referenced this pull request May 3, 2018
PR-URL: #250
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@NickNaso.local>
@mhdawson
Copy link
Member Author

mhdawson commented May 3, 2018

Landed as 75086da

@mhdawson mhdawson closed this May 3, 2018
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
PR-URL: nodejs/node-addon-api#250
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@NickNaso.local>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
PR-URL: nodejs/node-addon-api#250
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@NickNaso.local>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
PR-URL: nodejs/node-addon-api#250
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@NickNaso.local>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
PR-URL: nodejs/node-addon-api#250
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@NickNaso.local>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants