Add NonConstructor to allow customization of ObjectWrap behavior

[p120ph37]

Addresses #1123 by moving the error-throw into an overridable static method. Default behavior remains exactly the same, but now a developer can override NonConstructor in their ObjectWrap subclass in order to define alternate behavior. As a trivial example of behavior a developer might implement, this code in an ObjectWrap subclass would support the old (ugly) idiom of instantiating a class even when the function is called without the new keyword:

static Napi::Value MyObject::NonConstructor(const Napi::CallbackInfo& info) {
  std::vector<napi_value> args;
  for(size_t i = 0; i < info.Length(); i++) args.push_back(info[i]);
  return GetConstructor(info.Env()).New(args);
}

Which then allows this usage:

const MyObject = binding.MyObject;
const newConstructed = new MyObject();
const factoryConstructed = MyObject();
assert(newConstructed instanceof MyObject);
assert(factoryConstructed instanceof MyObject);

I've also included a new test case, which amounts to basically the above, and also tests that exceptions thrown inside the non-constructor are handled appropriately.

[p120ph37]

I know this is leaky without an env-shutdown-hook, but it's good enough to run the tests without clobbering Napi::Addon<T> via env.SetInstanceData() as would be the typical idiom.

[p120ph37]

Should probably add a comment to the source so nobody thinks it’s a good idea to copy paste this into a real project…

[p120ph37]

I've added an env cleanup-hook so this test-code shouldn't leak anymore and should be adaptable to real-world projects.

[mhdawson]

We discussed in the node-api team meeting today.

We agreed giving developers the option makes sense. We would like the PR to include updating the documentation as well.

We would prefer that instead of using NonConstructor that we use CreateViaFunctionCall

[p120ph37]

Sounds reasonable to me - I'll make the requested adjustments.

[p120ph37]

I'm happy to name it CreateViaFunctionCall if that's the consensus, but I do feel like that's a little bit misleading because the actual action may be something other than creating an object -- that's just one possible implementation for the method. All I intended to convey with my original NonConstructor name was that it handles the case where the new keyword was not used. Absent the new keyword, the function is technically allowed to do anything it wants, including returning an object of some completely different type, or returning a "builder" object, or an object-factory, etc. So calling it Create... seems a little presumptive ;-)

[vmoroz]

I'm happy to name it CreateViaFunctionCall if that's the consensus, but I do feel like that's a little bit misleading because the actual action may be something other than creating an object -- that's just one possible implementation for the method. All I intended to convey with my original NonConstructor name was that it handles the case where the new keyword was not used. Absent the new keyword, the function is technically allowed to do anything it wants, including returning an object of some completely different type, or returning a "builder" object, or an object-factory, etc. So calling it Create... seems a little presumptive ;-)

+@mhdawson and @gabrielschulhof

@p120ph37, it is a great point! There were a few points for the alternative name:

• NonConstructor sounds a little bit alien and does not communicate the function purpose clearly.
• What if we use the same terms as in the JS docs? E.g. in MDN Date they use the phrase When called as a function, returns a string representation of the current date and time .... Thus, we thought that the name should have something from the called as function phrase.
• The method name is better start with a verb.

Since Create... seems to be wrong here (even the Date() call does not create anything), could we use something like HandleWhenCalledAsFunction, OnCalledAsFunction, or something else along these lines? E.g. to communicate that this method handles the situation when the JS function representing the native class was called as a function rather than as a constructor. It could probably help to write the docs about the function first and see which function name represents best what was described in the doc.

[p120ph37]

I like OnCalledAsFunction - it's not too verbose, and the On prefix is already familiar and implies the mechanism of action: the developer should define the method in order to change the default behavior. I'll go ahead with that name first, and we can see what the doc looks like.

[vmoroz]

I like OnCalledAsFunction - it's not too verbose, and the On prefix is already familiar and implies the mechanism of action: the developer should define the method in order to change the default behavior. I'll go ahead with that name first, and we can see what the doc looks like.

I like it too! Though, the final decision is from @mhdawson and @gabrielschulhof.

[mhdawson]

adding my +1 for OnCalledAsFunction

[mhdawson]

LGTM

[mhdawson]

Landed as cee899a