refactor(Worklets): move serializable factories #8560
Conversation
| jsi::Value makeSerializableString(jsi::Runtime &rt, const jsi::String &string) { | ||
| const auto serializable = std::make_shared<SerializableString>(string.utf8(rt)); | ||
| return SerializableJSRef::newNativeStateObject(rt, serializable); | ||
| } |
There was a problem hiding this comment.
How about converting these function into several makeSerializable static methods with different argument types?
There was a problem hiding this comment.
This is my long-term plan but for the atomic PRs and ease of review I decided to move it to a separate file for now.
| : BaseClass(rt, std::forward<Args>(args)...), primaryRuntime_(&rt) {} | ||
|
|
||
| jsi::Value toJSValue(jsi::Runtime &rt); | ||
| jsi::Value toJSValue(jsi::Runtime &rt) { |
There was a problem hiding this comment.
What's the reason for having this function inlined? It makes debugging more challenging and historically we had a serious bug right in this place.
There was a problem hiding this comment.
It's a method of a generic class so it must be available for the compilation units which need it. Previously the only compilation units that needed the definition were in Serializable.cpp so they had access to the template. Now that the functions were moved to SerializableFactory.h/cpp they need to have access to the template through the header file.
Summary
Granular PR for the ease of review of refactor of some of Serializables native code, extracted from #8555.
Test plan
CI