n-api: add napi_get_own_property_names

[himself65]

fix #28942, and this pr should after #30006

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[addaleax]

@nodejs/n-api

I feel like this approach kind of leads to questions like “what if we also want symbols/inherited properties/no indices/…”, and we might want to come up with something that has flexibility which is similar to the V8 API?

[mhdawson]

+1 to making it more flexible as long as the options supported are part of the standard and will be supported by all engines.

[Trott]

@addaleax @mhdawson @nodejs/n-api Are we likely to land this? If not, I'd like to close it. If we're likely to land it, I'd like to nudge it forward. /ping @himself65! Or maybe reviewers can perhaps push fixup changes to the branch to get it where it needs to go and expedite things a bit?

[addaleax]

@Trott Yes, but @mhdawson’s comment need to be addressed and we do need somebody (e.g. @himself65) to figure out in what way we expose the other option flags, if we do so.

[himself65]

I will push fixed changes later :)

[gabrielschulhof]

The PR is good, but we really need to return napi_pending_exception whenever there is the possibility that an exception has occurred on the JS side.

[nodejs-github-bot]

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

[Trott]

@mhdawson @addaleax LGTY now?

[addaleax]

So, code-wise this LGTM, but I still feel like the API should expose flags similar to the ones we get from V8, otherwise this just begs for Yet Another Function That Does The Same Thing.

[gabrielschulhof]

@himself65 can you please check whether other engines, such as JerryScript, have the facilities to retrieve property names as selectively as V8? That is, could the kind of flexibility that V8 provides (skipping/not skipping symbols, rendering as string/leaving as number) be implemented

• on top of other engines (preferably), or
• into other engines,

if needed?

[addaleax]

@gabrielschulhof From a look at JerryScript’s API docs, the API only exposes a way to get enumerable properties. But I wouldn’t care too much for that here – any spec-compatible JS implementation needs to have a way to provide all properties, because it could always be implemented in terms of JS builtin functions.

[devsnek]

the js spec has operations that ask for a filtered subset of own properties, so I think it's reasonable that we can provide something similar here. Even if a napi wrapper has to do the work instead of the engine.

[gabrielschulhof]

@devsnek @addaleax sounds like we can safely expose the full flexibility provided by V8 because on other engines this same flexibility can be implemented by either filtering after then engine's native API returns, or calling a spec-compliant JS API if the native API is incapable of returning unfiltered results.

Similarly, I guess we can use type coercion in post-processing to address the flexibility in the second parameter.

@himself65 would you mind exposing the two parameters provided by V8?

I suppose we'll have to add a napi_get_all_property_names()(name eminently negotiable 😀) to correspond to napi_get_property_names().

Honestly, I'm not sure whether we shouldn't just go with

typedef enum {
  napi_key_own,
  napi_key_all
} napi_key_collection_mode;

typedef enum {
  napi_key_strings = 1,
  napi_key_symbols = 1 << 1
} napi_key_types;

typedef enum {
  napi_keep_numbers,
  napi_numbers_to_strings
} napi_key_coercion;

napi_status napi_get_all_property_names(napi_env env,
                                        napi_value object,
                                        napi_key_collection_mode key_mode,
                                        napi_key_types key_types,
                                        napi_key_coercion key_coercion,
                                        napi_value* result);

and basically expose v8::Object::GetPropertyNames() pretty much as-is, given that the resulting N-API can be implemented on any spec-compliant engine, if only by calling into JS.

We can then re-implement napi_get_property_names() as a call to this new function.

[legendecas]

Would filter ONLY_ENUMERABLE be reasonable to be supported in this API? Since napi_get_property_names() only expects enumerable property names, if napi_get_property_names() would be re-implemented as a call to this new function, it has to be supported too.

[gabrielschulhof]

@legendecas right, we need one more enum for one more flag to the method.

[himself65]

I will open a new pull request about that later 🤔

[fhinkel]

ping @himself65

[himself65]

Would filter ONLY_ENUMERABLE be reasonable to be supported in this API? Since napi_get_property_names() only expects enumerable property names, if napi_get_property_names() would be re-implemented as a call to this new function, it has to be supported too.

How to implement that? 🤔

[legendecas]

napi_key_enumerable in #30006 could be used to get enumerable keys on the object. BTW, should this PR be succeeded by #30006? Since #30006 covers every part of this API.

[himself65]

@legendecas Fixed and this pr should after #30006 for now

[gabrielschulhof]

IINM this PR is superseded by #30006, so I will close it. Please re-open if I am mistaken.