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

update to nan v2 (adds support for iojs 3) #324

Merged
merged 1 commit into from
Aug 12, 2015
Merged

Conversation

defunctzombie
Copy link
Collaborator

Nan v2 required more code changes but also adds support for iojs 3 and
hopefully provides a more stable API which will not have to change as
much going forward.

@defunctzombie
Copy link
Collaborator Author

Travis build is failing on iojs v3 but this seems to be unrelated to this repo and caused by errors from the v8 code. Maybe a stricter compiler or some other changes. It builds on my OSX machine so it could also be linux specific. If anyone has a linux machine locally, please give this PR a try on a few versions of node and iojs.

@ruimarinho
Copy link

Working fine here, iojs v3.0.0 on OSX (El Cap). Thank you!

@jbergstroem
Copy link

@defunctzombie iojs requires gcc 4.8.x or newer (v8 is c++11 only nowadays). People in this issue seems to have gone through the same pains.

Changes LGTM.

@defunctzombie
Copy link
Collaborator Author

Sigh. Why doesn't travis use the correct gcc when you specify iojs :(

Seems like we need to file a new ticket with them saying iojs-v3 support is incomplete because no native modules can build.

@jbergstroem
Copy link

@defunctzombie thinking it might rather be choice of distro than jenkins being conservative. Anyway, seems to be a few one-liners to getting 4.8.x running at travis in that issue.

Nan v2 required more code changes but also adds support for iojs 3 and
hopefully provides a more stable API which will not have to change as
much going forward.
defunctzombie added a commit that referenced this pull request Aug 12, 2015
update to nan v2 (adds support for iojs 3)
@defunctzombie defunctzombie merged commit 1c9a3b3 into master Aug 12, 2015
@defunctzombie defunctzombie deleted the update-nan branch August 12, 2015 21:54
@kkoopa
Copy link

kkoopa commented Aug 13, 2015

While this compiles for now, you still want to change all uses of v8::Value::ToXXX and v8::Value::XXXValue to Nan::To<XXX> https://github.com/nodejs/nan/blob/master/doc/converters.md#api_nan_to, since the former will be removed from V8 in coming versions.

I would also recommend Nan::Utf8String as a drop-in replacement of v8::String::Utf8Value, which handles invalid UTF-8 in a consistent way.

@defunctzombie
Copy link
Collaborator Author

@kkoopa can you give an example from this codebase of Nan::To use. The nan docs are not so clear to me how I would update the following:

const ssize_t rounds = info[0]->Int32Value();

My first stab at it was

const ssize_t rounds = Nan::To<ssize_t>(info[0]).FromJust();

but this doesn't build.

@kkoopa
Copy link

kkoopa commented Aug 17, 2015

Only the listed conversions are available. While int64_t is the closest to ssize_t, the original used int32_t.

@defunctzombie
Copy link
Collaborator Author

ssize_t depends on architecture iirc

So what would be the API to do the above conversion. Examples/docs are
lacking and/or unclear to me how when I would use these and how to work
with maybe objects.

Do I need to consult v8 maybe object docs?

On Monday, August 17, 2015, Benjamin Byholm [email protected]
wrote:

Only the listed conversions are available. While int64_t is the closest
to ssize_t, the original used int32_t.


Reply to this email directly or view it on GitHub
#324 (comment)
.

@kkoopa
Copy link

kkoopa commented Aug 17, 2015

They won't say more than what's written at https://github.com/nodejs/nan/blob/master/doc/maybe_types.md#api_nan_maybe

Use Nan::Maybe<>::FromJust if you are certain that it is valid, otherwise, use Nan::Maybe<>::FromMaybe with a suitable default value.

The above conversion is done as
const ssize_t rounds = Nan::To<int32_t>(info[0]).FromMaybe(0);

@defunctzombie
Copy link
Collaborator Author

Thanks, I will give that a try.

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

Successfully merging this pull request may close these issues.

5 participants