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

[WIP] Rename napi #37108

Closed
wants to merge 8 commits into from
Closed

[WIP] Rename napi #37108

wants to merge 8 commits into from

Conversation

gabrielschulhof
Copy link
Contributor

No description provided.

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. labels Jan 28, 2021
@gabrielschulhof gabrielschulhof changed the title Rename napi [WIP] Rename napi Jan 28, 2021
@gabrielschulhof gabrielschulhof added the node-api Issues and PRs related to the Node-API. label Jan 28, 2021
@gabrielschulhof
Copy link
Contributor Author

Hmmm ... it's not running the github actions.

@richardlau
Copy link
Member

Hmmm ... it's not running the github actions.

https://www.githubstatus.com/incidents/tf9v5jjmq2lg

@jayaddison
Copy link
Contributor

Note for any reviewers: it can be worth using a more accepting --find-renames threshold when reviewing this changeset, so that some of the files that contain a small number of lines are detected correctly as renames rather than add/remove operations.

For example: git show 5039837c74d22d1197db33f8eeaaa9f84942c4c2 --stat --find-renames=10% (there are some small whitespace changes bundled in with the renames too -- @gabrielschulhof as a small recommendation, if possible it's helpful to perform bulk file renames and bulk file edits as separate commits; I'm not always great at following this advice myself though)

@gabrielschulhof
Copy link
Contributor Author

@jayaddison if only github would have the option. The only option they have so far is side-by-side diff and -w.

@gabrielschulhof
Copy link
Contributor Author

@jayaddison good idea though. Do all the renames in the first commit, and then make changes afterwards.

@gabrielschulhof
Copy link
Contributor Author

Copied from the PR I closed:

@nodejs/n-api we're gonna have to come up with a name to replace the macro NAPI_MODULE() and NAPI_MODULE_INIT(), because NODE_API_MODULE() is taken by node-addon-api. So is NODE_API_ADDON() IINM, with the addition of the addon class. I have provisionally replaced NAPI_MODULE() with NODE_API_MODULE() in this PR, but we'll have to pick something else.

mhdawson
mhdawson reviewed Jan 29, 2021
View reviewed changes
src/js_native_api_types.h
@@ -20,6 +20,13 @@ typedef struct napi_handle_scope__* napi_handle_scope;
typedef struct napi_escapable_handle_scope__* napi_escapable_handle_scope;
typedef struct napi_callback_info__* napi_callback_info;
typedef struct napi_deferred__* napi_deferred;
#define node_api_env napi_env
Copy link
Member

Choose a reason for hiding this comment

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

I think all of these defines need to be guarded by NODE-API version 8. Otherwise add-ons could fail to compile against versions of Node.js that support the NODE-API version they are targeting but don't have the new defines. I see adding these defines as the equivalent to adding new supported functions to NODE-API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mhdawson one concern is that NAPI_VERSION itself has to change so, going forward, people would be able to select API versions by defining NODE_API_VERSION themselves.

Trott
Trott reviewed Jan 31, 2021
View reviewed changes
.github/CODEOWNERS
@@ -100,12 +100,12 @@
/lib/internal/bootstrap/loaders.js @nodejs/modules
/src/module_wrap* @nodejs/modules @nodejs/vm

# N-API
# Node.js API
Copy link
Member

Choose a reason for hiding this comment

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

"Node.js API" seems like it will inevitably cause confusion. Is this the name that reached consensus in discussion? (I can't find the issue in the tracker. Maybe I'm looking in the wrong repository?)

Copy link
Contributor

Choose a reason for hiding this comment

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

@Trott I believe the name is still open for discussion at nodejs/abi-stable-node#420.

Copy link
Member

Choose a reason for hiding this comment

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

node-api was what was being discussion to replace n-api and napi.

@gabrielschulhof gabrielschulhof deleted the rename-napi branch February 3, 2021 07:17
@gabrielschulhof gabrielschulhof restored the rename-napi branch February 3, 2021 07:19
@gabrielschulhof
Copy link
Contributor Author

This PR is too big. I'll break it into several, starting with #37217.

@gabrielschulhof gabrielschulhof deleted the rename-napi branch March 6, 2021 05:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott left review comments

@mhdawson mhdawson mhdawson left review comments

@jschlight jschlight jschlight left review comments

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. 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.

None yet
7 participants
@gabrielschulhof @richardlau @jayaddison @Trott @mhdawson @jschlight @nodejs-github-bot