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 more property defaults #35214

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
src/js_native_api_types.h Outdated Show resolved Hide resolved
@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
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.

None yet

6 participants