Deprecate old Callback::Call

[ofrobots]

Note: this builds on top of commits from #732. I will rebase after that PR lands.

Now that we have exposed the concept of AsyncResource to Nan::Callback and Nan::AsyncWorker, let's go ahead and deprecate the legacy Callback::Call functions that do not accept a context.

Related: deprecation of legacy MakeCallback in Node: nodejs/node#18632

[kkoopa]

Regarding Callback::GetFunction and its corresponding operator: There have been some cases where someone wants to do a function call without triggering the next tick, i.e. v8::Function::Call, unlike node::MakeCallback (which implicitly triggers the next tick -- as a side note, there ought to be a way of explicitly triggering the next tick -- but I am not aware of one). How do the async contexts fit in here?
Does it make sense to have asynchronous execution without triggering the next tick?

Until now, nan::Callback has (mostly) been a convenience class for persisting a v8::Function reference for whatever purpose. Would this now imply a semantic shift, wherein nan::Callback always is associated with asynchronous execution?

[reqshark]

nan::Callback has (mostly) been a convenience class for persisting a v8::Function reference for whatever purpose

this sounds useful for the need to perform synchronous/blocking operations

Does it make sense to have asynchronous execution without triggering the next tick?

interesting question, but I can tell you it would be unexpected if the Array.map() callback/param triggered next tick

[ofrobots]

@kkoopa @reqshark Those are good points! Thanks for bringing those up.

Until now, nan::Callback has (mostly) been a convenience class for persisting a v8::Function reference for whatever purpose. Would this now imply a semantic shift, wherein nan::Callback always is associated with asynchronous execution?

Yeah, my PR downplays the use of Callback to call functions synchronously, and that is a problem. I was mostly focussing on the more prevailing use-case of calling back asynchronously and some of the dangers there.

The motivation here was that calling asynchronously without providing the async context is problematic, so let's deprecate things to ensure the native module ecosystem can migrate to the 'correct' mechanism. At the same time, it is valid to have native modules that might be using Callback for synchronous calls. Those uses are fine and should be allowed to continue to exist.

Thinking about this some more, Nan provides two mechanisms to do a 'call' the function:

• Nan::Callback::Call and operator(). This is plumbed on top of node::MakeCallback. Existing uses of this are most likely asynchronous callbacks. It is possible that someone using this mechanism for synchronous calls, but those are likely mistakes. I think it is fair for users to get a deprecation warning as long as we provide them an alternative API for synchronous calls.
• Nan::Callback::GetFunction and operator*: These return a v8::Function that can be called synchronously. I added a TODO in the PR claiming that these should be removed. That was an oversight. This is a valid use case. Comparing to Callback::Call, this mechanism to call seems somewhat inconvenient.

Would it make sense to add a CallSync method that does v8::Function::Call directly? Perhaps we can optionally rename Call to CallAsync to clearly separate these use-cases and to add clarity? WDYT?

Does it make sense to have asynchronous execution without triggering the next tick?

I think that is a good question, but perhaps we should discuss this in a separate thread? It is a use-case that is currently unsupported.

[kkoopa]

Would it make sense to add a CallSync method that does v8::Function::Call directly? Perhaps we can optionally rename Call to CallAsync to clearly separate these use-cases and to add clarity? WDYT?

Yes, that could work. I believe it was suggested in the original ticket that wanted callbacks without ticks.
I think I decided against it then because the use case was fairly rare, Callback::GetFunction already existed and I did not and still do not like the division between synchronous calls, asynchronous calls, ticks, and no ticks. I am actually myself not entirely sure about what does what when, which is why I find it hard to believe that it is common knowledge. node::makeCallback does (or did) the correct things in most cases, which mainly is why it is the default.

I would also not be fully opposed to making nan::Callback an asynchronous class and having another synchronous class if that would help, but I think that might be too drastic.

[kkoopa]

I now remembered that v8::Function::Call is wrapped by Nan::Call. Maybe Nan::Call could get a set of overloads for Nan::Callback?

[ofrobots]

@kkoopa I've added a commit (f0b8f6e) that adds a Nan::Callback overload to Nan::Call. Is that what you had in mind?

[kkoopa]

Yes, that is precisely what I had in mind. This way it becomes more clear that Nan::Call is responsible for calling function-like objects synchronously without triggering the next tick, while Callback::Call is for the asynchronous case which triggers next tick and sets up async, although I think it should take a pointer to const. However, I wonder whether pointer or reference is the way to go. I can think of reasons for both, but currently reference seems more natural. Any thoughts on that?

[ofrobots]

I copied how AsyncWorker was doing it. But, const reference is definitely better. Done.

[kkoopa]

Great! This looks good to me. I'll merge it unless someone chimes in with more comments in a couple of days.