Modernize napi_property_attributes enum names

[TimothyGu]

typedef enum {
  napi_default = 0,
  napi_read_only = 1 << 0,
  napi_dont_enum = 1 << 1,
  napi_dont_delete = 1 << 2,

  // Used with napi_define_class to distinguish static properties
  // from instance properties. Ignored by napi_define_properties.
  napi_static_property = 1 << 10,
} napi_property_attributes;

(source)

The ReadOnly, DontEnum, and DontDelete flag names are a legacy from ES3 §8.6.1, and were replaced in ES5 by the modern [[Writable]], [[Enumerable]], [[Configurable]]. I feel we should use the modern names instead of archaic names unfamiliar with most JS programmers today.

A large reason why the older nomenclature survived is V8's v8::PropertyAttribute-based APIs, but even there more modern alternatives using ES5 names (v8::PropertyDescriptor) have been developed.

[digitalinfinity]

Thanks @TimothyGu- I tend to agree. @jasongin @mhdawson @gabrielschulhof @boingoing @sampsongao do any of you recall why we chose this instead of the new nomenclature? Was it because it was based on v8::PropertyAttribute?

Side-note: Now that the N-API PR has landed in nodejs/node, @mhdawson- should N-API issues be opened in that repo or should we continue to use this one?

[jasongin]

I deliberately chose to design those attributes this way (and I assume v8::PropertyAttribute did also) because writable/configurable/enumerable should default to true. In a C flags-enum, a value of 0 should be the default value for an enum, to make it convenient to zero-initialize a structure, so that you get default attributes unless some other attributes are specified explicitly. So that means these flags have to be negatives.

Since N-API is a C API, not a C++ API like V8, it doesn't have the luxury of a constructor that can initialize those attributes to true, like v8::PropertyDescriptor does.

A JavaScript property descriptor doesn't have this problem because it treats undefined to be equivalent to true for those attributes.

[digitalinfinity]

While I buy the default value argument, I think you could have achieved the same differently too- namely, something like the following:

typedef enum {
  napi_default = 0,
  napi_writable = 1 << 0,
  napi_enumerable = 1 << 1,
  napi_configurable = 1 << 2,

  // Used with napi_define_class to distinguish static properties
  // from instance properties. Ignored by napi_define_properties.
  napi_static_property = 1 << 10,
} napi_property_attributes;

and in APIs where napi_property_attributes is needed, we check for napi_default and treat it as napi_writable | napi_enumerable | napi_configurable.

In any case, irrespective of the default attributes argument, I still lean towards the names being consistent (both within the enum, but also with the ECMA spec). If you were preserving your original semantics, something like the following perhaps?

typedef enum {
  napi_default = 0,
  napi_not_writable = 1 << 0,
  napi_not_enumerable = 1 << 1,
  napi_not_configurable = 1 << 2,

  // Used with napi_define_class to distinguish static properties
  // from instance properties. Ignored by napi_define_properties.
  napi_static_property = 1 << 10,
} napi_property_attributes;

[jasongin]

The first suggestion doesn't work because there is no way to specify a property that is not writable, not configurable, and not enumerable. Also, it's very strange for a value (napi_default) to be a combination of other flags, but not actually have the numerically combined value.

I don't feel strongly about the names, but given that the meanings were the same as v8::PropertyAttribute I thought it would be clearer to use the same names also.

[mhdawson]

There is some argument for consistency with V8 in terms of a transition for existing code, however, I think I'd lean towards consistency with the ECMA spec being better in the long run.

[mhdawson]

In terms of the questions on which repo to use, I think we should be moving towards using the core repo when possible (like this issue). We may want to finish out a few things in this repo like an initial cut at the docs, but once a component has made it into master lets discus in issues on master it should be more visible there.

[TimothyGu]

because writable/configurable/enumerable should default to true

This runs counter to Object.defineProperty's behavior:

Object.getOwnPropertyDescriptor(
  Object.defineProperty({}, 'a', { value: 'a' }),
  'a');
// { value: 'a',
//   writable: false,
//   enumerable: false,
//   configurable: false }

[jasongin]

@TimothyGu You're absolutely right. I was mistakenly assuming the V8 API default behavior followed the spec, but actually the writable/enumerable/configurable flags do default to false. In that case, it certainly makes sense to change the attribute enum value names to match the spec.

I'll prepare a PR with this change tomorrow.