Awesome - thanks for putting this together! <3

@nodejs/n-api … how would one feature-detect this? Should we provide that?

That would be good - either that or it can be part of the standard API for napi version 5 and later.

@addaleax you could simply assume that passing func is optional and, if napi_create_threadsafe_function() returns napi_invalid_arg, you would know that you're on a version of N-API where func is mandatory.

I am curious as to the semverity of this change, though. If it's semver-minor, we need to go through the whole experimental life cycle, but I'm not sure how we would do that for a semantic change like this, rather than the clean addition of a new N-API.

@legendecas please also modify https://nodejs.org/docs/latest/api/n-api.html#n_api_napi_threadsafe_function_call_js by mentioning for the js_callback parameter that it can also be NULL if the thread-safe function was created without a JS callback.

@addaleax you could simply assume that passing func is optional and, if napi_create_threadsafe_function() returns napi_invalid_arg, you would know that you're on a version of N-API where func is mandatory.

Right, but you usually want to support as many versions as possible, so you’d still end up always providing func, because that’s the only thing you can practically do to resolve napi_invalid_arg, besides maybe erroring out, right?

If it's semver-minor, we need to go through the whole experimental life cycle

Is that a N-API-specific rule that’s written down somewhere? I would consider this semver-minor, but I agree that it doesn’t make sense to label this as experimental. (Fwiw, I think none of the N-API functions currently marked as experimental should be experimental, except maybe the threadsafe function feature due to its complexity.)

@addaleax you could simply assume that passing func is optional and, if napi_create_threadsafe_function() returns napi_invalid_arg, you would know that you're on a version of N-API where func is mandatory.

Right, but you usually want to support as many versions as possible, so you’d still end up always providing func, because that’s the only thing you can practically do to resolve napi_invalid_arg, besides maybe erroring out, right?

Well, if you get napi_invalid_arg, and knowing that the call should succeed, you would call again with a non-NULL func. But, of course, passing a dummy func is simplest.

My take is that this change is more about making napi_threadsafe_function into an async queue, which would clean up code that builds upon it, than it is about what to do with func.

If it's semver-minor, we need to go through the whole experimental life cycle

Is that a N-API-specific rule that’s written down somewhere? I would consider this semver-minor, but I agree that it doesn’t make sense to label this as experimental. (Fwiw, I think none of the N-API functions currently marked as experimental should be experimental, except maybe the threadsafe function feature due to its complexity.)

Well, the rule concerns new N-APIs. This isn't strictly a new N-API 🙂 But yeah, a semver-minor bump sounds like the way to go.

I think we can be flexible on going through an experimental cycle. For new functions, I think it makes sense and we should stick to that. In this case I think we'll get agreement that we should just do this immediately. My preference would be to make the change, and bump the N-API version in the same PR. In that way you will know you can depend on the parameter being optional if it is version 5 or higher. We might also want to review if we should promote any other functions out of experimental at the same time as we bump the version number (being careful to think about whether those other changes can be backported in case we want to backport version 5 support to 10.x and 8.x)

Not actually requesting changes. On my part the PR is approved as is, however, I'm marking it as "Request changes" so as to prevent it from landing before we've had a chance to co-ordinate what else should be added to N-API 5.

@gabrielschulhof Alternatively, you could approve it but add one of the blocked or wip labels. (But if a Request Changes is the best workflow for you, then by all means, do that.)

CI: https://ci.nodejs.org/job/node-test-pull-request/24016/

@nodejs/build can you please help me out here? The one failing job is from a different build. I don't get how come https://ci.nodejs.org/job/node-test-pull-request/24016/ didn't replace the job from https://ci.nodejs.org/job/node-test-pull-request/23975/.

@nodejs/build can you please help me out here? The one failing job is from a different build. I don't get how come https://ci.nodejs.org/job/node-test-pull-request/24016/ didn't replace the job from https://ci.nodejs.org/job/node-test-pull-request/23975/.

You can ignore the GitHub widget if it conflicts with what the actual Jenkins CI is telling you.

Landed in 5705d7b

@Trott thanks! Will do in the future.