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

Upgrade NAN to v2 #580

Closed
wants to merge 1 commit into from
Closed

Upgrade NAN to v2 #580

wants to merge 1 commit into from

Conversation

rvagg
Copy link
Contributor

@rvagg rvagg commented Jul 13, 2015

No description provided.

@rvagg
Copy link
Contributor Author

rvagg commented Jul 13, 2015

Sorry, empty description meant to say:

NAN v2 isn't released yet so this is a WIP, not intended to be merged. NAN v2 is a major rewrite that is forward compatible to V8 4.5 so it support io.js v3 which is in RC at the moment while we get NAN v2 out (because everything is broken thanks to V8 43 and 4.4). This version of NAN should also give us automatic support for Node.js v4 (most likely version, could be v5 though) when it comes out in a few months.

I'll update here when NAN is released and this is ready to merge. There's currently a bug holding this up from passing all the tests and NAN v2 isn't in npm yet either.

@rvagg
Copy link
Contributor Author

rvagg commented Jul 19, 2015

../src/PixelArray.cc: In static member function ‘static Nan::NAN_METHOD_RETURN_TYPE PixelArray::New(Nan::NAN_METHOD_ARGS_TYPE)’:
../src/PixelArray.cc:69:16: error: ‘class v8::Object’ has no member named ‘SetIndexedPropertiesToPixelData’
   info.This()->SetIndexedPropertiesToPixelData(
                ^
../src/PixelArray.cc: In static member function ‘static Nan::NAN_GETTER_RETURN_TYPE PixelArray::GetLength(v8::Local<v8::String>, Nan::NAN_GETTER_ARGS_TYPE)’:
../src/PixelArray.cc:83:59: error: ‘class v8::Object’ has no member named ‘GetIndexedPropertiesPixelDataLength’
   info.GetReturnValue().Set(Nan::New<Number>(info.This()->GetIndexedPropertiesPixelDataLength()));

The IndexedProperties*PixelData*() API is gone in V8 4.4 (and maybe 4.3?) so it doesn't compile on io.js next (3.0.0 RC). @kkoopa, @bnoordhuis, @trevnorris anyone got any suggestions here? I've got no idea what to replace this with.

@bnoordhuis
Copy link

There is no direct replacement. You could attach a node::Buffer or v8::TypedArray as a hidden property. Typed arrays won't work with v0.10 though, a Buffer is more portable.

@trevnorris
Copy link

@bnoordhuis Though that would present a minor change in API, since the CanvasPixelArray is clamped, but Uint8Array is not.

@bnoordhuis
Copy link

Good point. You'd have to use v8::Uint8ClampedArray in that case and guard with #ifdefs.

@rvagg
Copy link
Contributor Author

rvagg commented Jul 20, 2015

OK, this is getting a bit out of hand for what I have time for so I'm going to have to put this on the backburner and ask that anyone here who wants node-canvas to work with io.js 3.0 and beyond (including the next Node) can pick up from this branch if they want and work with that.

@micky2be
Copy link

Looking forward

@rvagg
Copy link
Contributor Author

rvagg commented Aug 18, 2015

this needs help from someone with the time to grok what's required in order to bring back the support that was lost with the removal of the relevant V8 APIs mentioned above

@kkoopa
Copy link
Contributor

kkoopa commented Aug 18, 2015

It's being done. #604

@ChALkeR
Copy link

ChALkeR commented Sep 12, 2015

Adding to the list: nodejs/node#2798.

@LinusU
Copy link
Collaborator

LinusU commented Sep 26, 2015

Superseded by another pull request that got merged

@LinusU LinusU closed this Sep 26, 2015
@mathiasbynens
Copy link
Contributor

Superseded by another pull request that got merged

For future reference, that PR was #622.

@LinusU
Copy link
Collaborator

LinusU commented Sep 26, 2015

Thank you 👍

@LinusU LinusU deleted the nan-2 branch April 24, 2016 17:09
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.

8 participants