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

FunctionReference is missing cargs overloads for Call and MakeCallback #320

Closed
Fishrock123 opened this issue Aug 23, 2018 · 4 comments
Closed
Assignees

Comments

@Fishrock123
Copy link

https://nodejs.github.io/node-addon-api/class_napi_1_1_function_reference.html

FunctionReference doesn't have these but Function does. Seems strange. cargs are easier for not very C++ affluent people like me to use.

@Fishrock123 Fishrock123 changed the title FunctionReference is missing a cargs overloads for Call and MakeCallback FunctionReference is missing cargs overloads for Call and MakeCallback Aug 23, 2018
@NickNaso
Copy link
Member

@gabrielschulhof @mhdawson I think that we can add the method as proposed by @Fishrock123. In the specific we should add these in napi.h :

Napi::Value Call(napi_value recv, size_t argc, const napi_value* args) const;

Napi::Value MakeCallback(napi_value recv, size_t argc, const napi_value* args) const;

and these on napi-inl.h:

inline Napi::Value FunctionReference::Call(
    napi_value recv, size_t argc, const napi_value* args) const {
  EscapableHandleScope scope(_env);
  Napi::Value result = Value().Call(recv, argc, args);
  if (scope.Env().IsExceptionPending()) {
    return Value();
  }
  return scope.Escape(result);
}

inline Napi::Value FunctionReference::MakeCallback(
    napi_value recv, size_t argc, const napi_value* args) const {
  EscapableHandleScope scope(_env);
  Napi::Value result = Value().MakeCallback(recv, argc, args);
  if (scope.Env().IsExceptionPending()) {
    return Value();
  }
  return scope.Escape(result);
}

if you agree I can work on it and at the same time start to create tests about FunctionReference.

@mhdawson
Copy link
Member

Adding those make sense to me. Makes it more consistent with the methods we have on Function.

@NickNaso NickNaso self-assigned this Aug 29, 2018
@NickNaso
Copy link
Member

OK I will take this.

@NickNaso
Copy link
Member

I'm closing the issue because the requested feature is landed with #344

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants