-
Notifications
You must be signed in to change notification settings - Fork 31
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
Upgraded to Nan 2.0 #90
Conversation
This would close #81 |
@@ -52,9 +52,9 @@ NAN_METHOD(protagonist::ParseSync) { | |||
drafter::ParseBlueprint(*sourceData, options, parseResult); | |||
|
|||
if (parseResult.report.error.code != snowcrash::Error::OK) { | |||
NanThrowError(SourceAnnotation::WrapSourceAnnotation(parseResult.report.error)); | |||
NanReturnUndefined(); | |||
Nan::ThrowError(SourceAnnotation::WrapSourceAnnotation(parseResult.report.error).As<v8::Value>()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.As<T>()
seems to be a problem in Node 0.10.x
See also: https://ci.appveyor.com/project/Apiary/protagonist/build/1.0.183#L2141
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am mostly sure that nan has a solution for this. We need to check thoroughly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can see, SourceAnnotation::WrapSourceAnnotation
is used to create a new object type for warnings and errors. It might be too much to have it return a Nan::Error
since it could be “just” a warning? But maybe it could return a v8::Local<v8::Value>
instead of Local<Object>
?
Build and test is passing on my machine
|
Adding to the list: nodejs/node#2798. Travis CI builds are failing because they use outdated gcc (4.6), while the minimum supported version is 4.8. This has to be configurable somehow on the Travis side, most likely in |
Travis build for Node 4.x now passes with gcc 4.8, but 0.10 & 0.8 still fails due to I'm on OS X, and it seems I'm using LLVM in place of gcc:
|
@@ -1,12 +1,23 @@ | |||
sudo: false | |||
language: node_js | |||
node_js: | |||
- "4.1" | |||
- "4.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have to give both the above versions. Please just add 4
and it should be enough.
Also, please cherry pick this commit and use it before your commits in this PR. 15b7c9d
So, current status is that tests are passing on all CI builds. Only outstanding issue, as far as I can see, is the wrapper introduced in 46851de to work around I don't know who (what code) is expected to consume the output of |
@pksunkara that's great. I've rebased, and it passes Travis CI. Notice: The signature of |
If you could publish this today - oh it would be sooo awesome. |
Do we need to do that? And yes, I am worried about how it will affect the code. As soon as that PR is merged, I will start looking into this. |
@pksunkara the reason for changing the signature was that passing
Someone pointed out that this might be a bug in Nan, since So perhaps the best way forward would be to try to actually exercise the code that handles warnings and errors, to see how it actually behaves. |
@tobiashm Can you create an issue in nan repo with the details from the above comment? I still haven't been able to find time to work on this. |
@pksunkara I've changed the code back so it doesn't change the signature for I've submitted an issue to Nan, but they think it must be a compiler bug. And that's too advanced for me to start looking into 🙀 |
Thanks for the change. The type casting is okay with me. I am now just waiting for #94 to be merged and we can rebase this PR and get this merged too. |
Can you rebase this over master and update the drafter submodule? Thanks. |
Nan 1.0 does not work with io.js 3.x and node.js >= 4.0, since they use newer versions of V8. *Warning:* This is an automatic conversion, using this script https://gist.github.com/thlorenz/7e9d8ad15566c99fd116 and some minor fixes by hand. The project builds and the tests passes, but I have no idea what I've done, so I can't guarantee for the correctness.
@pksunkara done. Updated drafter to current master. |
Thanks for the contribution @tobiashm 😄 |
Awesome |
Nice work! 👍 |
Good job guys! |
Will this be republished to |
@dsernst Yes, there should be an update in the coming week with this change. |
Good job guys! |
Nan 1.0 does not work with io.js 3.x and node.js >= 4.0, since they use newer versions of V8.
Warning:
This is an automatic conversion, using this script https://gist.github.com/thlorenz/7e9d8ad15566c99fd116 and some minor fixes by hand.
The project builds and the tests passes, but I have no idea what I've done, so I can't guarantee for the correctness.