Skip to content

Property accessors 6.2.0 #45

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

Merged
merged 1 commit into from
Jan 23, 2017

Conversation

jasongin
Copy link
Member

See #44

This change adds a new napi_set_accessor API which sets getter and setter accessors for a property on an object. In support of that, I refactored some of the callback-related code: the callback info opaque pointer type is now shared between function callbacks and accessor callbacks, so I renamed it from napi_func_cb_info to napi_callback_info, and renamed a few other things accordingly. In the internal implementation, there is a new CallbackInfoWrapper and subclasses that wrap the v8 FunctionCallbackInfo and PropertyCallbackInfo classes.

I assume we're not too concerned about breaking changes at this point, though of course any existing code using NAPI will need to be updated. I am updating the abi-stable-node-addon-examples repo for these changes, along with new examples for using property accessors.

@jasongin jasongin changed the title Property accessors Property accessors 6.2.0 Jan 12, 2017
@jasongin
Copy link
Member Author

Based on I've learned today I'm aware of at least a couple pieces missing from this PR:

  1. NAPI_GETTER and NAPI_SETTER macros in node_api_helpers.h
  2. Changes to napi_create_constructor_for_wrap_with_methods to also support accessors.

I'm working on these, but meanwhile I'd still appreciate feedback on the rest of the changes.

mhdawson

This comment was marked as off-topic.

@mhdawson
Copy link
Member

On the breaking changes front, agreed we still need to make breaking changes and as you say we need to then update our existing module ports and the add on examples so they continue to work.

On that front I'm thinking we might want to set up some ci jobs, I'll think about that a bit more when I get back from vacation.

@jasongin
Copy link
Member Author

Regarding CallbackInfoWrapper, I did spend some time trying to come up with something less repetitive. It's tricky because v8's FunctionCallbackInfo and PropertyCallbackInfo have a lot of the same methods but don't have any inheritance relationship. I tried doing something with templates, but it got very complicated to the point that I thought it wasn't worth it.

@jasongin
Copy link
Member Author

I pushed another commit that includes the following changes: 

  • Changed napi_method_descriptor to napi_property_descriptor and added several fields. This is now a C++ equivalent to the property descriptor object passed to the JS Object.defineProperty().
  • Updated napi_create_constructor() to take an array of property descriptors (instead of just the method descriptors), so it is able to define methods, accessors, and values for the class.
  • Added napi_define_property() that also takes a property descriptor and uses it to define a method, accessors, or value property on an object.
  • Refactored the internal callback wrapper helper classes and functions so they are cleaner and support passing a void* data pointer to any function or accessor callback.
  • Added napi_cb_get_data() to retrieve the callback data.
  • Added NAPI_GETTER and NAPI_SETTER macros in node_api_helpers.h. They're actually just the same as NAPI_METHOD because the args and return value are all handled via the callback info object in the same way. (Unlike nan where NAN_SETTER declares the value as a separate callback arg.)

I think it's ready for a full review now.

@jasongin
Copy link
Member Author

Oh, I still need to add/update tests covering these changes under test/addons-abi.

@jasongin
Copy link
Member Author

Pushed another commit with added/updated tests.

@gabrielschulhof
Copy link
Collaborator

LGTM

@jasongin
Copy link
Member Author

Waiting for the try-catch PR to be merged, then I will rebase this PR on top of that.

@jasongin
Copy link
Member Author

jasongin commented Jan 20, 2017

I squashed and rebased on top of the latest changes. but there are some test failures now. Please stand by.

 - Add support for property descriptors including accessor callbacks
 - Add a data pointer for function and accessor callbacks
 - Add API for defining a property using a property descriptor
 - Update the constructor API to use property descriptors
@jasongin
Copy link
Member Author

OK I fixed the problems after rebasing. This is ready now. Waiting for signoff from @boingoing who said he would review this afternoon.

boingoing

This comment was marked as off-topic.

boingoing

This comment was marked as off-topic.

@boingoing
Copy link

This looks very good to me. Thanks a lot for doing the work, especially for updating the tests and adding the data pointer to the callback info.

@jasongin jasongin merged commit 67f4581 into nodejs:api-prototype-6.2.0 Jan 23, 2017
@jasongin jasongin deleted the accessors-6 branch January 23, 2017 19:47
mhdawson

This comment was marked as off-topic.

@jasongin
Copy link
Member Author

Yes, we still need napi_set_property, because sometimes you just want to assign a value to a property (which will invoke any existing set accessor if there is one) without redefining the property.

jasongin added a commit to jasongin/abi-stable-node that referenced this pull request Jan 26, 2017
 - Add support for property descriptors including accessor callbacks
 - Add a data pointer for function and accessor callbacks
 - Add API for defining a property using a property descriptor
 - Update the constructor API to use property descriptors
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