-
Notifications
You must be signed in to change notification settings - Fork 507
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
Make compatible with io.js #223
Conversation
This did not seem to work as such. If it will work somehow, it is more likely to do so against the onepointfive branch which has a rewritten, saner |
Seems I hit a g++ 4.8 bug. I've pushed a fix-up that should fare better. |
@kkoopa are you around to cut a 1.5 within the next couple of days? io.js is going live and it would be good to have our bases covered with NAN. If you're not, then I can probably take care of it. |
Sure, I should be around, unless something unexpected happens. On Monday 12 January 2015 01:46:29 Rod Vagg wrote:
|
Has |
No, currently just the NODE_VERSION_* macros. I'll file a PR to bump NODE_MODULE_VERSION to something largish so it won't conflict with joyent/node.
A number of v8::V8 methods have been moved to v8::Isolate. I don't know the exact list off the top of my head but it's methods like SetFatalErrorHandler() and such that were already isolate-ified under the hood, just not visibly so in the API. Another change is that that v8::Value::To*() methods optionally (for now) take an Isolate argument. And of course there's a slew of new API methods for dealing with generators, promises, symbols, sets, maps, etc. A minor API addition that is really nice is that v8::Object and everything that descends from it now has a GetIsolate() method. |
Done and landed in nodejs/node#312. |
Okay, decision time. Do we:
|
one pro for using |
io.js ships a newer V8 version where String::ExternalAsciiStringResource has been replaced with String::ExternalOneByteStringResource. This change makes nan compile again with V8 3.29 and up. Fixes #222.
Okay. A counterpoint is that version numbers are arguably more readable. But like I said, it's mostly irrational, I'll update the PR. What about the first point? |
re |
Updated, PTAL. |
External strings are quite exotic, so I'm willing to fudge around a bit with it, but if removed we should make it NAN 2.0. What APIs use external string resource? Would it be possible to make a new class Foo implementing the functionality without changing a bunch of other functions that reference the class? Main concern, as usual, is to have consistent code and interfaces at the end user level, so the devs don't have to do version sniffing. On January 13, 2015 12:57:13 AM EET, Ben Noordhuis [email protected] wrote:
|
Overloads of String::NewExternal() and String::MakeExternal(). You can shim it but I don't think you need to bother, external strings aren't that popular. You use them when you have really big strings that you don't want to copy into the VM but in node land everyone uses buffers (and now typed arrays) for that. As a data point: the iojs code base has only one place where it uses external strings and there it's just an accident of history. |
I guess this could work as a temporary fix to keep things compiling so we can release NAN 1.5, but I want it gone in NAN 2.0, which should succeed 1.5. The only code in NAN using it is However, can you rebase against the onepointfive branch? |
I probably need to file a new PR for that. GH always gets terribly confused when you change branches halfway through a PR. |
Continues in #224. |
io.js ships a newer V8 version where String::ExternalAsciiStringResource
has been replaced with String::ExternalOneByteStringResource.
Use SFINAE to make nan compatible with a range of V8 versions.
Fixes #222.
R=@kkoopa