workshopper / goingnative

A NodeSchool style workshopper for learning how to write native Node.js addons
MIT License
414 stars 54 forks source link

Update to Nan 2.x #60

Closed SomeoneWeird closed 9 years ago

SomeoneWeird commented 9 years ago

I think I found everything that needed changing. Not sure what we want to do about explaining MaybeLocal vs Local, probably needs a sentence or two when we describe how Nan::New works initially?

Also if there are any style things you want me to change etc just lemme know.

R=@rvagg

julianduque commented 9 years ago

@rvagg this looks good to me

ChALkeR commented 9 years ago

Adding to the list: https://github.com/nodejs/node/issues/2798. Btw, this PR fixes #59.

SomeoneWeird commented 9 years ago

@kkoopa Thanks! I'll fix them up tomorrow when I get a chance :+1:

rvagg commented 9 years ago

sorry I haven't had a chance to review this, if @kkoopa is happy then I am, @julianduque has publish rights to push a new one out

julianduque commented 9 years ago

@SomeoneWeird let me know when changes from @kkoopa are ready and I'll bump and publish

rvagg commented 9 years ago

@julianduque would you mind adding "Adam Brady" (a.k.a. @SomeoneWeird) to both the README and credits.txt before publishing?

SomeoneWeird commented 9 years ago

@kkoopa Okay, I'm not really up on the best practice for what you should do about checking MaybeLocals. I pushed a commit that changes call me maybe to use Nan::To, if you have any comments on how I should format it etc would be appreciated. If it looks alright, i'll change the others and squash everything so we can merge.

kkoopa commented 9 years ago

The NAN docs already explain what's what. https://github.com/nodejs/nan/blob/master/doc/maybe_types.md

Best practice is: Use ToLocalChecked if the handle can be guaranteed not to be empty, otherwise use FromMaybe or ToLocal.

SomeoneWeird commented 9 years ago

Okay I pushed another commit, if you can't tell I barely know C++, so any help would be appreciated. Is something like this better?

kkoopa commented 9 years ago

Looking better, some places should still use Nan::To<>, the rest seems fine.

SomeoneWeird commented 9 years ago

@kkoopa When changing void Init(Handle<Object> exports) { to NAN_MODULE_INIT(Init) { I seem to get

../myaddon.cc:10:12: error: use of undeclared identifier 'exports'
  Nan::Set(exports, Nan::New("print").ToLocalChecked(),

Am I doing something wrong?

kkoopa commented 9 years ago

s/exports/target/

SomeoneWeird commented 9 years ago

Alright, how does this look?

kkoopa commented 9 years ago

Apart from that one letter documentation fix, it looks good to me.

SomeoneWeird commented 9 years ago

@kkoopa Awesome, thank you so much for your help :+1:

@julianduque Squashed, merge at will.

julianduque commented 9 years ago

Awesome work @SomeoneWeird @kkoopa :)

ChALkeR commented 9 years ago

59 should be closed now.

SomeoneWeird commented 9 years ago

@julianduque I just fixed an issue in e07cbab618a4d4c0e4b5408e3411ddbf61b259ba, are you able to publish 2.0.1?

julianduque commented 9 years ago

@SomeoneWeird published