Skip to content
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

nan 2.0.0 breaks in [email protected] #396

Closed
Raynos opened this issue Aug 5, 2015 · 11 comments
Closed

nan 2.0.0 breaks in [email protected] #396

Raynos opened this issue Aug 5, 2015 · 11 comments

Comments

@Raynos
Copy link

Raynos commented Aug 5, 2015

In file included from ../src/sse4_crc32.cpp:18:
../node_modules/nan/nan.h:1178:39: error: no member named 'REPLACE_INVALID_UTF8' in 'v8::String'
                          v8::String::REPLACE_INVALID_UTF8;
                          ~~~~~~~~~~~~^
1 error generated.
make: *** [Release/obj.target/sse4_crc32/src/sse4_crc32.o] Error 1

The sse4_crc32 module updated to nan@2 and broke my library in node 0.10.26

Does nan want to support node 0.10.26 ?

@kkoopa
Copy link
Collaborator

kkoopa commented Aug 5, 2015

Nope, you have to use 0.10.29 or later.

@rvagg
Copy link
Member

rvagg commented Aug 5, 2015

@kkoopa how difficult would this compatibility be? I know there are a few companies stuck on older versions and the security patches aren't relevant for their use cases.

@Raynos
Copy link
Author

Raynos commented Aug 5, 2015

At least for uber we really should use 0.10.32. We have no reason to not upgrade.

The bigger blocker is the memory leak which makes 2.0.0 unusable ;)

@kkoopa
Copy link
Collaborator

kkoopa commented Aug 5, 2015

Maintaining something this outdated is not worth the effort. It's currently 0.8.27+, 0.10.29+, 0.12.0+, 1.0.0+, 2.0.0+ and 3.0.0+, which is more than good enough. Everything takes precedence over supporting outdated minor versions of an outdated branch.

@bnoordhuis
Copy link
Member

It's easy to check the NODE_{MAJOR,MINOR,PATCH}_VERSION macros but I'm ambivalent on whether it's a good idea.

@kkoopa
Copy link
Collaborator

kkoopa commented Aug 5, 2015

Don't want to maintain more exceptions than necessary. This would also entail duplicating the functionality provided by https://gist.githubusercontent.com/tjfontaine/f869f373a8e9416809ba/raw/e3eb85201413a79d12ce24a7cb4b02edf0abc1a5/v0.10-invalid-utf8.patch

http://blog.nodejs.org/2014/06/16/openssl-and-breaking-utf-8-change/

The current version of Node 0.10 is 0.10.40, 0.10.29 was released over a year ago. I don't think requiring it at this point is aksing too much. If someone wants to run older versions of 0.10, they can apply the above patch themselves and build from source.

@bnoordhuis
Copy link
Member

See #399.

bnoordhuis added a commit that referenced this issue Aug 5, 2015
`v8::String::REPLACE_INVALID_UTF8` was added in node.js v0.10.29.
Check the version macros to see if it's available.

Fixes: #394
Fixes: #396
@bnoordhuis
Copy link
Member

Fixed by 60d6687.

@kkoopa Can we do a patch release for this? See e.g. fsevents/fsevents#78 for people that are waiting for it.

@kkoopa
Copy link
Collaborator

kkoopa commented Aug 6, 2015

Sure, I've been meaning to do one, but it is blocked on #389. No point upgrading if everything leaks. Also need to merge #392 and then #397.

@bnoordhuis
Copy link
Member

If it's already leaking in v2.0.0, then it's not a regression in v2.0.1, right? :-) The patch release makes things less worse for people stuck on older v0.10 versions.

@kkoopa
Copy link
Collaborator

kkoopa commented Aug 6, 2015

Fine.

amitparida added a commit to amitparida/npm-nan that referenced this issue Oct 3, 2024
`v8::String::REPLACE_INVALID_UTF8` was added in node.js v0.10.29.
Check the version macros to see if it's available.

Fixes: nodejs/nan#394
Fixes: nodejs/nan#396
PR-URL: nodejs/nan#399
Reviewed-By: Benjamin Byholm <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants