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

src: concentrate callbacks provided to core N-API #786

Conversation

gabrielschulhof
Copy link
Contributor

This change reduces the places where we declare private functions with
the napi_callback signature for the purpose of using them with C++
callbacks passed as template arguments. We basically have 4 types:

  1. static with void return
  2. static with napi_value return
  3. instance with void return
  4. instance with napi_value return

We can use one of these four calling patterns in the following places
where we accept callbacks as template arguments:

  • Napi::Function (1. and 2.)
  • Napi::PropertyDescriptor (1. for the setter, 2. for the getter)
  • Napi::InstanceWrap<T> (3., 4. for instance methods, 4. for
    instance getters)
  • Napi::ObjectWrap<T> (1., 2. for static methods, 2. for static
    getters)

In the case of InstanceWrap<T> and ObjectWrap<T> instance resp.
static property descriptors we can also remove the infrastructure
designed to allow for optional getters (GetterTag resp.
StaticGetterTag) because the API for specifying instance resp. class
property descriptors does not allow one to omit the getter.

Signed-off-by: @gabrielschulhof

This change reduces the places where we declare private functions with
the `napi_callback` signature for the purpose of using them with C++
callbacks passed as template arguments. We basically have 4 types:

  1. static with `void` return
  2. static with `napi_value` return
  3. instance with `void` return
  4. instance with `napi_value` return

We can use one of these four calling patterns in the following places
where we accept callbacks as template arguments:
  * `Napi::Function` (1. and 2.)
  * `Napi::PropertyDescriptor` (1. for the setter, 2. for the getter)
  * `Napi::InstanceWrap<T>` (3., 4. for instance methods, 4. for
     instance getters)
  * `Napi::ObjectWrap<T>` (1., 2. for static methods, 2. for static
     getters)

In the case of `InstanceWrap<T>` and `ObjectWrap<T>` instance resp.
static property descriptors we can also remove the infrastructure
designed to allow for optional getters (`GetterTag` resp.
`StaticGetterTag`) because the API for specifying instance resp. class
property descriptors does not allow one to omit the getter.

Signed-off-by: Gabriel Schulhof <[email protected]>
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

@mhdawson
Copy link
Member

I saw the AIX problem a few minutes ago while running node-addon-api tests for a different PR. I've pinged @richardlau and @AshCripps to look at getting that back on-line

gabrielschulhof pushed a commit that referenced this pull request Aug 25, 2020
This change reduces the places where we declare private functions with
the `napi_callback` signature for the purpose of using them with C++
callbacks passed as template arguments. We basically have 4 types:

  1. static with `void` return
  2. static with `napi_value` return
  3. instance with `void` return
  4. instance with `napi_value` return

We can use one of these four calling patterns in the following places
where we accept callbacks as template arguments:
  * `Napi::Function` (1. and 2.)
  * `Napi::PropertyDescriptor` (1. for the setter, 2. for the getter)
  * `Napi::InstanceWrap<T>` (3., 4. for instance methods, 4. for
     instance getters)
  * `Napi::ObjectWrap<T>` (1., 2. for static methods, 2. for static
     getters)

In the case of `InstanceWrap<T>` and `ObjectWrap<T>` instance resp.
static property descriptors we can also remove the infrastructure
designed to allow for optional getters (`GetterTag` resp.
`StaticGetterTag`) because the API for specifying instance resp. class
property descriptors does not allow one to omit the getter.

Signed-off-by: Gabriel Schulhof <[email protected]>
PR-URL: #786
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@gabrielschulhof
Copy link
Contributor Author

Landed in 9aceea7.

@gabrielschulhof gabrielschulhof deleted the simplify-objectwrap-templated-methods-new branch February 1, 2021 16:39
@gabrielschulhof gabrielschulhof restored the simplify-objectwrap-templated-methods-new branch February 1, 2021 16:41
@gabrielschulhof gabrielschulhof deleted the simplify-objectwrap-templated-methods-new branch March 7, 2021 18:44
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
This change reduces the places where we declare private functions with
the `napi_callback` signature for the purpose of using them with C++
callbacks passed as template arguments. We basically have 4 types:

  1. static with `void` return
  2. static with `napi_value` return
  3. instance with `void` return
  4. instance with `napi_value` return

We can use one of these four calling patterns in the following places
where we accept callbacks as template arguments:
  * `Napi::Function` (1. and 2.)
  * `Napi::PropertyDescriptor` (1. for the setter, 2. for the getter)
  * `Napi::InstanceWrap<T>` (3., 4. for instance methods, 4. for
     instance getters)
  * `Napi::ObjectWrap<T>` (1., 2. for static methods, 2. for static
     getters)

In the case of `InstanceWrap<T>` and `ObjectWrap<T>` instance resp.
static property descriptors we can also remove the infrastructure
designed to allow for optional getters (`GetterTag` resp.
`StaticGetterTag`) because the API for specifying instance resp. class
property descriptors does not allow one to omit the getter.

Signed-off-by: Gabriel Schulhof <[email protected]>
PR-URL: nodejs/node-addon-api#786
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
This change reduces the places where we declare private functions with
the `napi_callback` signature for the purpose of using them with C++
callbacks passed as template arguments. We basically have 4 types:

  1. static with `void` return
  2. static with `napi_value` return
  3. instance with `void` return
  4. instance with `napi_value` return

We can use one of these four calling patterns in the following places
where we accept callbacks as template arguments:
  * `Napi::Function` (1. and 2.)
  * `Napi::PropertyDescriptor` (1. for the setter, 2. for the getter)
  * `Napi::InstanceWrap<T>` (3., 4. for instance methods, 4. for
     instance getters)
  * `Napi::ObjectWrap<T>` (1., 2. for static methods, 2. for static
     getters)

In the case of `InstanceWrap<T>` and `ObjectWrap<T>` instance resp.
static property descriptors we can also remove the infrastructure
designed to allow for optional getters (`GetterTag` resp.
`StaticGetterTag`) because the API for specifying instance resp. class
property descriptors does not allow one to omit the getter.

Signed-off-by: Gabriel Schulhof <[email protected]>
PR-URL: nodejs/node-addon-api#786
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
This change reduces the places where we declare private functions with
the `napi_callback` signature for the purpose of using them with C++
callbacks passed as template arguments. We basically have 4 types:

  1. static with `void` return
  2. static with `napi_value` return
  3. instance with `void` return
  4. instance with `napi_value` return

We can use one of these four calling patterns in the following places
where we accept callbacks as template arguments:
  * `Napi::Function` (1. and 2.)
  * `Napi::PropertyDescriptor` (1. for the setter, 2. for the getter)
  * `Napi::InstanceWrap<T>` (3., 4. for instance methods, 4. for
     instance getters)
  * `Napi::ObjectWrap<T>` (1., 2. for static methods, 2. for static
     getters)

In the case of `InstanceWrap<T>` and `ObjectWrap<T>` instance resp.
static property descriptors we can also remove the infrastructure
designed to allow for optional getters (`GetterTag` resp.
`StaticGetterTag`) because the API for specifying instance resp. class
property descriptors does not allow one to omit the getter.

Signed-off-by: Gabriel Schulhof <[email protected]>
PR-URL: nodejs/node-addon-api#786
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
This change reduces the places where we declare private functions with
the `napi_callback` signature for the purpose of using them with C++
callbacks passed as template arguments. We basically have 4 types:

  1. static with `void` return
  2. static with `napi_value` return
  3. instance with `void` return
  4. instance with `napi_value` return

We can use one of these four calling patterns in the following places
where we accept callbacks as template arguments:
  * `Napi::Function` (1. and 2.)
  * `Napi::PropertyDescriptor` (1. for the setter, 2. for the getter)
  * `Napi::InstanceWrap<T>` (3., 4. for instance methods, 4. for
     instance getters)
  * `Napi::ObjectWrap<T>` (1., 2. for static methods, 2. for static
     getters)

In the case of `InstanceWrap<T>` and `ObjectWrap<T>` instance resp.
static property descriptors we can also remove the infrastructure
designed to allow for optional getters (`GetterTag` resp.
`StaticGetterTag`) because the API for specifying instance resp. class
property descriptors does not allow one to omit the getter.

Signed-off-by: Gabriel Schulhof <[email protected]>
PR-URL: nodejs/node-addon-api#786
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
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