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

Make compatible with io.js #223

Closed
wants to merge 1 commit into from
Closed

Make compatible with io.js #223

wants to merge 1 commit into from

Conversation

bnoordhuis
Copy link
Member

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

@kkoopa
Copy link
Collaborator

kkoopa commented Dec 14, 2014

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 NanNew. Could you rebase against that?

@bnoordhuis
Copy link
Member Author

Seems I hit a g++ 4.8 bug. I've pushed a fix-up that should fare better.

@bnoordhuis
Copy link
Member Author

@kkoopa @rvagg PTAL, it's #ifdef'ing on the node version now. The first commit removes the overloads for String::ExternalAsciiStringResource (the commit log explains why). The second commit brings them back in case you think it's best to keep them.

@rvagg
Copy link
Member

rvagg commented Jan 12, 2015

@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.

@rvagg rvagg mentioned this pull request Jan 12, 2015
14 tasks
@kkoopa
Copy link
Collaborator

kkoopa commented Jan 12, 2015

Sure, I should be around, unless something unexpected happens.

On Monday 12 January 2015 01:46:29 Rod Vagg wrote:

@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.


Reply to this email directly or view it on GitHub:
#223 (comment)

@kkoopa
Copy link
Collaborator

kkoopa commented Jan 12, 2015

Has NODE_MODULE_VERSION been updated to 13? If so, is it not better to branch on that? I was notified that v8 has moved around some things as well: At some point, ContextDisposedNotification has moved from v8::V8 to the current Isolate. Has anything else been moved (for no good reason)?

@bnoordhuis
Copy link
Member Author

Has NODE_MODULE_VERSION been updated to 13?

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.

Has anything else been moved

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.

@bnoordhuis
Copy link
Member Author

I'll file a PR to bump NODE_MODULE_VERSION

Done and landed in nodejs/node#312.

@bnoordhuis
Copy link
Member Author

Okay, decision time. Do we:

  1. Want to keep the ExternalAsciiStringResource stuff around, with the caveat that people will have to update their code some day anyway (only a temporary respite), and
  2. Should this PR use NODE_MODULE_VERSION or NODE_VERSION_AT_LEAST? I have a slight but mostly irrational preference for the latter.

@rvagg
Copy link
Member

rvagg commented Jan 12, 2015

one pro for using NODE_MODULE_VERSION is that it sets a better example for addon authors who are browsing through nan.h and learning from it, you shouldn't have to know what version ABI compat chaged, just that it changed.

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.
@bnoordhuis
Copy link
Member Author

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?

@rvagg
Copy link
Member

rvagg commented Jan 12, 2015

re ExternalAsciiStringResource, leave it for now I think so this isn't technically a "breaking" release

@bnoordhuis
Copy link
Member Author

Updated, PTAL.

@kkoopa
Copy link
Collaborator

kkoopa commented Jan 12, 2015

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 notifications@github.com wrote:

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?


Reply to this email directly or view it on GitHub:
#223 (comment)

@bnoordhuis
Copy link
Member Author

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?

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.

@kkoopa
Copy link
Collaborator

kkoopa commented Jan 13, 2015

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 _NanGetExternalParts, which is part of the deprecated NanRawString chain and should be removed anyway.

However, can you rebase against the onepointfive branch?

@bnoordhuis
Copy link
Member Author

I probably need to file a new PR for that. GH always gets terribly confused when you change branches halfway through a PR.

@bnoordhuis
Copy link
Member Author

Continues in #224.

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

Successfully merging this pull request may close these issues.

ExternalAsciiStringResource is gone in upstream V8
3 participants