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 method to get own properties #13925

Closed
mhdawson opened this issue Jun 26, 2017 · 4 comments
Closed

N-API: Add method to get own properties #13925

mhdawson opened this issue Jun 26, 2017 · 4 comments
Assignees
Labels
node-api Issues and PRs related to the Node-API.

Comments

@mhdawson
Copy link
Member

mhdawson commented Jun 26, 2017

  • Version: ALL
  • Platform: ALL
  • Subsystem: ALL

We need a new function to allow add-ons to get own properties consistently across versions:

```C++
napi_status napi_get_own_property(napi_env e,
                                  napi_value o,
                                  napi_propertyname p,
                                  napi_value* result);

which never walks up the prototype chain.

We also need to validate/ensure that napi_get_property() always walks up the prototype chain.

The N-API team will get to this based on priorities, but this is a good place for others to contribute as well. If you start working on this, assign the issue to yourself and add a comment that you are working on it.

@cjihrig
Copy link
Contributor

cjihrig commented Jun 27, 2017

Working on this.

@cjihrig
Copy link
Contributor

cjihrig commented Jun 28, 2017

@mhdawson I added a test for napi_get_property() walking the prototype chain in #13961. I'm questioning if we really need a napi_get_own_property() though. I understand the use case, but wouldn't it be better to have napi_has_own_property(), which would correspond to Object.prototype.hasOwnProperty()?

@cjihrig
Copy link
Contributor

cjihrig commented Jun 30, 2017

@mhdawson ping ^

cjihrig added a commit to cjihrig/node that referenced this issue Jun 30, 2017
Refs: nodejs#13925
PR-URL: nodejs#13961
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
addaleax pushed a commit to addaleax/node that referenced this issue Jul 3, 2017
Refs: nodejs#13925
PR-URL: nodejs#13961
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
cjihrig added a commit to cjihrig/node that referenced this issue Jul 6, 2017
Refs: nodejs#13925
PR-URL: nodejs#14063
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@cjihrig
Copy link
Contributor

cjihrig commented Jul 6, 2017

Closing this out as the test has been added and napi_has_own_property() should be enough for users to build a get_own_property() as needed.

@cjihrig cjihrig closed this as completed Jul 6, 2017
addaleax pushed a commit that referenced this issue Jul 11, 2017
Refs: #13925
PR-URL: #13961
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
addaleax pushed a commit that referenced this issue Jul 11, 2017
Refs: #13925
PR-URL: #14063
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
addaleax pushed a commit that referenced this issue Jul 18, 2017
Refs: #13925
PR-URL: #13961
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
addaleax pushed a commit that referenced this issue Jul 18, 2017
Refs: #13925
PR-URL: #14063
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Fishrock123 pushed a commit that referenced this issue Jul 19, 2017
Refs: #13925
PR-URL: #14063
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this issue Apr 10, 2018
Refs: nodejs#13925
PR-URL: nodejs#13961
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this issue Apr 10, 2018
Refs: nodejs#13925
PR-URL: nodejs#14063
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
MylesBorins pushed a commit that referenced this issue Apr 16, 2018
Refs: #13925
Backport-PR-URL: #19447
PR-URL: #13961
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this issue Apr 16, 2018
Refs: #13925
Backport-PR-URL: #19447
PR-URL: #14063
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

No branches or pull requests

2 participants