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

Function calls #507

Merged
merged 23 commits into from Jun 20, 2020
Merged

Function calls #507

merged 23 commits into from Jun 20, 2020

Conversation

goto-bus-stop
Copy link
Member

@goto-bus-stop goto-bus-stop commented Mar 27, 2020

Implements most simple function calls between Rust and JS code. (no thread-safe callbacks yet!) Builds on #493.

With function calls, we can start porting tests from the legacy runtime to n-api. I ported tests for objects and functions here.

There are some notable differences between NAN and n-api in how they deal with functions.

  • With n-api, the "data" pointer can be just a pointer, and n-api handles wrapping it inside a v8::External value. n-api also unwraps it for you when you ask for the data pointer. To align the two runtimes, I moved the unwrapping of the v8::External for the NAN runtime into neon-sys. Functions that were used to get data out from the v8::External value now take void pointers. get_dynamic_callback() is now a no-op but I left it in so the data format can be changed later.
  • With NAN, you have to call SetReturn() to set the return value for a function call. With n-api, you can return an napi_value from your functions instead. The n-api runtime gets its own Callback impl and neon_runtime::call::set_return is unused.
    • When there is no return value, I'm returning a nullptr, but this should maybe be an undefined napi_value.

The FunctionCallbackInfo structure was one that could only be used as a reference. n-api has an opaque napi_callback_info type alias for a pointer, which could not be used in this way. I changed FunctionCallbackInfo to be a pointer instead.

  • Need to confirm what this means for lifetimes—perhaps it should be a FunctionCallbackInfo<'a>, so we can't store it for longer than the actual function call lasts?

@goto-bus-stop goto-bus-stop marked this pull request as ready for review May 25, 2020 11:12
Copy link
Collaborator

@dherman dherman left a comment

Choose a reason for hiding this comment

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

This is incredible, and so many tests, too! Thank you for this amazing PR.

I just made a few small suggestions.

crates/neon-runtime/src/napi/call.rs Outdated Show resolved Hide resolved
crates/neon-runtime/src/napi/call.rs Outdated Show resolved Hide resolved
crates/neon-runtime/src/napi/call.rs Outdated Show resolved Hide resolved
crates/neon-sys/native/src/neon.cc Show resolved Hide resolved
test/napi/lib/functions.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@dherman dherman left a comment

Choose a reason for hiding this comment

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

Looks great! We can ship as long as we get CI passing.

@dherman dherman merged commit 35c8b60 into neon-bindings:master Jun 20, 2020
@dherman
Copy link
Collaborator

dherman commented Jun 20, 2020

Looks like CI is passing, Travis just somehow isn't updating the PR. Merged! 🎉🎉🎉🎉🎉

@goto-bus-stop goto-bus-stop deleted the functions branch June 20, 2020 21:21
@goto-bus-stop goto-bus-stop mentioned this pull request Jun 22, 2020
26 tasks
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.

None yet

2 participants