Implement examples for node 4.x.

[tomgco]

Hi all,

I have implemented examples 1-7 that targets and compiles on 4.x using the style guides in the doc's.
I would like some feedback as they are very much like the 0.12 versions but the Isolate's are retrieved from the args / Handles.

Thanks 💃

[rvagg]

💥

/cc @nodejs/addon-api looking for reviewers pls

[bnoordhuis]

You should check that args[0]->IsFunction() before casting it.

EDIT: That should probably be added to the examples for the other node versions as well. The way it's currently written is a ticking time bomb.

[tomgco]

Would the following make sense?

if (!args[0]->IsFunction()) {
  return args.GetReturnValue().SetUndefined();
}

[kkoopa]

Without looking at the surrounding context, I'd say yes. However, the default return value is set to Undefined, so a simple return statement would suffice.

On February 5, 2016 12:23:39 PM GMT+02:00, Tom Gallacher [email protected] wrote:

@@ -0,0 +1,18 @@
+#include <node.h>
+
+using namespace v8;
+
+void RunCallback(const FunctionCallbackInfo& args) {

• Isolate* isolate = args.GetIsolate();

• Local cb = Local::Cast(args[0]);

Would the following make sense?

if (!args[0]->IsFunction()) {
 return args.GetReturnValue().SetUndefined();
}

---

Reply to this email directly or view it on GitHub:
https://github.com/nodejs/node-addon-examples/pull/49/files#r51998512

[tomgco]

Thanks for the feedback @bnoordhuis, does anyone from @nodejs/addon-api have any more feedback? I have updated the commit with your fixes and suggestions and I am going to proceed to work on number 8 and then we should hopefully be ready to rock on!

[tomgco]

Oops, just notice an issue in that last push, 7) doesn't compile, fixing that up.

[tomgco]

Oops, just notice an issue in that last push, 7) doesn't compile, fixing that up.

Fixed in 4d6f2ec

[tomgco]

Ok, 8 has now been added and I believe that is all of the examples so far! 👯

[bnoordhuis]

Not strictly necessary in a construct call, the return value is always 'this'.

[bnoordhuis]

LGTM with suggestions.

[tomgco]

I have removed the calls to args.GetReturnValue().Set(args.This()); in construst calls.

[tomgco]

Hey @nodejs/addon-api, any more LGTM's so we can merge this in?

[rvagg]

what is this file? was this accidentally left in the commit or part of something else?

[rvagg]

@tomgco great work. Would you mind comparing to https://nodejs.org/docs/latest-v4.x/api/addons.html (you can make test-addons to have these extracted and tested in test/addons/ in core btw) to make sure we're not missing any updates that may have gone in to those. I'd be keen to sync up the two versions and ensure we're taking the best of both worlds. i.e. pull in anything from those versions to here and then PR back to nodejs/node/v4.x-staging an update from here.

While there, also have a look at https://nodejs.org/docs/latest-v5.x/api/addons.html because I'm noticing that we have some nice comments inline now (I think it all may have come from nodejs/node#4320 which I apparently participated in and then promptly forgot!). Perhaps you could pull in the best of all worlds for these updates and we can push them back in to a future v4.x release.

[Knighton910]

Great job @tomgco progress is needed with this repo, and this is the way to go.

[eljefedelrodeodeljefe]

I am gonna review and merge this later today. Has anyone changed his / her mind since last viewed?

[Knighton910]

@eljefedelrodeodeljefe
review and merge this later today = LGTM

[4Z4T4R]

Is anyone against forking the origin examples repo and merging all the conflict-free pull requests into that one? OR is there a real need for some fresh eyes to take command of this one as an admin?

[tomgco]

Should I close this PR @eljefedelrodeodeljefe, it is now over a year old and N-API is now almost stable making this redundant?

[mhdawson]

@eljefedelrodeodeljefe I'm going to close this, please let me know fi that is not the right thing to do. 4.x is passed it's LTS support date and the focus is on now on N-API examples.