search

mscdex / node-pcre

A pcre binding for node.js
MIT License
39 stars 9 forks source link

Broken on 0.11.13 #9

Open iliakan opened 10 years ago

iliakan commented 10 years ago

Fails to compile on 0.11.13.

mscdex commented 10 years ago

Yep, due to the changes in the v8 API.

dshadowwolf commented 10 years ago

There has been plans for a stable api announced, but whether that is going to happen for 0.12 or if its plannec for 'by 1.0' is something I don't know.

In the meantime it appears that the project NaN (native abstractions for node) can be used to change existing projects so they will work on 0.11 and still be compatible with old versions. I might need this package for a project I'm planning, so I am going to look into that.

If it works, I'll drop a pull request here.

mscdex commented 10 years ago

Yeah that's the rather aggravating thing about the upcoming node v0.12. If they release it without a nan-like solution built-in and/or they don't end up just integrating nan (as-is, in the future), then that means you either have rework your code twice (once to nan and then again whenever node core has its own stable API thing) or you just have a binding that doesn't work for a whole stable node branch (assuming the stable API thing doesn't come out until the next stable node version after v0.12).

I really wish the node core team would be more vocal about this for us binding authors because depending on the binding, it could mean a lot of (re-)work. A lot of us work on these projects in our free time, and getting the timing right on introducing a stable API thing is important.

dshadowwolf commented 10 years ago

Without using NaN (I was unaware of it at the time) it took me about a day to port the node binding for libyaml to 0.11 - and that was quick&dirty without caring for backwards compatibility or code cleanliness.

I think it'd take me about twice that to come up with a method of cleaning up the code and making it easy to read again. But a lot of that is because they changed the layering of things and v8::Persistent is no longer descended from v8::Handle... which, I suppose, could be handled by a wrapper or... hrm... just had a thought about how to get around some of the problems with the API differences that doesn't involve NaN.

I'll test that idea and get back to this with a response and maybe some example code for getting around some things :)

dshadowwolf commented 10 years ago

okay... haven't tested it, but...

if NODE_VERSION_AT_LEAST(0,11,0)

include <node_object_wrap.h>

include <util.h>

include <util-inl.h>

// helper macros

define AS_LOCAL(v) PersistentToLocal((v))

define True() v8::True(Isolate::GetCurrent())

define False() v8::False(Isolate::GetCurrent())

define Null() v8::Null(Isolate::GetCurrent())

else

define AS_LOCAL(v) (v)

endif

What is above should cover a lot of the use-cases and require no other changes to the code. However... Since there are now requirements for an Isolate* being passed as the first argument to just about everything and the definition of a HandleScope has changed from: HandleScope scope; to: HandleScope scope(Isolate*);

there are a number of things that need more than just those macros wrapping them. I'm thinking that there needs to be a wrapper around, at least, calls to the various String::NewXX and Integer::NewXX functions... Not to mention wrappers around returning from functions exported to the script environment itself... hrm...

Give me a couple days and I might have something supremely lightweight that I could suggest as a stop-gap to fill the void around more of this - potentially with little more than CPP macros.

mikemaccana commented 9 years ago

Thanks for the rad library @mscdex ! @dshadowwolf did your experiments turn up anything? Trying to get this running on current node.

mscdex commented 9 years ago

@mikemaccana I haven't had time to properly nan-ize this addon yet. I am open to PRs to fix that though.

dshadowwolf commented 9 years ago

@mikemaccana I haven't worked on that for a while - not since deciding that it would be better to make sure my code was functional under a wide-sprwad version first. However... I have noticed that a lot of the API changes are much deeper than can be covered the idea speculated about above.

The biggest issue is in native-code functions and how they interact with V8. In the older version they directly return a value. In the newer versions this has changed. This change would necessitate some complex macros for defining such functions and to wrap their existing 'return' statements. The latter is not a problem, but the former definitely is, as a good solution requires a macro system more capable than cpp.

With that being the case, well... My solution might wind up actually being an awk or m4 script - and, at that point, there is a lot more that becomes possible.