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

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

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
7 participants
@legendecas
Copy link
Contributor

commented 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

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

@addaleax addaleax added the n-api label May 26, 2019

@addaleax
Copy link
Member

left a comment

Show resolved Hide resolved src/js_native_api_v8.cc Outdated

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

@mhdawson

This comment has been minimized.

Copy link
Member

commented 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.

@TimothyGu
Copy link
Member

left a comment

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

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.

Show resolved Hide resolved src/js_native_api_v8.cc Outdated
@legendecas

This comment has been minimized.

Copy link
Contributor Author

commented 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

This comment has been minimized.

Copy link
Member

commented 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

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

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

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

@legendecas 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 legendecas force-pushed the legendecas:napi_define_properties branch 2 times, most recently from d1731b7 to fdea3b6 May 29, 2019

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

@legendecas

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

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

@legendecas

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

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

@TimothyGu
Copy link
Member

left a comment

Looks good, thanks!

@mhdawson

This comment has been minimized.

Copy link
Member

commented 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

This comment has been minimized.

Copy link
Contributor Author

commented 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

This comment has been minimized.

Copy link
Member

commented 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

This comment has been minimized.

@mhdawson
Copy link
Member

left a comment

LGTM

@mhdawson

This comment has been minimized.

Copy link
Member

commented 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 mhdawson added the blocked label Jun 13, 2019

@mhdawson

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

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

@mhdawson mhdawson removed the blocked label Jun 13, 2019

@mhdawson

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

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

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

n-api: define ECMAScript-compliant accessors on napi_define_properties
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 added a commit to BridgeAR/node that referenced this pull request Jun 17, 2019

n-api: define ECMAScript-compliant accessors on napi_define_class
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

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

Landed in 4329585 and 10a346e 🎉

@BridgeAR BridgeAR closed this Jun 17, 2019

@legendecas legendecas deleted the legendecas:napi_define_properties branch Jun 17, 2019

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

n-api: define ECMAScript-compliant accessors on napi_define_properties
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>

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

n-api: define ECMAScript-compliant accessors on napi_define_class
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>

@BridgeAR BridgeAR referenced this pull request Jun 17, 2019

Merged

v12.5.0 proposal #28268

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

n-api: define ECMAScript-compliant accessors on napi_define_properties
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>

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

n-api: define ECMAScript-compliant accessors on napi_define_class
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>

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

2019-06-18, Version 12.5.0 (Current)
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

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

2019-06-18, Version 12.5.0 (Current)
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

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

2019-06-26, Version 12.5.0 (Current)
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

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

2019-06-27, Version 12.5.0 (Current)
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

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

2019-06-27, Version 12.5.0 (Current)
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.