Conversation
9672e06 to
0b20fc1
Compare
|
|
||
| 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.
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.
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.
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.
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.
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.
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.
legendecas
left a comment
There was a problem hiding this comment.
Thank you for setting this up! A few minor comments
NickNaso
left a comment
There was a problem hiding this comment.
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: