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

Ported to NAN 1.2.0 #256

Merged
merged 1 commit into from
Jun 17, 2014
Merged

Ported to NAN 1.2.0 #256

merged 1 commit into from
Jun 17, 2014

Conversation

kkoopa
Copy link
Contributor

@kkoopa kkoopa commented May 22, 2014

This addresses #252. Uses NAN 1.2.0. Tests pass on 0.10.28 and 0.11.13.

@springmeyer
Copy link
Member

wow wow wow @kkoopa this is incredible. thank you!

A few questions right off:

  • mixed use of NanNew<string>("string") and just NanNew("string"). Is there a reason for this?.
  • v8::String::NewSymbol is not used by NanNew<String> right? If I'm understanding right I assume this is because NewSymbol is not recommended anymore given nodejs/node-v0.x-archive@f674b09?
  • (args.This()); -> (args.Holder());. Thanks for making these changes - from my understanding (mostly from ObjectWrap/NODE_SET_PROTOTYPE_METHOD/node::SetPrototypeMethod is unsafe nodejs/node-v0.x-archive#6690) its always best to use Holder - is that right?
  • ->ToObject() -> .As<Object>(). So .As works with older node and is best practice? I've noticed that either could be used but never paid close attention.
  • Color::constructor->HasInstance(obj) -> NanNew(Color::constructor)->HasInstance(obj) - can you tell me more about why this is needed?

@kkoopa
Copy link
Contributor Author

kkoopa commented May 22, 2014

  • No reason, just mix of manual porting and automated search and replace.This particular overload of NanNew will be introduced in 1.1, it is just a shorthand for a common use case.
  • Correct, NanSymbol will be deprecated in 1.1 for those precise reasons and because String::NewSymbol is no more in node 0.11. NAN 1.0.0 mapped it to String::New for node 0.11, but we decided to deprecate and remove it instead.
  • I don't really know myself. I changed those with ObjectWrap::Unwrap to use Holder(), but others still use This()
  • As<T>() is slightly faster, as it skips some checks that T::Cast() performs
  • Local<T> NanPersistentToLocal(const Persistent<T>&) was merged into Local<T> NanNew(const Persistent<T>&)

@springmeyer
Copy link
Member

excellent, thank you - all sounds good. So, do you have a timeline on the next Nan 1.1.0 release?

@kkoopa
Copy link
Contributor Author

kkoopa commented May 22, 2014

@rvagg promised a release earlier this week, maybe he can find time to do it tomorrow?

If you would like to try it out right now, you can change it to depend on NAN master.

@springmeyer
Copy link
Member

Tested against Nan master and it works perfectly. Will plan to merge this once a Nan release is out - but no rush on that. Thanks again!

@rvagg
Copy link

rvagg commented May 23, 2014

yes, I've just settled down in Melbourne for the weekend and this is near the top of my list. release coming soon!

@springmeyer
Copy link
Member

Looks like 1.2.0 is out? nodejs/nan@c12da50. @kkoopa Any adjustments needed before merging?

@mikemorris mikemorris mentioned this pull request Jun 9, 2014
3 tasks
@diorahman
Copy link

Tried to "merge" the last commit, with this PR to my own fork: https://github.com/rockybars/node-mapnik/compare/nan-1.2.0 but it couldn't pass at following tests:

  1) mapnik.VectorTile  should be able to clear data (sync):
     Error: serialization failed, possible race condition
  2) mapnik.VectorTile  should be able to clear data (async):
     Uncaught Error: serialization failed, possible race condition
  3) mapnik.VectorTile  should be able to add data:
     Uncaught Error: serialization failed, possible race condition
  4) mapnik.VectorTile  should be able to push an image tile directly into a vector tile layer without decoding:
     AssertionError: 11687 == 12146
  5) mapnik.VectorTile  should include image in getData pbf output:
     AssertionError: 11687 == 12146

The build job:
https://travis-ci.org/rockybars/node-mapnik/jobs/27408391

@kkoopa
Copy link
Contributor Author

kkoopa commented Jun 12, 2014

No adjustments needed, went ahead and changed the used version in package.json

All tests pass for me.

@kkoopa kkoopa changed the title Ported to NAN 1.1.0 Ported to NAN 1.2.0 Jun 12, 2014
@springmeyer springmeyer merged commit 203fb38 into mapnik:master Jun 17, 2014
@springmeyer
Copy link
Member

merged into master now :) Thank you!

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.

4 participants