-
-
Notifications
You must be signed in to change notification settings - Fork 11
Documentation updates #10
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
Conversation
9672e06 to
0b20fc1
Compare
README.md
Outdated
|
|
||
| The tests are divided into two buckets, based on the two header files declaring the Node-API functions: | ||
|
|
||
| - `tests/engine/*` testing Node-API defined in the [`js_native_api.h`](https://github.com/nodejs/node-api-headers/blob/main/include/js_native_api.h) header (located in [`./test/js-native-api` of the Node.js codebase](https://github.com/nodejs/node/tree/main/test/js-native-api)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we name the folders following the corresponding header file to be tested? In this case, we don't have to remember new names.
| - `tests/engine/*` testing Node-API defined in the [`js_native_api.h`](https://github.com/nodejs/node-api-headers/blob/main/include/js_native_api.h) header (located in [`./test/js-native-api` of the Node.js codebase](https://github.com/nodejs/node/tree/main/test/js-native-api)). | |
| - `tests/js_native_api/*` testing Node-API defined in the [`js_native_api.h`](https://github.com/nodejs/node-api-headers/blob/main/include/js_native_api.h) header (located in [`./test/js-native-api` of the Node.js codebase](https://github.com/nodejs/node/tree/main/test/js-native-api)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could - at the same time, I've always found these names confusing 🙈 Then again perhaps the "engine" vs "runtime" distinction is weird as well? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally found the terms like engine and runtime are abused in a lot of contexts and can be ambiguious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree "engine" and "runtime" is being used (and abused) interchangeably in the community, I don't however know if that means it will be bad to use here as well?
I'd refer to a definition introduced by @tmikov on the PR adding Node-API to Hermes:
The purpose of this PR is not to create an emulation of NodeJS. Instead the goal is to implement the engine-specific parts of the API.
Hermes is only the JS engine, functionally equivalent to v8 or JavaScriptCore, for example. It is not a complete runtime environment like NodeJS, Deno, Bun, etc. The JS engine is just one part of a runtime environment.
The PR aims to expose the JS engine-specific parts of Node-API. It is not meant to emulate NodeJS for the purpose of running existing native extensions.
I've found it beneficial to keep on referring to these as engine-specific and runtime-specific throughout the implementation of Node-API in Hermes (providing the engine-specific functions) / React Native (providing the runtime-specific functions on-top).
I believe the existing names ("js_native_api" and "node_api") are mainly due to historic reasons that we don't have a strong need to inherit in this repository. "node_api" is particularly confusing: Aren't both a part what we consider "Node-API"?
Again - I'm not feeling too strongly for this (perhaps a +1 on a -3/+3 scale), but I wanted to signal the reasoning behind my proposal to rename in the context of the CTS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the naming history of node-api, I'd prefer restricting on introducing new terms in this test suite, as it would increase the curve for people to get onboarded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. I'll update this to use the existing names for these directories. I'd like to add a clarification, on these lines instead, using the terms "engine" and "runtime", as I do think it's valuable for implementors to start referencing these parts by a more suitable name long term.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed a fix 👍
legendecas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for setting this up! A few minor comments
NickNaso
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add CODE_OF_CONDUCT.md as we did here: https://github.com/nodejs/node-addon-api/blob/main/CODE_OF_CONDUCT.md
|
@NickNaso it's already in the repo 🙂 |
Co-authored-by: Chengzhong Wu <legendecas@gmail.com>
|
Thank you! |
As part of #15, merging this PR will: