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 for Node 4.0.0 #180

Closed
brianmcd opened this Issue Sep 9, 2015 · 56 comments

Comments

Projects
None yet
@brianmcd
Owner

brianmcd commented Sep 9, 2015

No description provided.

@brianmcd

This comment has been minimized.

Show comment
Hide comment
@brianmcd

brianmcd Sep 9, 2015

Owner

@kkoopa - is this something you can help with? If not, is there an upgrade guide floating around, or should I work my way through the NAN changelog?

Owner

brianmcd commented Sep 9, 2015

@kkoopa - is this something you can help with? If not, is there an upgrade guide floating around, or should I work my way through the NAN changelog?

@kkoopa

This comment has been minimized.

Show comment
Hide comment
@kkoopa

kkoopa Sep 9, 2015

Collaborator

Is it still going to be maintained? Would it not be better to write something on the javascript level that uses the existing code for versions less than 0.12, and the revised vm module of Node for everything later.

Upgrading to NAN 2 wouldn't be hard, I can do it in ten minutes, but is it worth it?

Collaborator

kkoopa commented Sep 9, 2015

Is it still going to be maintained? Would it not be better to write something on the javascript level that uses the existing code for versions less than 0.12, and the revised vm module of Node for everything later.

Upgrading to NAN 2 wouldn't be hard, I can do it in ten minutes, but is it worth it?

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Sep 9, 2015

The problem is there's no way to write in package.json "install this module only in Node<4.0.0". You can declare it as an optional dependency but it will then try to get compiled in newer Nodes & fail; everything should work afterwards but the installation shouldn't have to try to compile things that are not needed, burning CPU cycles & printing confusing error-not-error messages.

mgol commented Sep 9, 2015

The problem is there's no way to write in package.json "install this module only in Node<4.0.0". You can declare it as an optional dependency but it will then try to get compiled in newer Nodes & fail; everything should work afterwards but the installation shouldn't have to try to compile things that are not needed, burning CPU cycles & printing confusing error-not-error messages.

@kkoopa

This comment has been minimized.

Show comment
Hide comment
@kkoopa

kkoopa Sep 9, 2015

Collaborator

Ah, I had not thought about that, but it is easily avoidable. Hide everything behind an ifdef and make it compile nothing.

Collaborator

kkoopa commented Sep 9, 2015

Ah, I had not thought about that, but it is easily avoidable. Hide everything behind an ifdef and make it compile nothing.

@ChALkeR

This comment has been minimized.

Show comment
Hide comment
@ChALkeR

ChALkeR Sep 10, 2015

Adding to the list: nodejs/node#2798.

ChALkeR commented Sep 10, 2015

Adding to the list: nodejs/node#2798.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Sep 10, 2015

FWIW, @rvagg has a PR in progress: #181.

mgol commented Sep 10, 2015

FWIW, @rvagg has a PR in progress: #181.

@brianmcd

This comment has been minimized.

Show comment
Hide comment
@brianmcd

brianmcd Sep 11, 2015

Owner

That's a cool thought, @kkoopa. My current plan is to test/merge #181 to get builds working ASAP, then look into doing the JS implementation for Node 4 and up.

Owner

brianmcd commented Sep 11, 2015

That's a cool thought, @kkoopa. My current plan is to test/merge #181 to get builds working ASAP, then look into doing the JS implementation for Node 4 and up.

@kkoopa

This comment has been minimized.

Show comment
Hide comment
@kkoopa

kkoopa Sep 11, 2015

Collaborator

It might make sense to use the built-in vm module already from 0.12 up, since @domenic based the rewrite on Contextify. I would think many bugs that were in Contextify have been fixed in the vm module in that process. Essentially, the C++ code of Contextify is only necessary for Node 0.10 and older.

Collaborator

kkoopa commented Sep 11, 2015

It might make sense to use the built-in vm module already from 0.12 up, since @domenic based the rewrite on Contextify. I would think many bugs that were in Contextify have been fixed in the vm module in that process. Essentially, the C++ code of Contextify is only necessary for Node 0.10 and older.

@thelinuxlich

This comment has been minimized.

Show comment
Hide comment
@thelinuxlich

thelinuxlich commented Sep 18, 2015

+1

@soenkekluth

This comment has been minimized.

Show comment
Hide comment
@soenkekluth

soenkekluth Sep 18, 2015

+1 actually one of our projects completely won't work on node 4.x just because of this...

soenkekluth commented Sep 18, 2015

+1 actually one of our projects completely won't work on node 4.x just because of this...

@rvagg

This comment has been minimized.

Show comment
Hide comment
@rvagg

rvagg Sep 19, 2015

Contributor

sorry, I'm blocked on one failure in #181 and don't have time to figure it out right now, happy for someone else to take my code if they can work it out

Contributor

rvagg commented Sep 19, 2015

sorry, I'm blocked on one failure in #181 and don't have time to figure it out right now, happy for someone else to take my code if they can work it out

@deployable deployable referenced this issue Sep 20, 2015

Closed

New release #2254

@KingScooty

This comment has been minimized.

Show comment
Hide comment
@KingScooty

KingScooty Sep 21, 2015

+1 It's affects a lot of packages i need since upgrading to node 4.x. It appears Browser Sync and a few of its plugins require it.

KingScooty commented Sep 21, 2015

+1 It's affects a lot of packages i need since upgrading to node 4.x. It appears Browser Sync and a few of its plugins require it.

@janneraiskila

This comment has been minimized.

Show comment
Hide comment
@janneraiskila

janneraiskila Sep 21, 2015

+1 Breaks lots of things

janneraiskila commented Sep 21, 2015

+1 Breaks lots of things

@base698

This comment has been minimized.

Show comment
Hide comment
@base698

base698 Sep 22, 2015

Using node 4.1.0

$ npm install contextify

contextify@0.1.14 install /Users/workspaces/soundselect/node_modules/contextify
node-gyp rebuild

CXX(target) Release/obj.target/contextify/src/contextify.o
In file included from ../src/contextify.cc:3:
../node_modules/nan/nan.h:261:25: error: redefinition of '_NanEnsureLocal'
NAN_INLINE v8::Local _NanEnsureLocal(v8::Local val) {
^
../node_modules/nan/nan.h:256:25: note: previous definition is here
NAN_INLINE v8::Local _NanEnsureLocal(v8::Handle val) {
^
../node_modules/nan/nan.h:661:13: error: no member named 'smalloc' in namespace 'node'
, node::smalloc::FreeCallback callback
~~~~~~^
../node_modules/nan/nan.h:672:12: error: no matching function for call to 'New'
return node::Buffer::New(v8::Isolate::GetCurrent(), data, size);
^~~~~~~~~~~~~~~~~
/Users/justinthomas/.node-gyp/4.1.0/include/node/node_buffer.h:31:40: note: candidate function not viable: no known conversion from
'uint32_t' (aka 'unsigned int') to 'enum encoding' for 3rd argument
NODE_EXTERN v8::MaybeLocalv8::Object New(v8::Isolate* isolate,

base698 commented Sep 22, 2015

Using node 4.1.0

$ npm install contextify

contextify@0.1.14 install /Users/workspaces/soundselect/node_modules/contextify
node-gyp rebuild

CXX(target) Release/obj.target/contextify/src/contextify.o
In file included from ../src/contextify.cc:3:
../node_modules/nan/nan.h:261:25: error: redefinition of '_NanEnsureLocal'
NAN_INLINE v8::Local _NanEnsureLocal(v8::Local val) {
^
../node_modules/nan/nan.h:256:25: note: previous definition is here
NAN_INLINE v8::Local _NanEnsureLocal(v8::Handle val) {
^
../node_modules/nan/nan.h:661:13: error: no member named 'smalloc' in namespace 'node'
, node::smalloc::FreeCallback callback
~~~~~~^
../node_modules/nan/nan.h:672:12: error: no matching function for call to 'New'
return node::Buffer::New(v8::Isolate::GetCurrent(), data, size);
^~~~~~~~~~~~~~~~~
/Users/justinthomas/.node-gyp/4.1.0/include/node/node_buffer.h:31:40: note: candidate function not viable: no known conversion from
'uint32_t' (aka 'unsigned int') to 'enum encoding' for 3rd argument
NODE_EXTERN v8::MaybeLocalv8::Object New(v8::Isolate* isolate,

@rvagg

This comment has been minimized.

Show comment
Hide comment
@rvagg

rvagg Sep 22, 2015

Contributor

If you're desperate then you can npm i 'contextify@rvagg/contextify#nan2' but see #181 for the context (har har) on that. It should be fine on 0.12+ but the fact that there's a weird hold-up on 0.10 suggests that all may not be quite right.

Contributor

rvagg commented Sep 22, 2015

If you're desperate then you can npm i 'contextify@rvagg/contextify#nan2' but see #181 for the context (har har) on that. It should be fine on 0.12+ but the fact that there's a weird hold-up on 0.10 suggests that all may not be quite right.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Sep 22, 2015

If finishing up the nan2 branch proves difficult to finish fast maybe @kkoopa's solution from #180 (comment) would work?

mgol commented Sep 22, 2015

If finishing up the nan2 branch proves difficult to finish fast maybe @kkoopa's solution from #180 (comment) would work?

@base698

This comment has been minimized.

Show comment
Hide comment
@base698

base698 Sep 22, 2015

I'm not using contextify directly--its used by several other modules like
jsdom. I could fork all of those and reference the pull requested version
but that seems non optimal.
On Sep 21, 2015 17:59, "Michał Gołębiowski" notifications@github.com
wrote:

If finishing up the nan2 branch proves difficult to finish fast maybe the
solution from #180 (comment)
#180 (comment)
would work?


Reply to this email directly or view it on GitHub
#180 (comment)
.

base698 commented Sep 22, 2015

I'm not using contextify directly--its used by several other modules like
jsdom. I could fork all of those and reference the pull requested version
but that seems non optimal.
On Sep 21, 2015 17:59, "Michał Gołębiowski" notifications@github.com
wrote:

If finishing up the nan2 branch proves difficult to finish fast maybe the
solution from #180 (comment)
#180 (comment)
would work?


Reply to this email directly or view it on GitHub
#180 (comment)
.

@slavaGanzin

This comment has been minimized.

Show comment
Hide comment
@slavaGanzin

slavaGanzin Sep 28, 2015

Just a comment to be in touch

Thanks for your work guys 👍

slavaGanzin commented Sep 28, 2015

Just a comment to be in touch

Thanks for your work guys 👍

@tonycurwen

This comment has been minimized.

Show comment
Hide comment
@tonycurwen

tonycurwen Sep 28, 2015

So clearly not a fix, but if you're deploying to Heroku just force Node 0.12.7 in the package.json file for your project until the 4.x support is resolved. Worked for me though I don't have any Node 4.0/4.1 dependencies at this time.

    "engines": {
        "node": "0.12.7",
        "npm": "2.11.3"
    },

I mention Heroku as this defect was blocking new deploys for me due to a recent Heroku Node upgrade.

tonycurwen commented Sep 28, 2015

So clearly not a fix, but if you're deploying to Heroku just force Node 0.12.7 in the package.json file for your project until the 4.x support is resolved. Worked for me though I don't have any Node 4.0/4.1 dependencies at this time.

    "engines": {
        "node": "0.12.7",
        "npm": "2.11.3"
    },

I mention Heroku as this defect was blocking new deploys for me due to a recent Heroku Node upgrade.

@idoby

This comment has been minimized.

Show comment
Hide comment
@idoby

idoby Oct 27, 2015

Collaborator

My 4.0.0 port #190 doesn't seem to compile on @brianmcd's CI server, but it should work if you have a C++11 compiler (required by the new NAN).

Collaborator

idoby commented Oct 27, 2015

My 4.0.0 port #190 doesn't seem to compile on @brianmcd's CI server, but it should work if you have a C++11 compiler (required by the new NAN).

@skeller88

This comment has been minimized.

Show comment
Hide comment
@skeller88

skeller88 commented Oct 27, 2015

+1

@abhas9

This comment has been minimized.

Show comment
Hide comment
@abhas9

abhas9 Oct 29, 2015

+1 for node 4.x support.

abhas9 commented Oct 29, 2015

+1 for node 4.x support.

@rvagg

This comment has been minimized.

Show comment
Hide comment
@rvagg

rvagg Oct 29, 2015

Contributor

I'm really looking forward to knowing what we'll be able to buy with all these +1's

Contributor

rvagg commented Oct 29, 2015

I'm really looking forward to knowing what we'll be able to buy with all these +1's

@idoby

This comment has been minimized.

Show comment
Hide comment
@idoby

idoby Oct 29, 2015

Collaborator

Collaborator

idoby commented Oct 29, 2015

@caljrimmer

This comment has been minimized.

Show comment
Hide comment
@caljrimmer

caljrimmer commented Nov 1, 2015

+1

4 similar comments
@aaronryden

This comment has been minimized.

Show comment
Hide comment
@aaronryden

aaronryden commented Nov 2, 2015

👍

@y9v

This comment has been minimized.

Show comment
Hide comment
@y9v

y9v commented Nov 4, 2015

+1

@jfraboni

This comment has been minimized.

Show comment
Hide comment
@jfraboni

jfraboni commented Nov 4, 2015

+1

@rpeterson

This comment has been minimized.

Show comment
Hide comment
@rpeterson

rpeterson commented Nov 4, 2015

+1

@reality

This comment has been minimized.

Show comment
Hide comment
@reality

reality commented Nov 5, 2015

+1

@why520crazy

This comment has been minimized.

Show comment
Hide comment
@why520crazy

why520crazy Nov 5, 2015

+111111111111111111111111111111111111111

why520crazy commented Nov 5, 2015

+111111111111111111111111111111111111111

@sattaman

This comment has been minimized.

Show comment
Hide comment
@sattaman

sattaman commented Nov 6, 2015

+1

6 similar comments
@SylvainCorlay

This comment has been minimized.

Show comment
Hide comment
@SylvainCorlay

SylvainCorlay commented Nov 8, 2015

+1

@kerawits

This comment has been minimized.

Show comment
Hide comment
@kerawits

kerawits commented Nov 9, 2015

+1

@afcastano

This comment has been minimized.

Show comment
Hide comment
@afcastano

afcastano commented Nov 9, 2015

+1

@DKnodel

This comment has been minimized.

Show comment
Hide comment
@DKnodel

DKnodel commented Nov 9, 2015

+1

@healyje

This comment has been minimized.

Show comment
Hide comment
@healyje

healyje commented Nov 10, 2015

+1

@pyephyomaung

This comment has been minimized.

Show comment
Hide comment
@pyephyomaung

pyephyomaung commented Nov 12, 2015

+1

@poshaughnessy

This comment has been minimized.

Show comment
Hide comment
@poshaughnessy

poshaughnessy Nov 12, 2015

This fixed it for me for now - thanks @sirbrillig: sirbrillig/jsdom@b30bc08

poshaughnessy commented Nov 12, 2015

This fixed it for me for now - thanks @sirbrillig: sirbrillig/jsdom@b30bc08

@brianmcd

This comment has been minimized.

Show comment
Hide comment
@brianmcd

brianmcd Nov 12, 2015

Owner

Needs a few more +1s

Owner

brianmcd commented Nov 12, 2015

Needs a few more +1s

@brianmcd brianmcd closed this Nov 12, 2015

@brianmcd

This comment has been minimized.

Show comment
Hide comment
@brianmcd

brianmcd Nov 12, 2015

Owner

(Fixed in 0.1.15)

Owner

brianmcd commented Nov 12, 2015

(Fixed in 0.1.15)

@mikemaccana

This comment has been minimized.

Show comment
Hide comment
@mikemaccana

mikemaccana commented Nov 12, 2015

+thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment