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: add napi_get_all_property_names #30006

Closed
wants to merge 4 commits into from
Closed

Conversation

himself65
Copy link
Member

@himself65 himself65 commented Oct 17, 2019

follow up by #28944

support to get all property of an object.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 17, 2019
@himself65 himself65 marked this pull request as ready for review October 17, 2019 14:58
src/js_native_api.h Outdated Show resolved Hide resolved
doc/api/n-api.md Outdated Show resolved Hide resolved
src/js_native_api_types.h Show resolved Hide resolved
src/js_native_api_types.h Outdated Show resolved Hide resolved
doc/api/n-api.md Outdated Show resolved Hide resolved
doc/api/n-api.md Outdated Show resolved Hide resolved
src/js_native_api_v8.cc Outdated Show resolved Hide resolved
@gabrielschulhof gabrielschulhof added the node-api Issues and PRs related to the Node-API. label Oct 24, 2019
doc/api/n-api.md Outdated Show resolved Hide resolved
doc/api/n-api.md Outdated Show resolved Hide resolved
doc/api/n-api.md Outdated Show resolved Hide resolved
doc/api/n-api.md Outdated Show resolved Hide resolved
doc/api/n-api.md Outdated Show resolved Hide resolved
doc/api/n-api.md Outdated Show resolved Hide resolved
doc/api/n-api.md Outdated Show resolved Hide resolved
doc/api/n-api.md Outdated Show resolved Hide resolved
doc/api/n-api.md Outdated

```C
typedef enum {
napi_key_include_all_properties,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would there be any possible ambiguity on these similar naming of napi_key_include_all_properties and napi_key_all_properties? IIUC the napi_key_collection_mode indicates whether or not the keys on the prototype chains are collected, and napi_key_filter filters property descriptors.

Would it be possible to name napi_key_collection_mode following v8's pattern? i.e. napi_key_include_prototypes, which would sound very precise.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed them to napi_key_include_prototypes and napi_key_include_own_properties to follow v8's pattern, I think

doc/api/n-api.md Outdated
```C
typedef enum {
napi_key_include_all_properties,
napi_key_include_own_properties

This comment was marked as outdated.

Copy link
Contributor

@gabrielschulhof gabrielschulhof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will be LGTM once the enums are documented.

src/js_native_api_types.h Show resolved Hide resolved
BethGriggs added a commit that referenced this pull request Apr 7, 2020
macOS package notarization and a change in builder configuration

The macOS binaries for this release, and future 10.x releases, are now
being compiled on macOS 10.15 (Catalina) with Xcode 11 to support
package notarization, a requirement for installing .pkg files on macOS
10.15 and later. Previous builds of Node.js 10.x were compiled on macOS
10.10 (Yosemite) with a minimum deployment target of macOS 10.7 (Lion).
As binaries are still being compiled to support a minimum of macOS 10.7
(Lion) we do not anticipate this having a negative impact on Node.js
10.x users with older versions of macOS.

Notable changes:

- buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc)
  [#19691](#19691)
- build: macOS package notarization (Rod Vagg)
  [#31459](#31459)
- deps:
  - update npm to 6.14.3 (Myles Borins)
    [#32368](#32368)
  - upgrade openssl sources to 1.1.1e (Hassaan Pasha)
    [#32328](#32328)
  - upgrade to libuv 1.34.2 (cjihrig)
    [#31477](#31477)
- n-api:
  - add napi\_get\_all\_property\_names (himself65)
    [#30006](#30006)
  - add APIs for per-instance state management (Gabriel Schulhof)
    [#28682](#28682)
  - define release 6
    [#32058](#32058)
  - turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen)
    [#26128](#26128)
- tls:
  - expose keylog event on TLSSocket (Alba Mendez)
    [#27654](#27654)
  - support TLS min/max protocol defaults in CLI (Sam Roberts)
    [#27946](#27946)
- url: handle quasi-WHATWG URLs in urlToOptions() (cjihrig)
  [#26226](#26226)

PR-URL: #31984
BethGriggs added a commit that referenced this pull request Apr 8, 2020
macOS package notarization and a change in builder configuration

The macOS binaries for this release, and future 10.x releases, are now
being compiled on macOS 10.15 (Catalina) with Xcode 11 to support
package notarization, a requirement for installing .pkg files on macOS
10.15 and later. Previous builds of Node.js 10.x were compiled on macOS
10.10 (Yosemite) with a minimum deployment target of macOS 10.7 (Lion).
As binaries are still being compiled to support a minimum of macOS 10.7
(Lion) we do not anticipate this having a negative impact on Node.js
10.x users with older versions of macOS.

Notable changes:

- buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc)
  [#19691](#19691)
- build: macOS package notarization (Rod Vagg)
  [#31459](#31459)
- deps:
  - update npm to 6.14.3 (Myles Borins)
    [#32368](#32368)
  - upgrade openssl sources to 1.1.1e (Hassaan Pasha)
    [#32328](#32328)
  - upgrade to libuv 1.34.2 (cjihrig)
    [#31477](#31477)
- n-api:
  - add napi\_get\_all\_property\_names (himself65)
    [#30006](#30006)
  - add APIs for per-instance state management (Gabriel Schulhof)
    [#28682](#28682)
  - define release 6
    [#32058](#32058)
  - turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen)
    [#26128](#26128)
- tls:
  - expose keylog event on TLSSocket (Alba Mendez)
    [#27654](#27654)
  - support TLS min/max protocol defaults in CLI (Sam Roberts)
    [#27946](#27946)
- url: handle quasi-WHATWG URLs in urlToOptions() (cjihrig)
  [#26226](#26226)

PR-URL: #31984
BethGriggs added a commit that referenced this pull request Apr 8, 2020
macOS package notarization and a change in builder configuration

The macOS binaries for this release, and future 10.x releases, are now
being compiled on macOS 10.15 (Catalina) with Xcode 11 to support
package notarization, a requirement for installing .pkg files on macOS
10.15 and later. Previous builds of Node.js 10.x were compiled on macOS
10.10 (Yosemite) with a minimum deployment target of macOS 10.7 (Lion).
As binaries are still being compiled to support a minimum of macOS 10.7
(Lion) we do not anticipate this having a negative impact on Node.js
10.x users with older versions of macOS.

Notable changes:

- buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc)
  [#19691](#19691)
- build: macOS package notarization (Rod Vagg)
  [#31459](#31459)
- deps:
  - update npm to 6.14.3 (Myles Borins)
    [#32368](#32368)
  - upgrade openssl sources to 1.1.1e (Hassaan Pasha)
    [#32328](#32328)
  - upgrade to libuv 1.34.2 (cjihrig)
    [#31477](#31477)
- n-api:
  - add napi\_get\_all\_property\_names (himself65)
    [#30006](#30006)
  - add APIs for per-instance state management (Gabriel Schulhof)
    [#28682](#28682)
  - define release 6
    [#32058](#32058)
  - turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen)
    [#26128](#26128)
- tls:
  - expose keylog event on TLSSocket (Alba Mendez)
    [#27654](#27654)
  - support TLS min/max protocol defaults in CLI (Sam Roberts)
    [#27946](#27946)
- url: handle quasi-WHATWG URLs in urlToOptions() (cjihrig)
  [#26226](#26226)

PR-URL: #31984
BethGriggs added a commit that referenced this pull request Apr 14, 2020
macOS package notarization and a change in builder configuration

The macOS binaries for this release, and future 10.x releases, are now
being compiled on macOS 10.15 (Catalina) with Xcode 11 to support
package notarization, a requirement for installing .pkg files on macOS
10.15 and later. Previous builds of Node.js 10.x were compiled on macOS
10.10 (Yosemite) with a minimum deployment target of macOS 10.7 (Lion).
As binaries are still being compiled to support a minimum of macOS 10.7
(Lion) we do not anticipate this having a negative impact on Node.js
10.x users with older versions of macOS.

Notable changes:

- buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc)
  [#19691](#19691)
- build: macOS package notarization (Rod Vagg)
  [#31459](#31459)
- deps:
  - update npm to 6.14.3 (Myles Borins)
    [#32368](#32368)
  - upgrade openssl sources to 1.1.1e (Hassaan Pasha)
    [#32328](#32328)
  - upgrade to libuv 1.34.2 (cjihrig)
    [#31477](#31477)
- n-api:
  - add napi\_get\_all\_property\_names (himself65)
    [#30006](#30006)
  - add APIs for per-instance state management (Gabriel Schulhof)
    [#28682](#28682)
  - define release 6
    [#32058](#32058)
  - turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen)
    [#26128](#26128)
- tls:
  - expose keylog event on TLSSocket (Alba Mendez)
    [#27654](#27654)
  - support TLS min/max protocol defaults in CLI (Sam Roberts)
    [#27946](#27946)
- url: handle quasi-WHATWG URLs in urlToOptions() (cjihrig)
  [#26226](#26226)

PR-URL: #31984
@targos targos removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lts-watch-v12.x labels Apr 25, 2020
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
Co-Authored-By: Gabriel Schulhof <[email protected]>

Backport-PR-URL: nodejs#32482
PR-URL: nodejs#30006
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2020
Co-Authored-By: Gabriel Schulhof <[email protected]>

Backport-PR-URL: #32482
PR-URL: #30006
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
tniessen added a commit to tniessen/node that referenced this pull request Jul 21, 2023
Even though the redefinition complies with the C standard because the
second definition is "effectively the same" as the first definition,
it's best to avoid any redefinition.

Refs: nodejs#28237
Refs: nodejs#30006
nodejs-github-bot pushed a commit that referenced this pull request Jul 23, 2023
Even though the redefinition complies with the C standard because the
second definition is "effectively the same" as the first definition,
it's best to avoid any redefinition.

Refs: #28237
Refs: #30006
PR-URL: #48879
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vladimir Morozov <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Jul 27, 2023
Even though the redefinition complies with the C standard because the
second definition is "effectively the same" as the first definition,
it's best to avoid any redefinition.

Refs: nodejs#28237
Refs: nodejs#30006
PR-URL: nodejs#48879
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vladimir Morozov <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
pluris pushed a commit to pluris/node that referenced this pull request Aug 6, 2023
Even though the redefinition complies with the C standard because the
second definition is "effectively the same" as the first definition,
it's best to avoid any redefinition.

Refs: nodejs#28237
Refs: nodejs#30006
PR-URL: nodejs#48879
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vladimir Morozov <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
pluris pushed a commit to pluris/node that referenced this pull request Aug 7, 2023
Even though the redefinition complies with the C standard because the
second definition is "effectively the same" as the first definition,
it's best to avoid any redefinition.

Refs: nodejs#28237
Refs: nodejs#30006
PR-URL: nodejs#48879
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vladimir Morozov <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
Even though the redefinition complies with the C standard because the
second definition is "effectively the same" as the first definition,
it's best to avoid any redefinition.

Refs: nodejs#28237
Refs: nodejs#30006
PR-URL: nodejs#48879
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vladimir Morozov <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
Even though the redefinition complies with the C standard because the
second definition is "effectively the same" as the first definition,
it's best to avoid any redefinition.

Refs: nodejs#28237
Refs: nodejs#30006
PR-URL: nodejs#48879
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vladimir Morozov <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
UlisesGascon pushed a commit to UlisesGascon/node that referenced this pull request Aug 14, 2023
Even though the redefinition complies with the C standard because the
second definition is "effectively the same" as the first definition,
it's best to avoid any redefinition.

Refs: nodejs#28237
Refs: nodejs#30006
PR-URL: nodejs#48879
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vladimir Morozov <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
Even though the redefinition complies with the C standard because the
second definition is "effectively the same" as the first definition,
it's best to avoid any redefinition.

Refs: #28237
Refs: #30006
PR-URL: #48879
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vladimir Morozov <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this pull request Nov 27, 2023
Even though the redefinition complies with the C standard because the
second definition is "effectively the same" as the first definition,
it's best to avoid any redefinition.

Refs: #28237
Refs: #30006
PR-URL: #48879
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vladimir Morozov <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
Even though the redefinition complies with the C standard because the
second definition is "effectively the same" as the first definition,
it's best to avoid any redefinition.

Refs: nodejs/node#28237
Refs: nodejs/node#30006
PR-URL: nodejs/node#48879
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vladimir Morozov <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
Even though the redefinition complies with the C standard because the
second definition is "effectively the same" as the first definition,
it's best to avoid any redefinition.

Refs: nodejs/node#28237
Refs: nodejs/node#30006
PR-URL: nodejs/node#48879
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vladimir Morozov <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants