Make callback optional in ThreadSafeFunction::New

[seishun]

Napi::ThreadSafeFunction::New has a required Napi::Function parameter which I don't need in my use case. I can't use Napi::TypedThreadSafeFunction since I need to use a different callback across calls to [Non]BlockingCall().

https://github.com/nodejs/node-addon-api/blob/48e6b584a396ee5c42ef971c216139ad454f478b/doc/threadsafe.md states the following regarding Napi::ThreadSafeFunction:

This API is designed without N-API 5 native support for the optional JavaScript function callback feature. ::New methods that do not have a Function parameter will construct a new, no-op Function on the environment to pass to the underlying N-API call.

However, this doesn't appear to be the case.

I will gladly make a PR if this is something you're open to.

[KevinEady]

Hi @seishun ,

Thank you for pointing this out. The implementation for the old TSFN was actually not modified in #742 , so I would say that actually this documentation is incorrect and needs to be updated, removing your bolded statement. Not sure how that slipped in there, maybe an erroneous edit for the typed variety. It was never my intention of modifying the old API.

@mhdawson and team, we can either:

• update the docs, or
• update the old ThreadSafeFunction::New constructors to match the docs

Thoughts?

Thanks, Kevin

[mhdawson]

@KevinEady unless there is a reason you don't think these make sense:

1. if we used to accept calls without the Function parameter (irrespective of whether was changed in your PR, just if we had changed it by accident at some point) then I think we should definitely update the code to support that.

2. If it was simply a doc problem since the beginning, then I think we should still consider updating the ThreadSafeFunction::New to match the docs. It seems like a non-breaking change to existing code and has use cases (as shown in this issue).

[NickNaso]

@KevinEady @mhdawson may I proceed for the new release or we need to fix this issue?

[mhdawson]

I can see that in d092a32 the text was moved from its original place into the common one. I also see from

node-addon-api/napi.h

Line 2260 in 48e6b58

// This API may only be called from the main thread.

that we document that behavior for the Typed version but not the original:

I think for now we should fix up the documentation to remove that comment for the non-typed case

node-addon-api/napi.h

Line 2046 in 48e6b58

#if (NAPI_VERSION > 3 && !defined(__wasm32__))

Therefore I think we should

1. Fix the documentation to remove the comment for the non-typed version to match the intention
2. Consider a follow on PR to add that functionality in a non-breaking way if possible (note that it needs to work back to the N-API version that the original ThreadSafeFunction was added.

@NickNaso in terms of the release it would be good to fix up the doc to remove the text before doing the release.

[KevinEady]

Hi @mhdawson , @NickNaso ,

I have created #860

[mhdawson]

@KevinEady thanks, landed that. @NickNaso think we should be good to go for the release now.

[github-actions]

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.