doc: Updating addons examples #5362

tomgco · 2016-02-22T12:19:13Z

Add more explicit error checking in c++
Specific namespaces in headers
const in JS examples instead of var

cc\ @nodejs/documentation

This is based on the work that is being carried out in nodejs/node-addon-examples#49.

indutny · 2016-02-22T20:42:17Z

LGTM if CI is green.

indutny · 2016-02-22T20:42:33Z

CI: https://ci.nodejs.org/job/node-test-pull-request/1726/

eljefedelrodeodeljefe · 2016-02-22T21:14:11Z

doc/api/addons.markdown

@@ -767,7 +793,7 @@ void MyObject::New(const FunctionCallbackInfo<Value>& args) {
    double value = args[0]->IsUndefined() ? 0 : args[0]->NumberValue();
    MyObject* obj = new MyObject(value);
    obj->Wrap(args.This());
-    args.GetReturnValue().Set(args.This());
+    return;


Why only return in c++?

This is fine.

Ah, actually. I see your point. No need in return here!

Can remove, I personally like returning early.

Please remove it, I think it is confusing.

Fine for me anyhow. First I was wondering though why args.GetReturnValue().Set(args.This()); is not necessary anymore, but I may have missed API changes.

Fine for me anyhow. First I was wondering though why args.GetReturnValue().Set(args.This()); is not necessary anymore, but I may have missed API changes.

Same here, but nodejs/node-addon-examples#49 (comment) says that with a construct call the return value is always this.

Please remove it, I think it is confusing.

Sure :]

Ah, very interesting. Thanks for pointing at this. If it makes sense, have the content of Bens comment in the docs explicitly.

eljefedelrodeodeljefe · 2016-02-22T21:20:38Z

~~Looks good. I am not the biggest fan of having using node::ObjectWrap; and implicit namespaces for V8, but I am no real c++ dev though. Also it was inconsistent before, right?~~ LGTM minus discussion about namespaces

bnoordhuis · 2016-02-22T22:53:11Z

Specific namespaces in headers

But why?

tomgco · 2016-02-23T10:25:13Z

But why?

To keep it consistent with the .cc files, it seems strange to me that we switch between implicit in one file and not in another.

rvagg · 2016-02-23T10:45:41Z

doc/api/addons.markdown

-console.log(obj1.msg+' '+obj2.msg); // 'hello world'
+const obj1 = addon('hello');
+const obj2 = addon('world');
+console.log(obj1.msg + ', ' + obj2.msg); // 'hello world'


as long as we're going const lets go console.log(${obj1.msg}, ${obj2.msg});

then 'strict mode' too I suppose ..

Actually, console.log takes variable arguments so console.log(obj1.msg, obj2.msg) would also work.

rvagg · 2016-02-23T10:47:32Z

lgtm pending the various points above being sorted out

* Add more explicit error checking in c++ * Specific namespaces in headers * `const` in JS examples instead of `var`

thefourtheye · 2016-02-23T12:34:11Z

doc/api/addons.markdown

@@ -345,9 +345,12 @@ using v8::Value;

 void RunCallback(const FunctionCallbackInfo<Value>& args) {
  Isolate* isolate = args.GetIsolate();
+  if (!args[0]->IsFunction()) {
+    return;


Shouldn't we throw here?

I was thinking the same thing since it's done below, but in terms of a learning experience it may be better to introduce these things at a slower pace? I don't mind either way tbh.

jasnell · 2016-03-02T17:09:26Z

LGTM ... looks like the PR may be to be rebased tho.

benjamingr · 2016-03-16T12:27:37Z

Ping @tomgco - are you still pursuing this?

addaleax · 2016-05-17T22:36:43Z

ping @tomgco again

jasnell · 2017-02-28T23:13:27Z

Closing given the lack of forward progress on this

mscdex added c++ Issues and PRs that require attention from people who are familiar with C++. doc Issues and PRs related to the documentations. labels Feb 22, 2016

tomgco force-pushed the addons-docs branch from c987388 to 6a3d5be Compare February 22, 2016 12:21

eljefedelrodeodeljefe reviewed Feb 22, 2016
View reviewed changes

rvagg reviewed Feb 23, 2016
View reviewed changes

doc: Updating addons examples

a1721a8

* Add more explicit error checking in c++ * Specific namespaces in headers * `const` in JS examples instead of `var`

tomgco force-pushed the addons-docs branch from 6a3d5be to a1721a8 Compare February 23, 2016 11:05

thefourtheye reviewed Feb 23, 2016
View reviewed changes

jasnell added the stalled Issues and PRs that are stalled. label Apr 19, 2016

estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:23

Trott force-pushed the master branch from b0df363 to c5ce7f4 Compare September 21, 2016 00:09

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:01

MylesBorins force-pushed the master branch from 8df7ee0 to 54fef67 Compare February 1, 2017 01:00

jasnell closed this Feb 28, 2017

Uh oh!

doc: Updating addons examples #5362

doc: Updating addons examples #5362

Uh oh!

Conversation

tomgco commented Feb 22, 2016

Uh oh!

indutny commented Feb 22, 2016

Uh oh!

indutny commented Feb 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eljefedelrodeodeljefe commented Feb 22, 2016

Uh oh!

bnoordhuis commented Feb 22, 2016

Uh oh!

tomgco commented Feb 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rvagg commented Feb 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Mar 2, 2016

Uh oh!

benjamingr commented Mar 16, 2016

Uh oh!

addaleax commented May 17, 2016

Uh oh!

jasnell commented Feb 28, 2017

Uh oh!

Uh oh!