Skip to content
This repository has been archived by the owner on Jun 10, 2024. It is now read-only.

[WIP] Port to Nan (Support node v4/5) #73

Merged
merged 16 commits into from
Apr 1, 2016
Merged

[WIP] Port to Nan (Support node v4/5) #73

merged 16 commits into from
Apr 1, 2016

Conversation

springmeyer
Copy link
Contributor

This ports the node-osmium code to use Nan. The changes required were extensive and largely done by hand + some sed work.

Now the code should compile against both node v0.10.x, v0.12.x, v4, and v5.

But there may be bugs introduced, so we should test this well.

TODO before merging:

  • Something is mysteriously broken with error handling
  1) handler should catch errors in handler callbacks and rethrow from apply function:
     AssertionError: Missing expected exception (Error)..
      at Function._throws (assert.js:301:5)
      at Function.assert.throws (assert.js:318:11)
      at Context.<anonymous> (test/handler.test.js:252:16)
  2) handler should catch errors in handler callbacks and rethrow from apply function:
     AssertionError: Missing expected exception (Error)..
      at Function._throws (assert.js:301:5)
      at Function.assert.throws (assert.js:318:11)
      at Context.<anonymous> (test/handler.test.js:252:16)
  3) osm object creation Uncaught error outside test suite:
     Uncaught TypeError: Cannot call method 'currentRetry' of undefined

https://travis-ci.org/osmcode/node-osmium/jobs/119723899#L3192

@joto - can you take a look to try figure out what is wrong with the error handling/why those tests are failing? I've looked for many hours already and just don't see it.

Refs #37

/cc @rclark

@springmeyer springmeyer mentioned this pull request Mar 31, 2016
2 tasks
@joto
Copy link
Member

joto commented Mar 31, 2016

I had a look and suspect there is something wrong with either Node or Nan. Nan::MakeCallback swallows the exception. It is unclear to me whether that's intended or not. There are a slew of mightily confusing tickets on this issue:

I tried different using nan::Call and nan::Callback instead of Nan::MakeCallback, but couldn't get it to work.

@springmeyer
Copy link
Contributor Author

@joto - thanks for digging in. Nan::MakeCallback swallowing the exception sounds likely. Can you share what you tried with nan::Call and nan::Callback and I'll poke a bit more when I have time?

/cc @rclark - overall this might be ready for your experimental use (as long as no javascript errors are thrown from within your handlers).

@joto
Copy link
Member

joto commented Mar 31, 2016

@springmeyer I simply replaced Nan::MakeCallback() with nan::Call(...) in the two places or with nan::Callback callback{...}; callback();. I tried a few other things, putting the TryCatch in other places, etc. but that doesn't change, that the trycatch.HasCaught() is never true.

@springmeyer
Copy link
Contributor Author

572fd27 fixes the failing tests in test/handler.test.js for me locally. Now all tests still pass on node v0.10. while one fails on node v4:

  1) location handler should throw on missing location if ignoreErrors is not set:
     AssertionError: Missing expected exception (Error)..
      at Function._throws (assert.js:306:5)
      at Function.assert.throws (assert.js:323:11)
      at Context.<anonymous> (test/location_handler.test.js:35:16)

@springmeyer
Copy link
Contributor Author

Going to merge as travis is now green: https://travis-ci.org/osmcode/node-osmium/builds/119961871 (ignore the coverage failure which is something to do with coveralls upstream).

@springmeyer springmeyer merged commit cf788cf into master Apr 1, 2016
@springmeyer springmeyer deleted the nan branch April 1, 2016 00:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants