Skip to content

test: add basic tests and doc for scopes#250

Closed
mhdawson wants to merge 1 commit intonodejs:masterfrom
mhdawson:handlescope
Closed

test: add basic tests and doc for scopes#250
mhdawson wants to merge 1 commit intonodejs:masterfrom
mhdawson:handlescope

Conversation

@mhdawson
Copy link
Copy Markdown
Member

No description provided.

Copy link
Copy Markdown
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.

Comment thread doc/object_lifetime_management.md Outdated
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
Copy Markdown
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.

Comment thread doc/handle_scope.md Outdated
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
created any time a node-addon-api Value(and subclasses) are created
or returned.
Copy link
Copy Markdown
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.

Comment thread doc/handle_scope.md
or returned.

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

Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread doc/escapable_handle_scope.md Outdated
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
Copy Markdown
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
Copy Markdown
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"

Comment thread doc/escapable_handle_scope.md Outdated
- `[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
Copy Markdown
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

Comment thread doc/escapable_handle_scope.md Outdated
```

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
Copy Markdown
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

Comment thread doc/escapable_handle_scope.md Outdated
- `[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
Copy Markdown
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

Comment thread doc/escapable_handle_scope.md Outdated

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
Copy Markdown
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
Copy Markdown
Member Author

mhdawson commented May 1, 2018

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

Comment thread doc/escapable_handle_scope.md Outdated
Creates a new escapable handle scope.

```cpp
EscapableHandleScope::New(napi_env env, napi_handle_scope scope);
Copy link
Copy Markdown
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?

Comment thread doc/escapable_handle_scope.md Outdated

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

Creates a new EscapableHandleScope
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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