InstanceMember creates read only, not enumerable and not configurable properties on default

[Flarna]

InstanceMember uses napi_default for attributes if nothing else is specified.

napi_default means read only, not enumerable and not configurable.

The corresponding NAN API SetPrototypeMethod calls PrototypeTemplate()->Set() and to my understanding of the v8 API they use PropertyAttribute::None which is defined inverted:

enum PropertyAttribute {
  /** None. **/
  None = 0,
  /** ReadOnly, i.e., not writable. **/
  ReadOnly = 1 << 0,
  /** DontEnum, i.e., not enumerable. **/
  DontEnum = 1 << 1,
  /** DontDelete, i.e., not configurable. **/
  DontDelete = 1 << 2
};

Is this difference intended? In general non configurable properties are quite uncommon in JS world.

FWIW: If I define a class in javascript members are configureable, writable and not enumerable.

[NickNaso]

Hi @Flarna,
today I tried to define a class and then print how his attributes has been set. With JavaScript on Node.js, Chrome and Firefox I obtained that the instance method is set writable and configurable by default.

'use strict'

class Class {
  method() {}
}

console.log(Object.getOwnPropertyDescriptors(Class.prototype))

{
 constructor: {
    value: [class Class],
    writable: true,
    enumerable: false,
    configurable: true
  },
  method: {
    value: [Function: method],
    writable: true,
    enumerable: false,
    configurable: true
  }
}

I did the same experiment for native add-on and I obtained the following result:

{
 method: {
    value: [Function: method],
    writable: false,
    enumerable: false,
    configurable: false
  },
  constructor: {
    value: [Function: Class],
    writable: true,
    enumerable: false,
    configurable: true
  }
}

I want to discuss about this in tomorrow meeting. Thanks for reporting.

[mhdawson]

We discussed in the N-API team meeting and its a result of history and trying to match the JavaScript spec but seems like we did not end up with the default (in both the C and C++ wrapper) we might have chosen thinking about it now.

At this point though we don't want to change the default as that will affect existing addons.

One idea is to introduce a new enum value napi_open_default to the napi_property_attributes. @Flarna would this help/be useful? You would still have to specify it, but it would be a bit easier.

[Flarna]

For the C-API I think there should be two new members:
napi_method_default which is writable: true, configurable: true, enumerable: false
napi_member_default which is writable: true, configurable: true, enumerable: true
Besides adding them doc should clearly state that they are preferred as the result behaves then like a JS class and samples should be adapted accordingly.

For C++: To my understanding this module is not bound to the C release cycle. Therefore I think changing the defaults in the various InstanceValue, InstanceAccessor and InstanceValue could be done earlier/easier.
In the end it's no breaking change and a simple recompile would be enough to get rid of the behavior change introduced by moving from NAN to Napi. Or is this seen as a semver major change?

Maybe some background how I found this: I work on an APM tool and we monkey patch sqlite3. sqlite3 re-exports native classes (e.g. Database) without an JS wrapper inbetween. We patch prototype methods of this class.
Since v5 sqlite3 uses napi and as a result patching is no longer possible like it is with v4 using NAN.

I guess that a lot addons use a JS wrapper class to do at least some type checking/data preparation on JS side before calling to native. Therefore issues like I saw it with sqlite3 are most likely not that frequent.

[mhdawson]

For C++: To my understanding this module is not bound to the C release cycle. Therefore I think changing the defaults in the various InstanceValue, InstanceAccessor and InstanceValue could be done earlier/easier.
In the end it's no breaking change and a simple recompile would be enough to get rid of the behavior change introduced by moving from NAN to Napi. Or is this seen as a semver major change?

It is easier, but we are still concerned about changing the behaviour of existing code. If the author meant for more restricted access then we'd be breaking that. From that perspective I still see it as SemVer major and even though we have more flexibility in node-addon-api we try to minimize/avlid any SemVer majors. I do think some shortcuts like you PR'd into core make sense though.

[gabrielschulhof]

This has now landed in core at nodejs/node@c9506a8.