xdenser / node-firebird-libfbclient

Firebird SQL binding
MIT License
82 stars 34 forks source link

Node 4.0 compatibility #58

Closed jacobalberty closed 8 years ago

jacobalberty commented 9 years ago

It seems this project is not currently working with node 4.0, I can provide the npm-debug.log if needed but after doing some digging i found it appears the main problem is it needs to be updated to NAN 2.x

jacobalberty commented 9 years ago

You can find my work to get it to compile at https://github.com/jacobalberty/node-firebird-libfbclient I dont know if it works yet but it does compile...

jacobalberty commented 9 years ago

I did some reformatting of the coding style to make it easier to read as I figured out what was going on so the diff is needlessly big between the two but it does compile, and connect and I've tested it with events.

The only change i've got a bad feeling about is in fb-bindings-result.cc around line 711 A direct translation would be Local ret = Nan::New(f_req->rowCallback->Call(1, argv)); But it doesn't compile so I tried removing the Nan::New() call around the callback call so it looks like Local ret = f_req->rowCallback->Call(1, argv); Documentation around the web indicated Local's dont need to be instantiated that way. But i'm not sure if Nan::Callback() on its own will bring the value into the right context.

[updated] unit tests run.

everything but test-events and test-blob work. test-blob gives me [updated] unit tests run.

test-blob.js
✔ textBlob
✔ asyncRead
*** Error in `node': free(): invalid pointer: 0x0000000002a0a988 ***

The test-event errors I always get when running tests, for some reason firebird on linux gives me weird events if I run more than one event per connection.

xdenser commented 9 years ago

For me under Windows all tests pass. I'll try linux tomorrow.

jacobalberty commented 9 years ago

Went ahead and opened the pull request so if you'd like to merge it in it's easier. See #59

jacobalberty commented 9 years ago

One issue with the patch, It doesn't work on 0.12, fails to compile.

diff --git a/src/fb-bindings-fbeventemitter.cc b/src/fb-bindings-fbeventemitter.cc
index fa2178d..4ec6452 100644
--- a/src/fb-bindings-fbeventemitter.cc
+++ b/src/fb-bindings-fbeventemitter.cc
@@ -23,7 +23,7 @@ void FBEventEmitter::Initialize(v8::Handle<v8::Object> target)
 void FBEventEmitter::Emit(Handle<String> event, int argc, Handle<Value> argv[])
   {
        Nan::HandleScope scope;
-    Handle<Value> argv1[11];
+    Local<Value> argv1[11];
     if(argc>10) Nan::ThrowError("Cant process more than 10 arguments");
     argv1[0] = event;
     for(int i=0;i<argc;i++) argv1[i+1] = argv[i];

Seems to be a proper fix, it appears to be working for me in both 4.0 and 0.12.

jacobalberty commented 9 years ago

A step closer, Tests pass up to events for me now, using copybuffer instead of newbuffer. Events always fail for me no matter how i test them so this might be the one. I'll be pushing it to my repository for the pull request as soon as I get git push straighted out with 2fa.