Skip to content

node 4x retry #74

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

Closed
wants to merge 3 commits into from
Closed

node 4x retry #74

wants to merge 3 commits into from

Conversation

erulabs
Copy link

@erulabs erulabs commented Sep 28, 2015

@erulabs
Copy link
Author

erulabs commented Sep 28, 2015

@sonnyp Maybe you can take a peek at this one? Should be a lot closer.

I think all the Nan2 stuff is working - but the core tests are failing :/

@erulabs erulabs mentioned this pull request Sep 28, 2015
Closed
@erulabs
Copy link
Author

erulabs commented Sep 29, 2015

Hey @sonnyp

Well, I'm close. Very close. Can't figure how why it's not throwing exceptions for fallback JS profiles properly...

I've got to stop working on this now (ran out of time). HUUUGE thanks goes out to anyone who spends some time on this for me :)

Thanks!

@@ -7,7 +7,7 @@ var UIDNA_ALLOW_UNASSIGNED = 1
var UIDNA_USE_STD3_RULES = 2

try {
var bindings = require('bindings')('node_stringprep.node')
var bindings = require('./node_modules/bindings/bindings.js')('node_stringprep.node')
Copy link
Contributor

Choose a reason for hiding this comment

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

whoa, no, why was this changed?

@rvagg
Copy link
Contributor

rvagg commented Sep 29, 2015

You should be able to replace all references to Handle with Local for maximum compatibility

@rvagg
Copy link
Contributor

rvagg commented Sep 29, 2015

Handle<Value> prepare(String::Value &str) should have a Nan::HandleScope scope; otherwise it'll leak data (looks like it has been till now anyway) and in this case it should be a Nan::EscapableHandleScope scope because you're returning a value from it.

@rvagg
Copy link
Contributor

rvagg commented Sep 29, 2015

NanThrowError(errorName());
return NanUndefined();
Nan::ThrowError(u_errorName(error));
return Nan::Undefined();
Copy link
Contributor

Choose a reason for hiding this comment

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

this is the scope.Escape(Nan::Undefined())

@rvagg
Copy link
Contributor

rvagg commented Sep 29, 2015

I did a write-up @ https://nodesource.com/blog/cpp-addons-for-nodejs-v4 about the upgrade process that's a bit more verbose than other resources currently available

@sonnyp
Copy link
Contributor

sonnyp commented Sep 29, 2015

closing this in favor of #72

@erulabs thanks!

@sonnyp sonnyp closed this Sep 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants