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: Update property attrs enum to match JS spec #12240

Closed
wants to merge 3 commits into from

Conversation

@jasongin
Copy link
Contributor

commented Apr 5, 2017

The napi_property_attributes enum used names and values from
v8::PropertyAttribute, but those negative flag names were outdated
along with the default behavior of a property being writable,
enumerable, and configurable unless otherwise specified. To match the
ES5 standard property descriptor those attributes should be positive
flags and should default to false unless otherwise specified.

Fixes: nodejs/abi-stable-node#221

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

N-API

@jasongin

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2017

@mhdawson @TimothyGu please review

@mhdawson
Copy link
Member

left a comment

LGTM

@mscdex mscdex added the n-api label Apr 5, 2017

test/addons-napi/test_constructor/test_constructor.c Outdated
{ "readonlyValue", 0, 0, 0, number, napi_read_only, 0},
{ "hiddenValue", 0, 0, 0, number, napi_read_only | napi_dont_enum, 0},
{ "echo", Echo, 0, 0, 0, napi_enumerable, 0 },
{ "accessorValue", 0, GetValue, SetValue, 0, napi_enumerable, 0},

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Apr 6, 2017

Member

The accessorValue is not actually referenced in the JS layer. You should also find a way to make certain the getters/setters are actually called when getting/setting the property.

Also, tests that cover the normalization logic fully should be added.

test/addons-napi/test_properties/test_properties.c Outdated
{ "readonlyValue", 0, 0, 0, number, napi_read_only, 0 },
{ "hiddenValue", 0, 0, 0, number, napi_read_only | napi_dont_enum, 0 },
{ "echo", Echo, 0, 0, 0, napi_enumerable, 0 },
{ "accessorValue", 0, GetValue, SetValue, 0, napi_enumerable, 0},

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Apr 6, 2017

Member

Ditto.

src/node_api.cc Outdated
// but V8 requires the ReadOnly attribute to match existence of a setter.
attributes = static_cast<v8::PropertyAttribute>(p->setter != nullptr
? attributes & ~v8::PropertyAttribute::ReadOnly
: attributes | v8::PropertyAttribute::ReadOnly);

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Apr 6, 2017

Member

Your call, but I think it might be better if this is factored out?

This comment has been minimized.

Copy link
@jasongin

jasongin Apr 6, 2017

Author Contributor

Yeah I sort of wanted to do that, but it didn't seem right to put it in v8impl::V8PropertyAttributesFromAttributes() because the logic depends on the kind of property descriptor. I'll consider further what to do with this.

src/node_api.cc Outdated

if (p->method) {
if (p->method != nullptr) {

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Apr 6, 2017

Member

Slightly off-topic and basic, but are the C NULL and the C++ nullptr interoperable?

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 6, 2017

Member

Slightly off-topic and basic, but are the C NULL and the C++ nullptr interoperable?

Yup. :) The different names are just historical oddities because C++ NULL is not defined the same way as C NULL, so C++11 introduced nullptr as a direct equivalent to C NULL.

jasongin added 2 commits Apr 5, 2017
n-api: Update property attrs enum to match JS spec
The napi_property_attributes enum used names and values from
v8::PropertyAttribute, but those negative flag names were outdated
along with the default behavior of a property being writable,
enumerable, and configurable unless otherwise specified. To match the
ES5 standard property descriptor those attributes should be positive
flags and should default to false unless otherwise specified.

Fixes: nodejs/abi-stable-node#221

@jasongin jasongin force-pushed the jasongin:napi_property_attributes branch to 05586f4 Apr 6, 2017

@jasongin

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2017

@TimothyGu I pushed a commit to address your feedback, and also rebased.

@jasongin jasongin referenced this pull request Apr 6, 2017
3 of 3 tasks complete
@TimothyGu
Copy link
Member

left a comment

LGTM except for the nit

@@ -1,4 +1,4 @@
/******************************************************************************
/******************************************************************************

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Apr 6, 2017

Member

Do you have an idea what this is?

This comment has been minimized.

Copy link
@jasongin

jasongin Apr 6, 2017

Author Contributor

It's a UTF-8 BOM. VS 2017 likes to insert those for some reason. Fixed.

attribute_flags |= (descriptor->setter == nullptr ?
v8::PropertyAttribute::ReadOnly : v8::PropertyAttribute::None);
}
else if ((descriptor->attributes & napi_writable) == 0) {

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Apr 6, 2017

Member

nit: coalesce the right brace with else

This comment has been minimized.

Copy link
@jasongin

jasongin Apr 6, 2017

Author Contributor

Fixed.

@TimothyGu

This comment has been minimized.

@mhdawson

This comment has been minimized.

Copy link
Member

commented Apr 7, 2017

CI green, landing.

@mhdawson

This comment has been minimized.

Copy link
Member

commented Apr 7, 2017

Landed as 8460284

@mhdawson mhdawson closed this Apr 7, 2017

mhdawson added a commit that referenced this pull request Apr 7, 2017
n-api: Update property attrs enum to match JS spec
The napi_property_attributes enum used names and values from
v8::PropertyAttribute, but those negative flag names were outdated
along with the default behavior of a property being writable,
enumerable, and configurable unless otherwise specified. To match the
ES5 standard property descriptor those attributes should be positive
flags and should default to false unless otherwise specified.

PR-URL: #12240
Fixes: nodejs/abi-stable-node#221
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
@refack

This comment has been minimized.

Copy link
Member

commented Apr 7, 2017

Guys I suspect you have a failing test on windows
CI: https://ci.nodejs.org/job/node-test-commit/8961/
Lets see.

@refack

This comment has been minimized.

Copy link
Member

commented Apr 8, 2017

this is the diff between the PR and 8460284

test/addons/make-callback-recurse/test.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/addons/make-callback-recurse/test.js b/test/addons/make-callback-recurse/test.js
index 46b1327d7..895769bc3 100644
--- a/test/addons/make-callback-recurse/test.js
+++ b/test/addons/make-callback-recurse/test.js
@@ -15,7 +15,7 @@ assert.throws(function() {
   makeCallback({}, function() {
     throw new Error('hi from domain error');
   });
-});
+}, /^Error: hi from domain error$/);
 
 
 // Check the execution order of the nextTickQueue and MicrotaskQueue in

And it's close to what failed on me on the CI

@refack refack referenced this pull request Apr 8, 2017
0 of 3 tasks complete
@jasnell jasnell referenced this pull request May 11, 2017
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 11, 2018
n-api: Update property attrs enum to match JS spec
The napi_property_attributes enum used names and values from
v8::PropertyAttribute, but those negative flag names were outdated
along with the default behavior of a property being writable,
enumerable, and configurable unless otherwise specified. To match the
ES5 standard property descriptor those attributes should be positive
flags and should default to false unless otherwise specified.

PR-URL: nodejs#12240
Fixes: nodejs/abi-stable-node#221
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 31, 2018
n-api: Update property attrs enum to match JS spec
The napi_property_attributes enum used names and values from
v8::PropertyAttribute, but those negative flag names were outdated
along with the default behavior of a property being writable,
enumerable, and configurable unless otherwise specified. To match the
ES5 standard property descriptor those attributes should be positive
flags and should default to false unless otherwise specified.

PR-URL: nodejs#12240
Fixes: nodejs/abi-stable-node#221
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
n-api: Update property attrs enum to match JS spec
The napi_property_attributes enum used names and values from
v8::PropertyAttribute, but those negative flag names were outdated
along with the default behavior of a property being writable,
enumerable, and configurable unless otherwise specified. To match the
ES5 standard property descriptor those attributes should be positive
flags and should default to false unless otherwise specified.

PR-URL: nodejs#12240
Fixes: nodejs/abi-stable-node#221
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
MylesBorins added a commit that referenced this pull request Apr 16, 2018
n-api: Update property attrs enum to match JS spec
The napi_property_attributes enum used names and values from
v8::PropertyAttribute, but those negative flag names were outdated
along with the default behavior of a property being writable,
enumerable, and configurable unless otherwise specified. To match the
ES5 standard property descriptor those attributes should be positive
flags and should default to false unless otherwise specified.

Backport-PR-URL: #19447
PR-URL: #12240
Fixes: nodejs/abi-stable-node#221
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
@MylesBorins MylesBorins referenced this pull request Apr 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.