Skip to content

Add NonConstructor to allow customization of ObjectWrap behavior#1125

Closed
p120ph37 wants to merge 4 commits intonodejs:mainfrom
p120ph37:issue-1123
Closed

Add NonConstructor to allow customization of ObjectWrap behavior#1125
p120ph37 wants to merge 4 commits intonodejs:mainfrom
p120ph37:issue-1123

Conversation

@p120ph37
Copy link
Copy Markdown
Contributor

@p120ph37 p120ph37 commented Jan 11, 2022

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
p120ph37 commented Jan 11, 2022
View reviewed changes
Comment thread test/objectwrap_nonconstructor.cc Outdated
}

static std::unordered_map<napi_env, Napi::FunctionReference*> constructors;

Copy link
Copy Markdown
Contributor Author

@p120ph37 p120ph37 Jan 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@p120ph37 p120ph37 Jan 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@p120ph37 p120ph37 Feb 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@p120ph37 p120ph37 force-pushed the issue-1123 branch 4 times, most recently from 5d75653 to 1d1ad35 Compare January 11, 2022 23:32
@p120ph37 p120ph37 mentioned this pull request Jan 12, 2022
Closed
@mhdawson
Copy link
Copy Markdown
Member

mhdawson commented Feb 4, 2022

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
Copy link
Copy Markdown
Contributor Author

p120ph37 commented Feb 4, 2022

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

@p120ph37
Copy link
Copy Markdown
Contributor Author

p120ph37 commented Feb 4, 2022

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
Copy link
Copy Markdown
Member

vmoroz commented Feb 4, 2022 •
edited
Loading

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
Copy link
Copy Markdown
Contributor Author

p120ph37 commented Feb 4, 2022 •
edited
Loading

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
Copy link
Copy Markdown
Member

vmoroz commented Feb 4, 2022

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
Copy link
Copy Markdown
Member

mhdawson commented Feb 7, 2022

adding my +1 for OnCalledAsFunction

mhdawson
mhdawson approved these changes Feb 17, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

mhdawson pushed a commit that referenced this pull request Feb 17, 2022
@p120ph37 @mhdawson
src: allow customization of ObjectWrap behavior
cee899a 
Add new method to allow customization of ObjectWrap behavior

PR-URL: #1125
Reviewed-By: Michael Dawson <midawson@redhat.com
@mhdawson
Copy link
Copy Markdown
Member

mhdawson commented Feb 17, 2022

Landed as cee899a

@mhdawson mhdawson closed this Feb 17, 2022
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
@kevindavies8
src: allow customization of ObjectWrap behavior
ce01870 
Add new method to allow customization of ObjectWrap behavior

PR-URL: nodejs/node-addon-api#1125
Reviewed-By: Michael Dawson <midawson@redhat.com
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
@Marlyfleitas
src: allow customization of ObjectWrap behavior
4576a8e 
Add new method to allow customization of ObjectWrap behavior

PR-URL: nodejs/node-addon-api#1125
Reviewed-By: Michael Dawson <midawson@redhat.com
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
@wroy7860
src: allow customization of ObjectWrap behavior
9064e5b 
Add new method to allow customization of ObjectWrap behavior

PR-URL: nodejs/node-addon-api#1125
Reviewed-By: Michael Dawson <midawson@redhat.com
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
src: allow customization of ObjectWrap behavior
8e9b5ad 
Add new method to allow customization of ObjectWrap behavior

PR-URL: nodejs/node-addon-api#1125
Reviewed-By: Michael Dawson <midawson@redhat.com
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mhdawson mhdawson mhdawson approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@p120ph37 @mhdawson @vmoroz