n-api: define ECMAScript-compliant accessors on napi_define_* #27851

legendecas · May 24, 2019

Fixes: #26551
Fixes: nodejs/node-addon-api#485

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

addaleax · May 26, 2019

addaleax approved these changes May 26, 2019

View changes

@nodejs/n-api

src/js_native_api_v8.cc

nodejs-github-bot · May 26, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/23348/
CI: https://ci.nodejs.org/job/node-test-pull-request/23870/

mhdawson · May 27, 2019

Overall I think this looks good. Since it is changing behaviour and N-API modules should run across LTS versions I think I should ask how likely it is to break existing code. My assumption is extremely low as it is changing from behaviour that is unexpected and to what should be expected. Still would like @legendecas and @addaleax to weigh in on that front.

devsnek · May 27, 2019

devsnek approved these changes May 27, 2019

View changes

TimothyGu · May 27, 2019

TimothyGu reviewed May 27, 2019

View changes

The existing change looks good. You should probably change napi_define_class()

node/src/js_native_api_v8.cc

Lines 804 to 838 in 1514b11

    
           v8::PropertyAttribute attributes = 
        
               v8impl::V8PropertyAttributesFromDescriptor(p); 
        
           // This code is similar to that in napi_define_properties(); the 
        
           // difference is it applies to a template instead of an object, 
        
           // and prefered PropertyAttribute for lack of Object::DefineProperty 
        
           // support. 
        
           if (p->getter != nullptr || p->setter != nullptr) { 
        
             v8::Local<v8::Value> cbdata = v8impl::CreateAccessorCallbackData( 
        
               env, p->getter, p->setter, p->data); 
        
             tpl->PrototypeTemplate()->SetAccessor( 
        
               property_name, 
        
               p->getter ? v8impl::GetterCallbackWrapper::Invoke : nullptr, 
        
               p->setter ? v8impl::SetterCallbackWrapper::Invoke : nullptr, 
        
               cbdata, 
        
               v8::AccessControl::DEFAULT, 
        
               attributes); 
        
           } else if (p->method != nullptr) { 
        
             v8::Local<v8::Value> cbdata = 
        
                 v8impl::CreateFunctionCallbackData(env, p->method, p->data); 
        
             RETURN_STATUS_IF_FALSE(env, !cbdata.IsEmpty(), napi_generic_failure); 
        
             v8::Local<v8::FunctionTemplate> t = 
        
               v8::FunctionTemplate::New(isolate, 
        
                 v8impl::FunctionCallbackWrapper::Invoke, 
        
                 cbdata, 
        
                 v8::Signature::New(isolate, tpl)); 
        
             tpl->PrototypeTemplate()->Set(property_name, t, attributes); 
        
           } else { 
        
             v8::Local<v8::Value> value = v8impl::V8LocalValueFromJsValue(p->value); 
        
             tpl->PrototypeTemplate()->Set(property_name, value, attributes); 
        
           }

the same way, and then remove V8PropertyAttributesFromDescriptor and CreateAccessorCallbackData.

src/js_native_api_v8.cc

legendecas · May 28, 2019

@TimothyGu: You should probably change napi_define_class()

AFAIK, there would be an error on invoking Object.getOwnPropertyNames/Object.getOwnPropertyDescriptors on the propotype of result of napi_define_class.

A case could be got hands on:

/node/test/js-native-api/6_object_wrap/test.js:15
var proto = Object.getPrototypeOf(obj)
Object.getOwnPropertyDescriptors(proto)
^

Error: Invalid argument
    at Function.getOwnPropertyDescriptors (<anonymous>)
    at Object.<anonymous> (/node/test/js-native-api/6_object_wrap/test.js:15:20)

And v8 doesn't provide an API to define ECMA compliant accessors on the ObjectTemplate. Would be pleased to know if you have any thoughts on that.

TimothyGu · May 28, 2019

@legendecas

And v8 doesn't provide an API to define ECMA compliant accessors on the ObjectTemplate. Would be pleased to know if you have any thoughts on that.

V8 does support that, as ObjectTemplate inherits from Template which has a SetAccessorProperty method. See

node/src/env-inl.h

Lines 1027 to 1040 in aa42d37

    
           inline void Environment::SetProtoMethod(v8::Local<v8::FunctionTemplate> that, 
        
                                                   const char* name, 
        
                                                   v8::FunctionCallback callback) { 
        
             v8::Local<v8::Signature> signature = v8::Signature::New(isolate(), that); 
        
             v8::Local<v8::FunctionTemplate> t = 
        
                 NewFunctionTemplate(callback, signature, v8::ConstructorBehavior::kThrow, 
        
                                     v8::SideEffectType::kHasSideEffect); 
        
             // kInternalized strings are created in the old space. 
        
             const v8::NewStringType type = v8::NewStringType::kInternalized; 
        
             v8::Local<v8::String> name_string = 
        
                 v8::String::NewFromUtf8(isolate(), name, type).ToLocalChecked(); 
        
             that->PrototypeTemplate()->Set(name_string, t); 
        
             t->SetClassName(name_string);  // NODE_SET_PROTOTYPE_METHOD() compatibility. 
        
           }

for some examples on how its sibling function v8::Template::Set() is used. Note in particular the use of v8::Signature there, which makes things like Object.getOwnPropertyDescriptor(MyClass.prototype, 'prop').get.call(notMyClassObj) throw an error.

legendecas · May 28, 2019

@TimothyGu Thank you for your detailed explanation! I'll try to fix napi_define_class too.

legendecas · Jun 3, 2019

I have updated the napi_define_class, please take a look @TimothyGu :)

legendecas · Jun 11, 2019

ping n-api team, please take a look :)

TimothyGu · Jun 11, 2019

TimothyGu approved these changes Jun 11, 2019

View changes

Looks good, thanks!

mhdawson · Jun 13, 2019

Was hoping for some feedback from @legendecas and @addaleax in terms of how much risk there is in terms of breaking existing code?

legendecas · Jun 13, 2019

For napi_define_properties, I would assume it should define accessor pattern property for getter/setter by its name (same as Object.defineOwnProperties), and it is documented to define property as defined by DefineOwnProperty.

For napi_define_class, we could get hands on the property descriptors defined by napi_define_class with the patch, rather than an unexpected error on calling Object.getOwnPropertyDescriptor.

Thus there would not be noticeable breaking on existing code IMHO.

mhdawson · Jun 13, 2019

@legendecas I did validate that the current patch fixes nodejs/node-addon-api#485. It does break one of the node-addon-api tests but I think its a test problem so I'll look at fixing that.

addaleax · Jun 13, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/23870/

mhdawson · Jun 13, 2019

mhdawson approved these changes Jun 13, 2019

View changes

LGTM

mhdawson · Jun 13, 2019

Looks good but we should land nodejs/node-addon-api#495 first so that we don't break the node-addon-api tests.

mhdawson · Jun 13, 2019

Added the blocked label until we get the node-add-api fix in. Will try to expedite that.

mhdawson · Jun 13, 2019

Removed blocked label as update to node-addon-api testcase is now landed.

BridgeAR · Jun 17, 2019

Landed in 4329585 and 10a346e 🎉

nodejs-github-bot added the C++ label May 24, 2019

legendecas force-pushed the legendecas:napi_define_properties branch 2 times, most recently from 1deb5fc to 1dfa776 May 24, 2019

addaleax added the n-api label May 26, 2019

legendecas force-pushed the legendecas:napi_define_properties branch 2 times, most recently from 0144dfe to 1514b11 May 27, 2019

mhdawson referenced this pull request May 28, 2019
Closed
Object.getOwnPropertyDescriptor throws on accessor properties of an ObjectWrap class' prototype object #485

legendecas force-pushed the legendecas:napi_define_properties branch 2 times, most recently from b7f2dd7 to 5efe42c May 29, 2019

n-api: define ECMAScript-compliant accessors on napi_define_properties
Fixes: #26551

Verified

This commit was signed with a verified signature.

legendecas legendecas

GPG key ID: 321332BDA88D6AA3 Learn about signing commits

be84fae

legendecas force-pushed the legendecas:napi_define_properties branch from 5efe42c to 0f22b86 May 29, 2019

legendecas changed the title ~~n-api: define ECMAScript-compliant accessors on napi_define_properties~~ n-api: define ECMAScript-compliant accessors on napi_define_* May 29, 2019

legendecas force-pushed the legendecas:napi_define_properties branch 2 times, most recently from d1731b7 to fdea3b6 May 29, 2019

n-api: define ECMAScript-compliant accessors on napi_define_class
Fixes: nodejs/node-addon-api#485

Verified

This commit was signed with a verified signature.

legendecas legendecas

GPG key ID: 321332BDA88D6AA3 Learn about signing commits

Loading status checks…

2dfecc4

legendecas force-pushed the legendecas:napi_define_properties branch from fdea3b6 to 2dfecc4 May 29, 2019

addaleax added the author ready label Jun 13, 2019

mhdawson referenced this pull request Jun 13, 2019
Closed
src: fix objectwrap test case #495

mhdawson added the blocked label Jun 13, 2019

mhdawson removed the blocked label Jun 13, 2019

BridgeAR closed this Jun 17, 2019

legendecas deleted the legendecas:napi_define_properties branch Jun 17, 2019

BridgeAR referenced this pull request Jun 17, 2019
Merged
v12.5.0 proposal #28268

nodejs/node

Perfect your code

n-api: define ECMAScript-compliant accessors on napi_define_* #27851

Conversation

legendecas commented May 24, 2019 • edited

Checklist

nodejs-github-bot added the C++ label May 24, 2019

legendecas force-pushed the legendecas:napi_define_properties branch 2 times, most recently from 1deb5fc to 1dfa776 May 24, 2019

addaleax added the n-api label May 26, 2019

addaleax approved these changes May 26, 2019 View changes

addaleax left a comment

This comment has been minimized.

nodejs-github-bot commented May 26, 2019 • edited

legendecas force-pushed the legendecas:napi_define_properties branch 2 times, most recently from 0144dfe to 1514b11 May 27, 2019

This comment has been minimized.

mhdawson commented May 27, 2019

devsnek approved these changes May 27, 2019 View changes

TimothyGu reviewed May 27, 2019 View changes

TimothyGu left a comment

This comment has been minimized.

legendecas commented May 28, 2019

This comment has been minimized.

TimothyGu commented May 28, 2019 • edited

This comment has been minimized.

legendecas commented May 28, 2019

mhdawson referenced this pull request May 28, 2019

Object.getOwnPropertyDescriptor throws on accessor properties of an ObjectWrap class' prototype object #485

legendecas force-pushed the legendecas:napi_define_properties branch 2 times, most recently from b7f2dd7 to 5efe42c May 29, 2019

legendecas force-pushed the legendecas:napi_define_properties branch from 5efe42c to 0f22b86 May 29, 2019

legendecas changed the title n-api: define ECMAScript-compliant accessors on napi_define_properties n-api: define ECMAScript-compliant accessors on napi_define_* May 29, 2019

legendecas force-pushed the legendecas:napi_define_properties branch 2 times, most recently from d1731b7 to fdea3b6 May 29, 2019

legendecas force-pushed the legendecas:napi_define_properties branch from fdea3b6 to 2dfecc4 May 29, 2019

This comment has been minimized.

legendecas commented Jun 3, 2019

This comment has been minimized.

legendecas commented Jun 11, 2019

TimothyGu approved these changes Jun 11, 2019 View changes

TimothyGu left a comment

This comment has been minimized.

mhdawson commented Jun 13, 2019

This comment has been minimized.

legendecas commented Jun 13, 2019 • edited

This comment has been minimized.

mhdawson commented Jun 13, 2019

addaleax added the author ready label Jun 13, 2019

This comment has been minimized.

addaleax commented Jun 13, 2019

mhdawson referenced this pull request Jun 13, 2019

src: fix objectwrap test case #495

mhdawson approved these changes Jun 13, 2019 View changes

mhdawson left a comment

This comment has been minimized.

mhdawson commented Jun 13, 2019

mhdawson added the blocked label Jun 13, 2019

This comment has been minimized.

mhdawson commented Jun 13, 2019

mhdawson removed the blocked label Jun 13, 2019

This comment has been minimized.

mhdawson commented Jun 13, 2019

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jun 17, 2019

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jun 17, 2019

This comment has been minimized.

BridgeAR commented Jun 17, 2019

BridgeAR closed this Jun 17, 2019

legendecas deleted the legendecas:napi_define_properties branch Jun 17, 2019

BridgeAR added a commit that referenced this pull request Jun 17, 2019

BridgeAR added a commit that referenced this pull request Jun 17, 2019

BridgeAR referenced this pull request Jun 17, 2019

v12.5.0 proposal #28268

targos added a commit that referenced this pull request Jun 18, 2019

targos added a commit that referenced this pull request Jun 18, 2019

BridgeAR added a commit that referenced this pull request Jun 26, 2019

BridgeAR added a commit that referenced this pull request Jun 26, 2019

BridgeAR added a commit that referenced this pull request Jun 26, 2019

BridgeAR added a commit that referenced this pull request Jun 27, 2019

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jun 27, 2019

legendecas commented May 24, 2019 •

edited

legendecas force-pushed the legendecas:napi_define_properties branch 2 times, most recently from `1deb5fc` to `1dfa776` May 24, 2019

addaleax approved these changes May 26, 2019

View changes

nodejs-github-bot commented May 26, 2019 •

edited

legendecas force-pushed the legendecas:napi_define_properties branch 2 times, most recently from `0144dfe` to `1514b11` May 27, 2019

devsnek approved these changes May 27, 2019

View changes

TimothyGu reviewed May 27, 2019

View changes

TimothyGu commented May 28, 2019 •

edited

legendecas force-pushed the legendecas:napi_define_properties branch 2 times, most recently from `b7f2dd7` to `5efe42c` May 29, 2019

legendecas force-pushed the legendecas:napi_define_properties branch from `5efe42c` to `0f22b86` May 29, 2019

legendecas force-pushed the legendecas:napi_define_properties branch 2 times, most recently from `d1731b7` to `fdea3b6` May 29, 2019

legendecas force-pushed the legendecas:napi_define_properties branch from `fdea3b6` to `2dfecc4` May 29, 2019

TimothyGu approved these changes Jun 11, 2019

View changes

legendecas commented Jun 13, 2019 •

edited

mhdawson approved these changes Jun 13, 2019

View changes