Preliminary refactoring from Babylon Native #569
Conversation
|
If you want N-API without buffers, N-API is probably not what you want. |
| @@ -2045,7 +2050,7 @@ inline Error Error::New(napi_env env, const std::string& message) { | |||
| } | |||
|
|
|||
| inline NAPI_NO_RETURN void Error::Fatal(const char* location, const char* message) { | |||
| napi_fatal_error(location, NAPI_AUTO_LENGTH, message, NAPI_AUTO_LENGTH); | |||
| napi_fatal_error(location, NAPI_AUTO_LENGTH, message, NAPI_AUTO_LENGTH); | |||
| void* Data(); ///< Gets a pointer to the data buffer. | ||
| size_t ByteLength(); ///< Gets the length of the array buffer in bytes. | ||
| void* Data() const; ///< Gets a pointer to the data buffer. | ||
| size_t ByteLength() const; ///< Gets the length of the array buffer in bytes. |
There was a problem hiding this comment.
These changes are unrelated, though relevant. Can you please submit them in a separate PR?
|
The same separation needs to be made for the tests as well. The non-Node.js-specific tests need to be able to run entirely on their own. The non-Node.js-specific native portions must not contain any addon initialization code, but they may include a common file which contains such code. Please have a look at https://github.com/nodejs/node/blob/master/test/js-native-api/entry_point.c and https://github.com/nodejs/node/blob/master/test/js-native-api/test_array/binding.gyp#L6 for an example of a common file and its use in an otherwise non-Node.js-specific test. |
|
Would it be possible and simpler to separate these declarations into two files like what's done in |
|
Discussed in the N-API team meeting together. There is consensus that is should be refactored into 2 files napi.h - the existing one that should include the new file
instead of using macros. |
|
@syntheticmagus is this something you'd still like to pursue? |
|
@mhdawson Sorry for the delayed reply. This is still something we want to pursue, and I think the edits suggested here will work great for us. However, to make those edits and update this PR, we'll need to upgrade all our integrations to merge with the latest changes from this repo. That's also something we want to do, but unfortunately it's not our top priority at the moment. So, in short, we're not working on this actively at the moment, but we do hope to be able to take action on it in the coming months. |
|
@syntheticmagus I think we should probably close this PR until we are ready to work on it again. What do you think? |
|
@bghgary Makes sense to me. |
Pursuant to issue #399, this is a minimal set of changes that allows Babylon Native to use
napi.handnapi_inl.hwithout modification. This may just be a first step as we're still trying to understand what the right integration strategy would be, and it's possible there might be a better way to achieve the result we're looking for (for instance, factoring out the node-specific APIs instead of guarding them inside a compiler define), but this PR may be a good place to start the relevant conversation.