new serialization for js <-> cpp interaction#2400
new serialization for js <-> cpp interaction#2400aw-c wants to merge 1 commit intoskyrim-multiplayer:mainfrom
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 9323adc in 2 minutes and 10 seconds
More details
- Looked at
227lines of code in4files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. serialization/include/archives/NapiOutputArchive.h:32
- Draft comment:
Avoid using 'static_assert(!sizeof(T), "not string")'. This always fails when instantiated. Consider using a dependent false idiom (e.g., 'static_assert(always_false::value, "not a string type")') to disable this overload via SFINAE. - Reason this comment was not posted:
Marked as duplicate.
2. skymp5-server/cpp/server_guest_lib/Inventory.cpp:200
- Draft comment:
Most vexing parse: 'NapiOutputArchive ar();' is interpreted as a function declaration rather than an object. Use 'NapiOutputArchive ar(env);' (or brace initialization) to properly instantiate the archive with the environment. - Reason this comment was not posted:
Marked as duplicate.
3. skymp5-server/cpp/server_guest_lib/Inventory.cpp:201
- Draft comment:
Usage of 'const_cast' to call Serialize() suggests that Serialize might modify the object. Consider revisiting the design to either allow Serialize() to be const if it only affects serialization state, or avoid const_cast to maintain const-correctness. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
4. skymp5-server/cpp/server_guest_lib/Inventory.cpp:200
- Draft comment:
Typographical issue: the declaration 'NapiOutputArchive ar();' in Inventory::ToNapiObject() is interpreted by the C++ compiler as a function declaration (due to the most vexing parse) instead of an object instantiation. It should be changed to 'NapiOutputArchive ar;' to correctly create an instance of NapiOutputArchive. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_TuXrSKOOzjgJHvG4
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| Napi::Value Inventory::ToNapiObject() const | ||
| { | ||
| auto env = jsEngine; | ||
| NapiOutputArchive ar(); |
There was a problem hiding this comment.
Avoid most vexing parse: 'NapiOutputArchive ar();' declares a function instead of instantiating an object. Use 'NapiOutputArchive ar(env);' instead.
| NapiOutputArchive ar(); | |
| NapiOutputArchive ar(env); |
| template <StringLike T> | ||
| NapiOutputArchive& Serialize(const T& input) | ||
| { | ||
| static_assert(!sizeof(T), "not string"); |
There was a problem hiding this comment.
The static_assert in the StringLike overload always fails because sizeof(T) is never 0. Consider using a dependent false typetrick instead.
| static_assert(!sizeof(T), "not string"); | |
| static_assert(std::false_type::value, "not string"); |
|
Ой, там да, есть прикол с jsEngine. По другому env должен передаваться |
| NapiOutputArchive& Serialize(const char* key, const std::optional<T>& input) | ||
| { | ||
| if (input.has_value()) | ||
| { |
There was a problem hiding this comment.
Please format with clang-format
- 2 spaces for indents
{on the same line for if-s and for-s
| return *this; | ||
| } | ||
|
|
||
| Napi::Value extract_output() |
There was a problem hiding this comment.
Please keep in line with the current conventions https://github.com/skyrim-multiplayer/skymp/blob/main/docs/docs_cpp_code_guidelines.md
| Napi::Value extract_output() | |
| Napi::Value ExtractOutput() |
|
|
||
| Napi::Env m_env; | ||
| std::optional<Napi::Value> m_outputOther; | ||
| std::optional<Napi::Object> m_outputObject; |
There was a problem hiding this comment.
Currently, there is no rule to prefix member names m_
| return std::move(*m_outputObject); | ||
| } | ||
|
|
||
| throw std::runtime_error("Uninitialized field!"); |
There was a problem hiding this comment.
We'd also have to change this to more meaningful messages before merge. Feel free to keep it as it is while it's wip though
3 participants
Important
Add
NapiOutputArchivefor C++ to JavaScript serialization and updateInventoryclass and CMake configuration accordingly.NapiOutputArchiveclass inNapiOutputArchive.hfor serializing C++ objects to JavaScript objects using Node-API.ToNapiObject()method inInventory.cppto serializeInventoryobjects to JavaScript objects usingNapiOutputArchive.NapiOutputArchive.hand<napi.h>inInventory.cppandInventory.h.CMakeLists.txtto find and linkunofficial-node-addon-api.This description was created by
for 9323adc. It will automatically update as commits are pushed.