Add support for callback scopes

[mhdawson]

Now that nodejs/node#18089 has landed we should add equivalent support in node-addon-api.

[romandev]

I'll take this issue.

[mhdawson]

@romandev thanks !

[romandev]

@mhdawson,

Sorry for delayed work. I was on a vacation until last week. I'm looking into this issue now but need your confirmation.

Shouldn't we need to implement the async hooks wrapper(e.g. napi_async_init) first before implementing this feature? I found the related comments in the PR[1] but didn't find any follow-up change in C++ wrapper side. Do you have a plan to make a followup change? Otherwise, I'm available.

[1] #140

[mhdawson]

@romandev that is a very good point. If you can work on adding async hooks wrapper that would be great :)

[ebickle]

Is there any way to use callback scopes from a napi_async_work? Normally an async worker would create it's Javascript values from within the scope of the complete_callback, but there's an issue right now around error handling - there's no way to return a Javascript error from the execute_callback, just a string or manual workarounds.

Noticed the comments here mentioning an async hooks wrapper for napi_async_init; might be another use-case worth thinking about.

[rolftimmermans]

What's the story for new features like this when running precompiled libraries on older versions of Node? Would this library provide a backport for older Node.js versions or would a library depending on nodejs/node#18089 have to target Node.js 9.6.0+ only?

[mhdawson]

@rolftimmermans, in general not all new features would be backported to earlier versions. However, I am hoping that we will backport what becomes non-experimental in 10.x to 8.x (and we'll have to look at 6.x) in order to accelerate initial adoption.

The complication is that not all versions of 8.x will have the feature, is that something you can work around by requiring Node.js 8.X or higher ?

[mhdawson]

@ebickle I have a todo to look at the error issue with complete_callback, but I'm going to be travelling a fair amount over the next 3 weeks so I'm not quite sure how/when I'm going to get to it.

[rolftimmermans]

@mhdawson I guess it is possible to depend on a certain 8.X version, but it's less than ideal. The feature was introduced in 9.6 it seems, so not sure if the specific 8.X version would matter?

[mhdawson]

The issue is that if/when we backport, it will go into the latest 8.X release. So it would be available in 8.Y or later and 9.6 and later.

[rolftimmermans]

The issue is that if/when we backport, it will go into the latest 8.X release. So it would be available in 8.Y or later and 9.6 and later.

I see. It's probably less than ideal because a lot of distros have already shipped with Node 8. So people who can easily install a future 8.X version can probably also install 9 or 10.

[rolftimmermans]

The following code is how I implemented a C++ wrapper in my code currently. Maybe this is useful for someone looking to create a similar wrapper in this project?

#define NAPI_THROW_IF_FAILED(env, status, ...)                                   \
    if ((status) != napi_ok) {                                                   \
        Napi::Error::New(env).ThrowAsJavaScriptException();                      \
        return;                                                                  \
    }

struct CallbackScope {
    napi_env env;
    napi_handle_scope handle_scope;
    napi_callback_scope callback_scope;

    CallbackScope(napi_env env) : env(env) {
        napi_status status;
        napi_async_context context;
        napi_value resource_name, resource_object;

        status = napi_open_handle_scope(env, &handle_scope);
        NAPI_THROW_IF_FAILED(env, status);

        status = napi_create_string_utf8(env, "zmq", NAPI_AUTO_LENGTH, &resource_name);
        NAPI_THROW_IF_FAILED(env, status);

        status = napi_create_object(env, &resource_object);
        NAPI_THROW_IF_FAILED(env, status);

        status = napi_async_init(env, resource_object, resource_name, &context);
        NAPI_THROW_IF_FAILED(env, status);

        status = napi_open_callback_scope(env, resource_object, context, &callback_scope);
        NAPI_THROW_IF_FAILED(env, status);
    }

    ~CallbackScope() {
        napi_status status;
        status = napi_close_callback_scope(env, callback_scope);
        NAPI_THROW_IF_FAILED(env, status);

        status = napi_close_handle_scope(env, handle_scope);
        NAPI_THROW_IF_FAILED(env, status);
    }
};

[mhdawson]

@rolftimmermans thanks.

@romandev FYI, in case you are first to get to working on adding support to the wrapper for callback scopes.

[NickNaso]

Hi everyone I think that this issue could be close when we will land this PR #362 Is it right?

[NickNaso]

@mhdawson could we close this issue now?

[mhdawson]

Closing

[NickNaso]

I'm closing this, we just released version 1.6.0 that contains the Napi::CallbackScope. Feel free to reopen if I misunderstood something.