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.

Sign up for GitHub

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

Jump to bottom

doc: fix napi version for node_api_symbol_for #42878

Closed
wants to merge 2 commits into from
Closed

doc: fix napi version for node_api_symbol_for #42878

wants to merge 2 commits into from

Conversation

danielleadams
Copy link
Member

@danielleadams danielleadams commented Apr 27, 2022 •
edited
Loading

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
NickNaso
NickNaso reviewed Apr 27, 2022
View reviewed changes
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
Member Author

danielleadams commented Apr 27, 2022 •
edited
Loading

@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 •
edited
Loading

@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.

NickNaso
NickNaso suggested changes Apr 29, 2022
View reviewed changes
doc/api/n-api.md Outdated
napiVersion:
- v17.5.0
- v16.15.0
napiVersion: 8
Copy link
Member

@NickNaso NickNaso Apr 29, 2022 •
edited
Loading

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.

mhdawson
mhdawson reviewed May 27, 2022
View reviewed changes
doc/api/n-api.md Outdated Show resolved Hide resolved
mhdawson
mhdawson approved these changes May 27, 2022
View reviewed changes
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
@danielleadams @mhdawson
doc: fix napi version for node_api_symbol_for
0818b52 
PR-URL: #42878
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@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
@danielleadams @bengl
doc: fix napi version for node_api_symbol_for
776e746 
PR-URL: #42878
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@bengl bengl mentioned this pull request May 31, 2022
Merged
danielleadams added a commit that referenced this pull request Jun 27, 2022
@danielleadams
doc: fix napi version for node_api_symbol_for
65f20a0 
PR-URL: #42878
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
targos pushed a commit that referenced this pull request Jul 12, 2022
@danielleadams @targos
doc: fix napi version for node_api_symbol_for
2d2ab45 
PR-URL: #42878
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
@danielleadams @targos
doc: fix napi version for node_api_symbol_for
10e9868 
PR-URL: #42878
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@targos targos mentioned this pull request Aug 3, 2022
Merged
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@danielleadams @guangwong
doc: fix napi version for node_api_symbol_for
bf40f1a 
PR-URL: nodejs/node#42878
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NickNaso NickNaso NickNaso approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@mhdawson mhdawson mhdawson approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. node-api Issues and PRs related to the Node-API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@danielleadams @nodejs-github-bot @NickNaso @mhdawson @lpinca @cjihrig