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

legendecas · 2019-05-24T13:49:43Z

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

@nodejs/n-api

src/js_native_api_v8.cc

nodejs-github-bot · 2019-05-26T15:00:44Z

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

mhdawson · 2019-05-27T21:36:10Z

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.

TimothyGu

The existing change looks good. You should probably change napi_define_class() https://github.com/nodejs/node/blob/1514b117c6e1ecd36f1a3c6b044d5f8094be8b19/src/js_native_api_v8.cc#L804-L838 the same way, and then remove V8PropertyAttributesFromDescriptor and CreateAccessorCallbackData.

src/js_native_api_v8.cc

legendecas · 2019-05-28T04:01:53Z

@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 · 2019-05-28T04:38:55Z

@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 · 2019-05-28T04:48:18Z

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

Fixes: nodejs#26551

Fixes: nodejs/node-addon-api#485

legendecas · 2019-06-03T04:09:34Z

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

legendecas · 2019-06-11T09:49:01Z

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

TimothyGu

Looks good, thanks!

mhdawson · 2019-06-13T15:14:58Z

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

legendecas · 2019-06-13T15:31:21Z

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 · 2019-06-13T19:31:59Z

@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 · 2019-06-13T19:50:56Z

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

mhdawson

LGTM

mhdawson · 2019-06-13T20:14:44Z

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

mhdawson · 2019-06-13T20:16:16Z

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

mhdawson · 2019-06-13T21:47:09Z

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

PR-URL: nodejs#27851 Fixes: nodejs#26551 Fixes: nodejs/node-addon-api#485 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

BridgeAR · 2019-06-17T10:15:59Z

Landed in 4329585 and 10a346e 🎉

PR-URL: #27851 Fixes: #26551 Fixes: nodejs/node-addon-api#485 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Notable changes: This release contains `semver-major` commits. These are in fact not `semver-major` due to follow-up commits that remove all breaking changes. * build: * The startup time is reduced by enabling V8 snapshots by default #28181 * deps: * Updated `V8` to 7.5.288.22 #27375 * The numeric separator (v8.dev/features/numeric-separators) feature is now enabled by default * Updated `OpenSSL` to 1.1.1c #28211 * inspector: * The `--inspect-publish-uid` flag was added to specify ways of the inspector web socket url exposure #27741 * n-api: * Accessors on napi_define_* are now ECMAScript-compliant #27851 * report: * The cpu info got added to the report output #28188 * src: * Restore the original state of the stdio file descriptors on exit to prevent leaving stdio in raw or non-blocking mode #24260 * tools,gyp: * Introduce MSVS 2019 #27375 * util: * inspect: * Array grouping became more compact and uses more columns than before #28059 #28070 * Long strings will not be split at 80 characters anymore. Instead they will be split on new lines #28055 * worker: * `worker.terminate()` now returns a promise and using the callback is deprecated #28021 PR-URL: #28268

Notable changes: * build: * The startup time is reduced by enabling V8 snapshots by default #28181 * deps: * Updated `V8` to 7.5.288.22 #27375 * The numeric separator (v8.dev/features/numeric-separators) feature is now enabled by default * Updated `OpenSSL` to 1.1.1c #28211 * inspector: * The `--inspect-publish-uid` flag was added to specify ways of the inspector web socket url exposure #27741 * n-api: * Accessors on napi_define_* are now ECMAScript-compliant #27851 * report: * The cpu info got added to the report output #28188 * src: * Restore the original state of the stdio file descriptors on exit to prevent leaving stdio in raw or non-blocking mode #24260 * tools,gyp: * Introduce MSVS 2019 #27375 * util: * inspect: * Array grouping became more compact and uses more columns than before #28059 #28070 * Long strings will not be split at 80 characters anymore. Instead they will be split on new lines #28055 * worker: * `worker.terminate()` now returns a promise and using the callback is deprecated #28021 PR-URL: #28268

Notable changes: * build: * The startup time is reduced by enabling V8 snapshots by default nodejs#28181 * deps: * Updated `V8` to 7.5.288.22 nodejs#27375 * The numeric separator (v8.dev/features/numeric-separators) feature is now enabled by default * Updated `OpenSSL` to 1.1.1c nodejs#28211 * inspector: * The `--inspect-publish-uid` flag was added to specify ways of the inspector web socket url exposure nodejs#27741 * n-api: * Accessors on napi_define_* are now ECMAScript-compliant nodejs#27851 * report: * The cpu info got added to the report output nodejs#28188 * src: * Restore the original state of the stdio file descriptors on exit to prevent leaving stdio in raw or non-blocking mode nodejs#24260 * tools,gyp: * Introduce MSVS 2019 nodejs#27375 * util: * inspect: * Array grouping became more compact and uses more columns than before nodejs#28059 nodejs#28070 * Long strings will not be split at 80 characters anymore. Instead they will be split on new lines nodejs#28055 * worker: * `worker.terminate()` now returns a promise and using the callback is deprecated nodejs#28021 PR-URL: nodejs#28268

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label May 24, 2019

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

addaleax added the node-api Issues and PRs related to the Node-API. label May 26, 2019

addaleax approved these changes May 26, 2019

View reviewed changes

src/js_native_api_v8.cc Outdated Show resolved Hide resolved

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

devsnek approved these changes May 27, 2019

View reviewed changes

TimothyGu reviewed May 27, 2019

View reviewed changes

src/js_native_api_v8.cc Outdated Show resolved Hide resolved

mhdawson mentioned this pull request May 28, 2019

Object.getOwnPropertyDescriptor throws on accessor properties of an ObjectWrap class' prototype object nodejs/node-addon-api#485

Closed

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

n-api: define ECMAScript-compliant accessors on napi_define_properties

be84fae

Fixes: nodejs#26551

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

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 napi_define_properties branch 2 times, most recently from d1731b7 to fdea3b6 Compare May 29, 2019 05:55

n-api: define ECMAScript-compliant accessors on napi_define_class

2dfecc4

Fixes: nodejs/node-addon-api#485

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

TimothyGu approved these changes Jun 11, 2019

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 13, 2019

mhdawson mentioned this pull request Jun 13, 2019

src: fix objectwrap test case nodejs/node-addon-api#495

Closed

mhdawson approved these changes Jun 13, 2019

View reviewed changes

mhdawson added the blocked PRs that are blocked by other issues or PRs. label Jun 13, 2019

mhdawson removed the blocked PRs that are blocked by other issues or PRs. label Jun 13, 2019

BridgeAR closed this Jun 17, 2019

legendecas deleted the napi_define_properties branch June 17, 2019 10:23

BridgeAR mentioned this pull request Jun 17, 2019

v12.5.0 proposal #28268

Merged

Uh oh!

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

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

Uh oh!

Conversation

legendecas commented May 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodejs-github-bot commented May 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhdawson commented May 27, 2019

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

legendecas commented May 28, 2019

Uh oh!

TimothyGu commented May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

legendecas commented May 28, 2019

Uh oh!

legendecas commented Jun 3, 2019

Uh oh!

legendecas commented Jun 11, 2019

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Jun 13, 2019

Uh oh!

legendecas commented Jun 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhdawson commented Jun 13, 2019

Uh oh!

addaleax commented Jun 13, 2019

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Jun 13, 2019

Uh oh!

mhdawson commented Jun 13, 2019

Uh oh!

mhdawson commented Jun 13, 2019

Uh oh!

BridgeAR commented Jun 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

legendecas commented May 24, 2019 •

edited

Loading

nodejs-github-bot commented May 26, 2019 •

edited

Loading

TimothyGu commented May 28, 2019 •

edited

Loading

legendecas commented Jun 13, 2019 •

edited

Loading