0.9 WIP

[rvagg]

Targeting 0.11.13, or before if we're comfortable with the stability.

Replaces #64

[kkoopa]

Removed:

• NanInitPersistent
• NanPersistentToLocal
• NanFromV8String
• NanMakeWeak
• NanNewLocal
• NAN_WEAK_CALLBACK_OBJECT
• NAN_WEAK_CALLBACK_DATA

Introduced:

• NanEscapableScope
• NanEscapeScope
• NanMakeWeakPersistent callback argument must be specialized to work on both old and new node
• NanNew
• NanGetCurrentContext
• NanAdjustExternalMemory
• NanAddGCEpilogueCallback
• NanAddGCPrologueCallback
• NanGetHeapStatistics
• NanRemoveGCEpilogueCallback
• NanRemoveGCPrologueCallback
• NanSetTemplate
• NanCompileScript
• NanRunScript
• NanMakeCallback
• NanUndefined
• NanNull
• NanTrue
• NanFalse

Modified:

• NanSymbol now does NanNew<v8::String>(value) on new Node. (What's the new, experimental Symbol class in v8 for? v8::String::NewSymbol() is gone.)
• NAN_WEAK_CALLBACK now has an argument called data with access to the callback object and data, as well as methods for renewing or disposing the weak callback
• NanAssignPersistent may have been leaking persistent handles in old versions of Node
• type arguments are gone from everywhere
• NAN_INLINE no longer takes arguments
• NAN_DEPRECATED no longer takes arguments

[kkoopa]

Is there any point maintaining NanPersistentToLocal when NanNew could handle it?
What about doing a #define $ NanNew?

[rvagg]

This is broken on 0.8.x btw, fine on 0.10.x and master but none of the 0.8's pass.

  CXX(target) Release/obj.target/asyncworker/cpp/asyncworker.o
In file included from ../cpp/asyncworker.cpp:11:0:
../../nan.h: In function 'v8::Local<v8::Object> NanNewBufferHandle(const char*, uint32_t)':
../../nan.h:1140:59: error: invalid conversion from 'const char*' to 'char*' [-fpermissive]
In file included from ../../nan.h:135:0,
                 from ../cpp/asyncworker.cpp:11:
/usr/src/node/src/node_buffer.h:101:18: error:   initializing argument 1 of 'static node::Buffer* node::Buffer::New(char*, size_t)' [-fpermissive]
make: make: Leaving directory `/dnt/test/build'
*** [Release/obj.target/asyncworker/cpp/asyncworker.o] Error 1

considering we've had calls for 0.6 compatibility (which I believe we've largely had), this would upset people if we released like this.

[bnoordhuis]

That's because of nodejs/node-v0.x-archive@7ab4a77d. It should be alright to do a const_cast<char*>(...) here; Buffer::New() in v0.8 is conceptually const, just not in its argument list. :-)

[kkoopa]

So it needs patching for ancient versions. Question then is whether to keep NanNewBufferHandle or create yet another overload of NanNew. NanNewContext could also be integrated into NanNew and dropped.

[kkoopa]

I have not tested, but I guess it should work on 0.8 now.

[rvagg]

👍 thanks!

rvagg@tweedy (0.9-wip) ~/git/nan$ sudo dnt
Node@v0.10.26: PASS wrote output to /tmp/nan-dnt-v0.10.26.out
Node@v0.10.25: PASS wrote output to /tmp/nan-dnt-v0.10.25.out
Node@v0.10.24: PASS wrote output to /tmp/nan-dnt-v0.10.24.out
Node@master  : PASS wrote output to /tmp/nan-dnt-master.out
Node@v0.10.23: PASS wrote output to /tmp/nan-dnt-v0.10.23.out
Node@v0.10.22: PASS wrote output to /tmp/nan-dnt-v0.10.22.out
Node@v0.10.21: PASS wrote output to /tmp/nan-dnt-v0.10.21.out
Node@v0.10.20: PASS wrote output to /tmp/nan-dnt-v0.10.20.out
Node@v0.10.19: PASS wrote output to /tmp/nan-dnt-v0.10.19.out
Node@v0.10.18: PASS wrote output to /tmp/nan-dnt-v0.10.18.out
Node@v0.8.26 : PASS wrote output to /tmp/nan-dnt-v0.8.26.out
Node@v0.8.25 : PASS wrote output to /tmp/nan-dnt-v0.8.25.out
Node@v0.8.24 : PASS wrote output to /tmp/nan-dnt-v0.8.24.out
Node@v0.8.23 : PASS wrote output to /tmp/nan-dnt-v0.8.23.out
Node@v0.8.22 : PASS wrote output to /tmp/nan-dnt-v0.8.22.out
Took 36s to run 15 versions of Node

[rvagg]

Still passing on Node master, is anyone aware of issues with this branch against master? We should prepare for a release soon, I'll probe the core guys about the promised 0.11.13.

In the meantime, anything new that's been added or changed, please update the parts of the docs you're responsible, also filling out some dot points in the Changelog at the top of nan.h would save me some time at release digging through the git log to come up with one myself.

[rvagg]

Upgraded LevelDOWN to this branch, kind of epic but it's working on 0.10 and master now. The actual commit is here. I have a failing test involving a buffer but I'm leaning towards an error on my end rather than NAN or Node master.

I'm loving the new NanNew() stuff, fantastic work on that @kkoopa! Unfortunately there's going to be a lot of pain out there for people to catch up to NAN changes and also the major V8 changes.. Might need a more detailed write-up than normal.

[kkoopa]

There's no support for making v8::Scripts yet. This was changed in the latest v8 upgrade. Don't know what else has changed.

[bnoordhuis]

That's the only major change between 3.24 and 3.25, everything else is either small fix-ups or new functionality but not anything that should affect nan.

[TooTallNate]

Removed:
NanInitPersistent
NanPersistentToLocal
NanFromV8String
NanMakeWeak
NanNewLocal
NAN_WEAK_CALLBACK_OBJECT
NAN_WEAK_CALLBACK_DATA

So, are we actually breaking API here, or just deprecating? Breaking the API should be reserved for a v1.0.0 release IMO, otherwise, any packages out there that have i.e. "nan": "0" would break once this is released.

[kkoopa]

I don't think many packages have it added that way. Those who use NAN use it to support 0.12 (and 0.11 inbetween), everybody should want to run the latest working 0.11 release, as it is an unstable target. In order to support Node 0.11.13 and on, there will have to be rewriting any way, so this means that all packages using nan will need changing. Even if we did not break NAN, there would still have to be rewriting for everything, due to the v8 changes.

Oh yeah, while many items could be retained as deprecated, the weak API can't be unbroken, so anything using that will break no matter what.

I guess we could declare it 1.0, but up until now the promise has been that builds are stable within a minor version, so doing "nan" : "0" would not have been correct for that behavior, it should always have been "nan" : "~0.8".

[rvagg]

Yeah, we should probably start following correct semver now because of the ^ change in npm. 1.0.0 will be fine for this release, there's lots of breakage.

[bnoordhuis]

I don't have an opinion on what the version number should be, just wanted to point out that ^ works like ~ when the major version is < 0.

(I have opinions on npm trying to shove semver down everyone's throat but that's besides the point. Suffice it to say that none of my modules will ever see a 1.0.0 release.)

[kkoopa]

I think this should be fairly feature-complete now, there could still be some polishing. Maybe NanNull() et al should return Locals and not Handles, I've made all others return Locals. Then there is the Buffer and Context constructors which I would like to get into NanNew, but they are a little problematic to fit. All those overloads of NanNew might also need some oversight, perhaps they could be better structured somehow, or generated via macros. NanNew still does not support everything you might think it should, like Dates. These extra overloads are however not strictly necessary, just nice additions to make things unified.

[rvagg]

Agreed on pushing things through NanNew, makes the API easier to deal with if you don't have to look up what to use each time you want to do something new.

[Rush]

You guys certainly been busy. I have managed to compile the addon for 0.11.13-pre but it seems 0.10 is completely broken now:

n file included from ../include/x509.h:7:0,
                 from ../src/addon.cc:5:
../node_modules/nan/nan.h: In function ‘v8::Local<v8::Script> NanNew(v8::Local<v8::String>, const v8::ScriptOrigin&)’:
../node_modules/nan/nan.h:895:37: error: no matching function for call to ‘v8::Script::New(v8::Local<v8::String>&, const v8::ScriptOrigin&)’
     return v8::Script::New(s, origin);
                                     ^
../node_modules/nan/nan.h:895:37: note: candidates are:
In file included from /home/rush/.node-gyp/0.10.18/src/node.h:62:0,
                 from ../include/addon.h:4,
                 from ../src/addon.cc:4:
/home/rush/.node-gyp/0.10.18/deps/v8/include/v8.h:615:24: note: static v8::Local<v8::Script> v8::Script::New(v8::Handle<v8::String>, v8::ScriptOrigin*, v8::ScriptData*, v8::Handle<v8::String>)
   static Local<Script> New(Handle<String> source,
                        ^
/home/rush/.node-gyp/0.10.18/deps/v8/include/v8.h:615:24: note:   no known conversion for argument 2 from ‘const v8::ScriptOrigin’ to ‘v8::ScriptOrigin*’
/home/rush/.node-gyp/0.10.18/deps/v8/include/v8.h:630:24: note: static v8::Local<v8::Script> v8::Script::New(v8::Handle<v8::String>, v8::Handle<v8::Value>)
   static Local<Script> New(Handle<String> source,
                        ^
/home/rush/.node-gyp/0.10.18/deps/v8/include/v8.h:630:24: note:   no known conversion for argument 2 from ‘const v8::ScriptOrigin’ to ‘v8::Handle<v8::Value>’
In file included from ../include/x509.h:7:0,
                 from ../src/addon.cc:5:
../node_modules/nan/nan.h: At global scope:
../node_modules/nan/nan.h:904:40: error: redefinition of ‘template<class T> v8::Local<v8::Script> NanNew(v8::Local<v8::String>, const v8::ScriptOrigin&)’
   NAN_INLINE v8::Local<NanBoundScript> NanNew(
                                        ^
../node_modules/nan/nan.h:891:42: error: ‘template<class T> v8::Local<v8::Script> NanNew(v8::Local<v8::String>, const v8::ScriptOrigin&)’ previously declared here
   NAN_INLINE v8::Local<NanUnboundScript> NanNew(
                                          ^
../node_modules/nan/nan.h: In function ‘v8::Local<v8::Script> NanNew(v8::Local<v8::String>, const v8::ScriptOrigin&)’:
../node_modules/nan/nan.h:908:41: error: no matching function for call to ‘v8::Script::Compile(v8::Local<v8::String>&, const v8::ScriptOrigin&)’
     return v8::Script::Compile(s, origin);
                                         ^
../node_modules/nan/nan.h:908:41: note: candidates are:
In file included from /home/rush/.node-gyp/0.10.18/src/node.h:62:0,
                 from ../include/addon.h:4,
                 from ../src/addon.cc:4:
/home/rush/.node-gyp/0.10.18/deps/v8/include/v8.h:649:24: note: static v8::Local<v8::Script> v8::Script::Compile(v8::Handle<v8::String>, v8::ScriptOrigin*, v8::ScriptData*, v8::Handle<v8::String>)
   static Local<Script> Compile(Handle<String> source,
                        ^
/home/rush/.node-gyp/0.10.18/deps/v8/include/v8.h:649:24: note:   no known conversion for argument 2 from ‘const v8::ScriptOrigin’ to ‘v8::ScriptOrigin*’
/home/rush/.node-gyp/0.10.18/deps/v8/include/v8.h:667:24: note: static v8::Local<v8::Script> v8::Script::Compile(v8::Handle<v8::String>, v8::Handle<v8::Value>, v8::Handle<v8::String>)
   static Local<Script> Compile(Handle<String> source,
                        ^
/home/rush/.node-gyp/0.10.18/deps/v8/include/v8.h:667:24: note:   no known conversion for argument 2 from ‘const v8::ScriptOrigin’ to ‘v8::Handle<v8::Value>’
In file included from ../include/x509.h:7:0,
                 from ../src/addon.cc:5:
../node_modules/nan/nan.h: At global scope:
../node_modules/nan/nan.h:912:40: error: redefinition of ‘v8::Local<T> NanNew(P) [with T = v8::Script; P = v8::Local<v8::String>]’
   NAN_INLINE v8::Local<NanBoundScript> NanNew(v8::Local<v8::String> s) {
                                        ^
../node_modules/nan/nan.h:899:42: error: ‘v8::Local<T> NanNew(P) [with T = v8::Script; P = v8::Local<v8::String>]’ previously declared here
   NAN_INLINE v8::Local<NanUnboundScript> NanNew(v8::Local<v8::String> s) {

[rvagg]

probably due to a change introduced yesterday on that branch of NAN, it'll be sorted out before a proper release and we won't go live with a git dependency here either

[rvagg]

sorry, I thought this was a leveldown thread! ooops. I'm sure @kkoopa will be on to this soon and have it sorted.

[Rush]

Hopefully there will be an end to the changes in v8 at some point and broken official node releases. It is pretty tedious to fix stuff for such moving target. :)

[Rush]

Now I have problem because I have ported node-x509 to 0.11.13 through this pull request but as soon as as the control flow returns from native addon to node, the node process terminates abruptly without any error or segault. It simply exits. Now I cannot check if it works with other versions but if you have similar issues then please advise.

[rvagg]

@RushPL have a look for references that may be garbage collected. The GC behaviour all through 0.11 has been quite different. If you don't hold on to a Persistent to something that you need later on then you can get this behaviour. A callback is the most likely candidate, it gets cleaned up and the call never gets propagated up into the JS layer even though it might be made in the C++ layer.

[rvagg]

merged and 1.0.0 released

[kkoopa]

LGTM But I wonder how tests pass down to 0.11.10, were there constructors with isolates already then?

Rod Vagg notifications@github.com wrote:

I've just done the changelog in nan.h, please have a quick look and let me know if I've missed anything or if you want to expand on anything (actually, just change it yourself asap). I'm just going to do a quick note on the README and mention some of the major changes that will impact most people -- NanNew, NanEscapableScope, the NanAssignPersistent sig change, NanDisposePersistent, NanMakeCallback. Once I've written that up I'll bump to 1.0.0 and release. I'll follow up with a leveldown release and a node-bignum release. After that I guess we should all go around and update the various projects we contribute this stuff to.

—
Reply to this email directly or view it on GitHub.

[rvagg]

my guess is that our test suite is too incomplete to pick up incompatibilities! yes, about 1/2 of the constructors had isolates, now it's pervasive