Skip to content

Complete initial port to N-API#57

Closed
NickNaso wants to merge 5 commits intowadey:napifrom
NickNaso:napi
Closed

Complete initial port to N-API#57
NickNaso wants to merge 5 commits intowadey:napifrom
NickNaso:napi

Conversation

@NickNaso
Copy link
Copy Markdown
Contributor

@NickNaso NickNaso commented Mar 16, 2018

Initial work to support the new N-API as reported here:
#56

@NickNaso NickNaso mentioned this pull request Mar 21, 2018
Comment thread src/microtime.cc Outdated

if (r < 0) {
return Nan::ThrowError(Nan::ErrnoException(errno, "gettimeofday"));
throw Napi::Error::New(info.Env(), "gettimeofday");
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old code here retained the errno error code from C land in the error message, does the new Napi::Error does this automatically or is there a way we can preserve that behavior from nan?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @wadey,
you are right so I just updated the interested code. Now I create the error and than I set the code field like reported here:

Napi::Error e = Napi::Error::New(info.Env(), "gettimeofday");
e.Set("code", Napi::Number::New(info.Env(), errno));
throw e;

@wadey
Copy link
Copy Markdown
Owner

wadey commented Mar 29, 2018

I've fixed up the errno exception a bit more, can I push the improved version directly to your branch so it's part of this PR?

@NickNaso
Copy link
Copy Markdown
Contributor Author

Hi @wadey,
for me it's ok.

At least include the `strerror` result as the error message. This is not
perfect, but copying all of Node::ErrnoException would be a lot of code.
This is 90% there and we don't expect errors to happen anyways.
@wadey
Copy link
Copy Markdown
Owner

wadey commented Mar 29, 2018

Ok, I have pushed my patch for a better ErrnoException. Let me read up on the deployment process for napi and I will merge this soon. Thanks!

@NickNaso
Copy link
Copy Markdown
Contributor Author

Thanks to you!

@NickNaso
Copy link
Copy Markdown
Contributor Author

Hi @wadey,
when you publish the tagged version of the module please send me a notice or comment on the related issue.
Thanks!

@gabrielschulhof
Copy link
Copy Markdown

@wadey have you had a chance to take a look at this N-API port?

@wadey
Copy link
Copy Markdown
Owner

wadey commented Jan 31, 2019

This was merged into my https://github.com/wadey/node-microtime/tree/napi branch, which will soon be merged into master. Thank you!

@wadey wadey closed this Jan 31, 2019
@wadey
Copy link
Copy Markdown
Owner

wadey commented Jan 31, 2019

For anyone following this ticket, please follow issue #56 instead. You can also test now with the n-api npm tag, latest release is 3.0.0-1. This will soon be released as 3.0.0.

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.

3 participants