Create JSI implementation for IJSValueReader and IJSValueWriter #5009
Conversation
ZihanChen-MSFT
added
the
AutoMerge
|
Hello @ZihanChen-MSFT! Because this pull request has the Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 10 hours. No worries though, I will be back when the time is right! 😉 p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
|
Could you add unit tests? |
Before getting ChakraRuntime run in the ComponentTests project, it is not possible to write unit test. I will investigate about this in the short future. |
| <DependentUpon>IReactNotificationService.idl</DependentUpon> | ||
| <SubType>Code</SubType> | ||
| </ClCompile> | ||
| <ClCompile Include="JsiReader.cpp"> |
There was a problem hiding this comment.
<ClC [](start = 4, length = 4)
It seems that the PR is missing changes to the MS.RN.vcsproj.filters file.
There was a problem hiding this comment.
Visual Studio doesn't change the filters file, the reason could be that, all added files have already been made depending on idl files in vcxproj, so there is nothing to be added to filters.
| } else if (value.isString() || value.isBool() || value.isNumber()) { | ||
| m_currentPrimitiveValue = {m_runtime, value}; | ||
| } else { | ||
| m_currentPrimitiveValue = facebook::jsi::Value::null(); |
There was a problem hiding this comment.
Should we assert that we have exhaused the options?
I.e. add cases for undefined and null. i.e. internally JSI has:
enum ValueKind {
UndefinedKind,
NullKind,
BooleanKind,
NumberKind,
SymbolKind,
StringKind,
ObjectKind,
PointerKind = SymbolKind,
};
There was a problem hiding this comment.
in DynamicReader, everything that is not recognizable becomes null. Here I use the same logic. For anything that is not an Object, it will be store in m_currentPrimitiveValue. In here everything other than Object/String/Boolean/Number, becomes null.
| if (obj.isArray(m_runtime)) { | ||
| m_containers.push_back(obj.getArray(m_runtime)); | ||
| } else { | ||
| m_containers.push_back({m_runtime, std::move(obj)}); |
There was a problem hiding this comment.
not sure about the design, but functions would fall into this case as well. Should we have a check for that?
i.e. use:
/// \return true iff the Object is callable. If so, then \c
/// getFunction will succeed.
bool isFunction(Runtime& runtime) const {
return runtime.isFunction(*this);
}
There was a problem hiding this comment.
In this case properties of Function will get enumerated. There are still other cases, you are not able to check all system classes. But this is fine, if users use our codegen, they won't be able to do this in TypeScript.
ghost
merged commit 
3 participants
JsiReaderandJsiWriterfor interfaceIJSValueReaderandIJSValueWriter.DynamicReaderandDynamicWriter, with a little change, because JSI doesn't supportint64_t, and doesn't differentiate integer and double.ReactHost,ABICxxModuleandABIViewManagerhave a turbo module version,JsiReaderandJsiWriterwill not be called in the project.Microsoft Reviewers: Open in CodeFlow