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

doc: fix napi version for node_api_symbol_for #42878

Closed
wants to merge 2 commits into from

Conversation

danielleadams
Copy link
Contributor

@danielleadams danielleadams commented Apr 27, 2022

Fixes added napi version from Node version to napi version.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/node-api

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. node-api Issues and PRs related to the Node-API. labels Apr 27, 2022
Copy link
Member

@NickNaso NickNaso left a comment

Choose a reason for hiding this comment

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

node_api_symbol_for is an experimental feature so it's not part of any Node-API version. For example napi_object_freeze was added as expermental feature in Node.js v14.14.0 and v12.20.0 and promoted as stable api in Node-API v8.
napiVersion should represent the version of Node-API when a feature is promoted as stable.

@danielleadams
Copy link
Contributor Author

danielleadams commented Apr 27, 2022

@NickNaso thanks for the explanation. This looked like a typo when pulling into v16.x release and my quick skim through the docs (albeit, probably not thorough) did not find anything.

The version section in the docs looks related, could we clarify how napiVersion is incremented with experimental vs stable here? I can do that in this PR or close and open another one.

@NickNaso
Copy link
Member

NickNaso commented Apr 27, 2022

@NickNaso thanks for the explanation. This looked like a typo when pulling into v16.x release and my quick skim through the docs (albeit, probably not thorough) did not find anything.

The version section in the docs looks related, could we clarify how napiVersion is incremented with experimental vs stable here? I can do that in this PR or close and open another one.

@danielleadams what I know is that all the features under NAPI_EXPERIMENTAL flag should not be considered part of a version of Node-API (N-API)

#ifdef NAPI_EXPERIMENTAL
NAPI_EXTERN napi_status node_api_create_syntax_error(napi_env env,
                                                     napi_value code,
                                                     napi_value msg,
                                                     napi_value* result);
#endif  // NAPI_EXPERIMENTAL

The files interested are:

I did a look at these files and the following functions should be marked as experimental in the documentation:

The documentation for node_api_symbol_for needs to be updated like reported below:

#### `node_api_symbol_for`

<!-- YAML
added:
  - v17.5.0
  - v16.15.0
-->

> Stability: 1 - Experimental

The documentation for node_api_create_syntax_error needs to be updated like reported below:

#### `node_api_create_syntax_error`

<!-- YAML
added:
  - v17.2.0
  - v16.14.0
-->

> Stability: 1 - Experimental

The documentation for node_api_throw_syntax_error needs to be updated like reported below:

#### `node_api_throw_syntax_error`

<!-- YAML
added:
  - v17.2.0
  - v16.14.0
-->

> Stability: 1 - Experimental

The documentation for node_api_get_module_file_name seems to be right.

When these and / or other functions will be promoted as stable a new version of Node-API will be released and at that time the documentation will need to be updated adding the napiVersion.

Maybe it's more appropriate fix the documentation in this PR and then open a new PR to clarify how the Node-API version is incremented.

I will discuss about this at next Node-API team meeting this friday.

@NickNaso
Copy link
Member

Hi @danielleadams in the today Node-API Team meeting we discussed about this PR and please remove the napiVersion for the node_api_symbol_for . I will provide to fix all other problems I found and to enrich the documentation explain better what is a Node-API experimental feature.

doc/api/n-api.md Outdated
napiVersion:
- v17.5.0
- v16.15.0
napiVersion: 8
Copy link
Member

@NickNaso NickNaso Apr 29, 2022

Choose a reason for hiding this comment

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

Please remove napiVersion this function is an experimental function.

doc/api/n-api.md Outdated Show resolved Hide resolved
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson
Copy link
Member

Doc only and linters are passing so landing.

mhdawson pushed a commit that referenced this pull request May 27, 2022
PR-URL: #42878
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@mhdawson
Copy link
Member

Landed in 0818b52

@mhdawson mhdawson closed this May 27, 2022
bengl pushed a commit that referenced this pull request May 30, 2022
PR-URL: #42878
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@bengl bengl mentioned this pull request May 31, 2022
danielleadams added a commit that referenced this pull request Jun 27, 2022
PR-URL: #42878
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request Jul 12, 2022
PR-URL: #42878
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
PR-URL: #42878
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
PR-URL: nodejs/node#42878
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants