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: clarify use of NAPI_EXPERIMENTAL #35195

Closed
wants to merge 9 commits into from
Closed

doc: clarify use of NAPI_EXPERIMENTAL #35195

wants to merge 9 commits into from

Conversation

mhdawson
Copy link
Member

@mhdawson mhdawson commented Sep 14, 2020 •
edited
Loading

We've had a few questions about APIs not being available which
were related to not having specified NAPI_EXPERIMENTAL.

Add some additional documentation to explain this common
issue:

Refs: nodejs/node-addon-api#810

Signed-off-by: Michael Dawson mdawson@devrus.com

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@mhdawson
doc: clarify use of NAPI_EXPERIMENTAL
74c6564 
We've had a few questions about APIs not being available which
were related to not having specified `NAPI_EXPERIMENTAL`.

Add some additional documentation to explain this common
issue:

Refs: nodejs/node-addon-api#810

Signed-off-by: Michael Dawson <mdawson@devrus.com>
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/n-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 Sep 14, 2020
NickNaso
NickNaso approved these changes Sep 15, 2020
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.

LGTM

gabrielschulhof
gabrielschulhof reviewed Sep 17, 2020
View reviewed changes
Copy link
Contributor

@gabrielschulhof gabrielschulhof left a comment

Choose a reason for hiding this comment

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

Per our guidelines we should avoid using "you" in our documentation. I tried to re-word it more passively.

doc/api/n-api.md Outdated Show resolved Hide resolved
doc/api/n-api.md Outdated Show resolved Hide resolved
doc/api/n-api.md Outdated Show resolved Hide resolved
doc/api/n-api.md Outdated Show resolved Hide resolved
mhdawson and others added 4 commits September 17, 2020 14:37
@mhdawson @gabrielschulhof
Update doc/api/n-api.md
e1e0178 
Co-authored-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
@mhdawson @gabrielschulhof
Update doc/api/n-api.md
36d8e94 
Co-authored-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
@mhdawson @gabrielschulhof
Update doc/api/n-api.md
9a976c2 
Co-authored-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
@mhdawson @gabrielschulhof
Update doc/api/n-api.md
5c5e4f6 
Co-authored-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
mhdawson
mhdawson commented Sep 17, 2020
View reviewed changes
doc/api/n-api.md Outdated Show resolved Hide resolved
@mhdawson
Copy link
Member Author

@gabrielschulhof updated

gabrielschulhof
gabrielschulhof approved these changes Sep 17, 2020
View reviewed changes
Copy link
Contributor

@gabrielschulhof gabrielschulhof left a comment

Choose a reason for hiding this comment

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

I realized that as it is the doc never explains that N-API version: and added in: are headers accompanying each N-API. I suggested improved wording to explain that.

doc/api/n-api.md Outdated Show resolved Hide resolved
doc/api/n-api.md Outdated Show resolved Hide resolved
mhdawson and others added 3 commits September 18, 2020 16:08
@mhdawson @gabrielschulhof
Update doc/api/n-api.md
d8d8c90 
Co-authored-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
@mhdawson @gabrielschulhof
Update doc/api/n-api.md
4ed6bcc 
Co-authored-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
@mhdawson
fix linter issues
0162f80
@mhdawson
Copy link
Member Author

Linters look ok, doc only landing

mhdawson added a commit that referenced this pull request Sep 18, 2020
@mhdawson
doc: clarify use of NAPI_EXPERIMENTAL
f4586c9 
We've had a few questions about APIs not being available which
were related to not having specified `NAPI_EXPERIMENTAL`.

Add some additional documentation to explain this common
issue:

Refs: nodejs/node-addon-api#810

Signed-off-by: Michael Dawson <mdawson@devrus.com>

PR-URL: #35195
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@mhdawson
Copy link
Member Author

Landed in f4586c9

@mhdawson mhdawson closed this Sep 18, 2020
ruyadorno pushed a commit that referenced this pull request Sep 21, 2020
@mhdawson @ruyadorno
doc: clarify use of NAPI_EXPERIMENTAL
6119e95 
We've had a few questions about APIs not being available which
were related to not having specified `NAPI_EXPERIMENTAL`.

Add some additional documentation to explain this common
issue:

Refs: nodejs/node-addon-api#810

Signed-off-by: Michael Dawson <mdawson@devrus.com>

PR-URL: #35195
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Sep 21, 2020
Merged
4 tasks
addaleax pushed a commit that referenced this pull request Sep 22, 2020
@mhdawson @addaleax
doc: clarify use of NAPI_EXPERIMENTAL
6669eff 
We've had a few questions about APIs not being available which
were related to not having specified `NAPI_EXPERIMENTAL`.

Add some additional documentation to explain this common
issue:

Refs: nodejs/node-addon-api#810

Signed-off-by: Michael Dawson <mdawson@devrus.com>

PR-URL: #35195
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@codebytere codebytere mentioned this pull request Sep 28, 2020
Merged
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
@mhdawson @joesepi
doc: clarify use of NAPI_EXPERIMENTAL
d7b075e 
We've had a few questions about APIs not being available which
were related to not having specified `NAPI_EXPERIMENTAL`.

Add some additional documentation to explain this common
issue:

Refs: nodejs/node-addon-api#810

Signed-off-by: Michael Dawson <mdawson@devrus.com>

PR-URL: nodejs#35195
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@legendecas legendecas legendecas approved these changes

@gengjiawen gengjiawen gengjiawen approved these changes

@gabrielschulhof gabrielschulhof gabrielschulhof approved these changes

@NickNaso NickNaso NickNaso 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
@mhdawson @nodejs-github-bot @NickNaso @gabrielschulhof @gengjiawen @legendecas