napi_call_function (and maybe others) - we assume napi_value passed in is not NULL

[gabrielschulhof]

napi_status napi_call_function(napi_env env,
                               napi_value recv,
                               napi_value func,
                               size_t argc,
                               const napi_value* argv,
                               napi_value* result) {
  NAPI_PREAMBLE(env);

  v8::Isolate* isolate = env->isolate;
  v8::Local<v8::Context> context = isolate->GetCurrentContext();

  v8::Local<v8::Value> v8recv = v8impl::V8LocalValueFromJsValue(recv);
...

What if recv is NULL? I am tempted to allow NULL here, because we forego an additional N-API call just to retrieve a JS undefined as a napi_value. Still, we should not assume that it's not NULL.

[jasongin]

It looks like we're missing calls to both CHECK_ARG(recv) and CHECK_ARG(func) here.

I do agree that it might make sense to allow recv to be null, though in that case I think it should substitute the global object rather than undefined.

napi_make_callback() has the same issues.

[addaleax]

though in that case I think it should substitute the global object rather than undefined.

@jasongin Why? I’ve noticed that node-api does it that way too, and it looks very deliberate, but I’m still confused by that. I would say it’s an antipattern in JavaScript to let this default to global, and I wouldn’t be surprised if this tripped up users that are used to JS strict mode.

[jasongin]

OK, that would be my mistake then. I didn't realize strict mode changed that behavior. (I don't normally write JavaScript code that relies on the default value of this either way.)