Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Porting FunctionTemplate to N-API #92

Closed
mcollina opened this issue Aug 1, 2017 · 10 comments
Closed

Porting FunctionTemplate to N-API #92

mcollina opened this issue Aug 1, 2017 · 10 comments

Comments

@mcollina
Copy link
Member

mcollina commented Aug 1, 2017

How do I port FunctionTemplate to N-API?

v8::Local<v8::FunctionTemplate> tpl = Nan::New<v8::FunctionTemplate>(New);

This is the best I could do be

Napi::FunctionReference tpl = Napi::FunctionReference(env, New);

But it still did not work.

@jasongin
Copy link
Member

jasongin commented Aug 1, 2017

Templates are a V8-specific concept. There is no equivalent in N-API. Just create a Napi::Function directly.

Napi::FunctionReference is equivalent to a v8::Persistent reference to a v8::Function.

@mcollina
Copy link
Member Author

mcollina commented Aug 1, 2017

Templates are a V8-specific concept. There is no equivalent in N-API. Just create a Napi::Function directly.

That is a very common pattern everywhere in native module authoring. It's definitely not clear how to port something like https://github.com/mcollina/native-hdr-histogram/blob/master/hdr_histogram_wrap.cc#L12-L28.

Napi::FunctionReference is equivalent to a v8::Persistent reference to a v8::Function.

The convert.js  automatically replaced my FunctionTemplate with a FunctionReference. Was it wrong?

@jasnell
Copy link
Member

jasnell commented Aug 1, 2017

FunctionTemplate is definitely something that we make fairly extensive use of within core so it's likely going to need to be something N-API supports at some point. Having a Napi::FunctionTemplate would be important, even if there is no concept of it in Chakra-Core or other VMs.

@mcollina
Copy link
Member Author

mcollina commented Aug 1, 2017

Currently I was able to port it to:

 Napi::Function tpl = Napi::Function::New(env, HdrHistogramWrap::New, "HdrHistogram");

Not sure if it's correct, but I was able to move on to the next build error.

@jasongin
Copy link
Member

jasongin commented Aug 1, 2017

This is where we need a conversion guide. The code you linked is defining a class with a bunch of methods. For that, you should definitely use Napi::ObjectWrap and its DefineClass() method. See for example https://github.com/jasongin/node-canvas/blob/napi/src/Canvas.cc#L36 and https://github.com/jasongin/node-canvas/blob/napi/src/Canvas.h#L52

@jasongin
Copy link
Member

jasongin commented Aug 1, 2017

FunctionTemplate is definitely something that we make fairly extensive use of within core so it's likely going to need to be something N-API supports at some point.

We've converted a lot of code so far (which has used v8::FunctionTemplate) and haven't really needed it. The N-API implementation uses it internally where necessary.

@jasongin
Copy link
Member

jasongin commented Aug 1, 2017

The convert.js  automatically replaced my FunctionTemplate with a FunctionReference. Was it wrong?

Maybe not. There's a common pattern where a Napi::FunctionReference can be used instead of a v8::FunctionTemplate, though not in exactly the same way. That is when the constructor function of a class is persisted for later instanceof checks.

@jasnell
Copy link
Member

jasnell commented Aug 1, 2017

Ok, then yeah, I think a conversion guide is definitely needed :-) It appears the case is covered, it's just not immediately clear how. Great work and thank you for the patience as we feel our way around

@mhdawson
Copy link
Member

@mcollina just following up to see if there is still an open question here or you have the info you needed to do the conversion.

@gabrielschulhof
Copy link
Contributor

I think we can close this issue. We have the guide at https://github.com/nodejs/node-addon-api/tree/master/tools.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants