Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Support Node 0.11.13 #208

Closed
wants to merge 1 commit into from

9 participants

@kkoopa

This is a backwards-compatible patch for Node 0.11.13+.

@defunctzombie
Collaborator

Gonna need way more background info on this NAN stuff and what it is for, why we would move to it.

As an aside, this won't be merged or really looked at until 0.12 lands since node can still change in various ways. Lets revisit this when that happens. I will keep this open for now for gathering comments.

@kkoopa

Native Abstractions for Node.js

Thanks to the crazy changes in V8 (and some in Node core), keeping native addons compiling happily across versions, particularly 0.10 to 0.11/0.12, is a minor nightmare. The goal of this project is to store all logic necessary to develop native Node.js addons without having to inspect NODE_MODULE_VERSION and get yourself into a macro-tangle.

This project also contains some helper utilities that make addon development a bit more pleasant.

https://github.com/rvagg/nan

You should use it if you care for backwards compatibility across Node versions.

@defunctzombie
Collaborator
@kkoopa

Nan will at least be available as a node module in the near future or at the latest when 0.12 comes, then it will be possible to include it with a simple addition in binding.gyp and package.json, that will take care of that problem.

There might be something coming along the way, but it is not known when or what it will set out to do.
You're going to have to rewrite it anyway
Newsgroup discussion

@kkoopa

NAN is now available through NPM.

@defunctzombie
Collaborator
@kkoopa

This is it (as of kkoopa/libxmljs@d098232)

@defunctzombie
Collaborator

please squash these commits into one

@kkoopa

There.

@defunctzombie
Collaborator

For the most part, the pull request looks good to me. Just some minor cosmetic changes and a few questions about NAN usage.

@kkoopa

Fixed.

@defunctzombie
Collaborator

LGTM. Once we get closer to 0.12 release I will merge this in and publish a new version. Holding off on the merge into master since 0.11 could still change in fun ways.

@leukhin

Looks like time to add new version support, what you think?

Thanks.

@defunctzombie
Collaborator

So did this external library to be the compat later get put into node mainline? I find it really stupid that it is a separate module when it seems pretty clear to me that EVERYONE that has a native module will need to do this same stuff and use the same module.

@defunctzombie
Collaborator

To be clear on my previous comment. The following line:

kkoopa@3565a59#L0R5

Is completely stupid imho and should not exist. the real fix is to have the compat later for binary modules ship with node if they care about being backwards compatible at all (my guess is no one really does and only say they do).

@Raynos

@shtylman

From a small core philosophy it's nice to have multiple compat layer for binary modules in npm competing on the best solution rather then having a blessed solution

@defunctzombie
Collaborator

@Raynos I disagree. IMHO this is just bikeshedding.

@olostan

Wow... spent hours making libxmljs compilable on 0.11.8 instead of looking on pull requests. Please merge this PR or at least add note on a main page about it.

@defunctzombie
Collaborator

@olostan out of curiosity, did you use nan or just ported to only work on 0.11? I've been looking at some of the nan source code and am not sure things are being done the right way for some wrapped operations.

@defunctzombie
Collaborator

I have read up a bit about nan and understand more about it and some of the breaking changes in the v8 api. This pull request will be merged only after 0.12 is released and nan is updated to work on 0.12 (there could be other breaking changes to come). We are not a core library and thus can't play this arms race trying to stay up to date against unreleased versions of node.

@olostan

@defunctzombie without nan. It was quite lot of changes. But I needed only SAX parser, so I commented out all other code.

If anybody want to do changes needed for supporting node 0.11.* I can share my diff of sax parser.

@defunctzombie
Collaborator

Just to be clear (and so @kkoopa doesn't think his hard work is for naught) I will be merging this patch. It just won't happen until 0.12 is officially released.

@danielecr

2 stupids questions:
how do I apply this patch to a project? (it is possible via npm?)
would someone point me to a guide/doc so that I can do the same with another nodejs package? (the same means: propose patch to be merged to an existing project)

@polotek
Owner

@danielecr there are some good answers for this on stack overflow. http://stackoverflow.com/questions/5553156/how-do-i-merge-a-pull-request-on-someone-elses-project-in-git

If you do this and use it on node 0.11, let us know how it goes.

@danielecr

ok, it is merged now, but I got these stranges compilation errors:

In file included from ../src/libxmljs.cc:8:0:
../src/xml_document.h: At global scope:
../src/xml_document.h:11:45: error: expected class-name before ‘{’ token
class XmlDocument : public node::ObjectWrap {
^
../src/xml_document.h:42:44: error: ‘Arguments’ in namespace ‘v8’ does not name a type
static v8::Handlev8::Value New(const v8::Arguments& args);
^
../src/xml_document.h:42:59: error: ISO C++ forbids declaration of ‘args’ with no type [-fpermissive]
static v8::Handlev8::Value New(const v8::Arguments& args);
^
../src/xml_document.h:43:49: error: ‘Arguments’ in namespace ‘v8’ does not name a type
static v8::Handlev8::Value FromHtml(const v8::Arguments& args);
^
../src/xml_document.h:43:64: error: ISO C++ forbids declaration of ‘args’ with no type [-fpermissive]
static v8::Handlev8::Value FromHtml(const v8::Arguments& args);
^
../src/xml_document.h:44:48: error: ‘Arguments’ in namespace ‘v8’ does not name a type
static v8::Handlev8::Value FromXml(const v8::Arguments& args);
^

.... and so on, it seems it fails to check some build dependencies, which are those?
( @polotek thank you so much for to the useful link)

@kkoopa
@danielecr

v0.11.11

@kkoopa
@jedahan

Latest version that worked for me was 0.11.3

@kkoopa kkoopa changed the title from Support Node 0.11.4 to Support Node 0.11.13
@rohiniwork

+1

Can we merge this in.

The reason its failing for node 6.x is that nan v1.0.0 calls v8::FatalException.

This issue is resolved in the latest version of nan v1.1.x where it calls node::FatalException.

@kkoopa would be great if you can use latest nan and push your updates to this PR.
Could we use the latest nan and

@defunctzombie
Collaborator

@kkoopa can you squash these three commits and I will merge. Even tho I don't think node 0.12 is ever gonna be released might as well get this out there since there is no going back on the v8 stuff.

@kkoopa

Finally!
Go ahead and merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 2, 2014
  1. @kkoopa

    Use NAN 1.1.2

    kkoopa authored
Something went wrong with that request. Please try again.