Skip to content

n-api: add more property defaults #35214

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Sep 15, 2020

Add a default value for class method and js like property in enum napi_property_attributes.

n-api currently offers only one default which is non configurable, non writable, non enumerable - like Object.defineProperty(). While this is formal correct the usual way to create properties in JS is either by defining a class or use obj.prop = value.

The defaults from these variants are now backed into enum values.

Refs: nodejs/node-addon-api#811

Add a default value for class method and js like property in enum
napi_property_attributes.

n-api currently offers only one default which is non configurable,
non writable, non enumerable - like Object.defineProperty(). While
this is formal correct the usual way to create properties in JS is
either by defining a class or use obj.prop = value.

The defaults from these variants are now backed into enum values.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/n-api

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 15, 2020
@Flarna Flarna added the node-api Issues and PRs related to the Node-API. label Sep 15, 2020
@Flarna
Copy link
Member Author

Flarna commented Sep 15, 2020

Do we need an #if NAPI_VERSION around this? or is this a normal semver-minor addition?

@Flarna Flarna added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 15, 2020
@gabrielschulhof
Copy link
Contributor

@Flarna yeah, the new enum values need to be inside a

#ifdef NAPI_EXPERIMENTAL
...
#endif  // NAPI_EXPERIMENTAL

block.

@Flarna
Copy link
Member Author

Flarna commented Sep 16, 2020

@gabrielschulhof ok, will add the flag. I found also the napi guideline. It tells A new API must be considered experimental for at least one minor version release of Node.js before it can be considered for promotion out of experimental. .
I wonder if this strict rule is applicable to such simple enhancements like this enum extensions.
If not I would love to get this into n-api 7 (refs: #35199).

Besides that I will split the PR to move the changes in the doc samples into a separate PR to avoid that they use an experimental part.

@Flarna Flarna added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 16, 2020
@gabrielschulhof
Copy link
Contributor

@Flarna in general we would like to mark things as stable when they are available in all LTS versions. I know this is a small change, but it would be best if we backported it first. We can delay landing #35199 until after our N-API meeting, wherein we can discuss whether we should first backport this and then release N-API 7 with these changes. This can land as experimental until then. @nodejs/n-api WDYT?

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 16, 2020
@nodejs-github-bot

This comment has been minimized.

@Flarna
Copy link
Member Author

Flarna commented Sep 16, 2020

@gabrielschulhof Sounds good.

I expect that n-api v7 should be part of node.js 15. Not sure if we end up in hitting some deadline here. I don't want to block your changes for n-api 7 so feel free to go on with your v7 changes.

Co-authored-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
@Flarna Flarna added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 16, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 16, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@legendecas
Copy link
Member

@Flarna I expect that n-api v7 should be part of node.js 15

Bumping n-api version is a semver-minor change. If the consensus that this should be included in n-api v7 has been reached, I'd believe it is ok to exceed the deadline of v15.0.0 and land in a later minor version.

Flarna added a commit that referenced this pull request Sep 17, 2020
Add a default value for class method and js like property in enum
napi_property_attributes.

n-api currently offers only one default which is non configurable,
non writable, non enumerable - like Object.defineProperty(). While
this is formal correct the usual way to create properties in JS is
either by defining a class or use obj.prop = value.

The defaults from these variants are now backed into enum values.

PR-URL: #35214
Refs: nodejs/node-addon-api#811
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
@Flarna
Copy link
Member Author

Flarna commented Sep 17, 2020

Landed in c9506a8

@Flarna Flarna closed this Sep 17, 2020
# for free to join this conversation on GitHub. Already have an account? # 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.

6 participants