New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RuntimeDecorator
cleanup
#4027
Merged
Merged
RuntimeDecorator
cleanup
#4027
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jwajgelt
force-pushed
the
@jwajgelt/runtime_decorator_cleanup
branch
from
February 3, 2023 15:33
2af191a
to
9556360
Compare
graszka22
reviewed
Feb 9, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice templates 👍 I have just two questions
graszka22
approved these changes
Feb 10, 2023
Merged
tomekzaw
added a commit
that referenced
this pull request
Feb 14, 2023
## Summary This PR fixes formatting after changes in #4027. ## Test plan Check if CI is green.
fluiddot
pushed a commit
to wordpress-mobile/react-native-reanimated
that referenced
this pull request
Jun 5, 2023
<!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please follow the template so that the reviewers can easily understand what the code changes affect. --> ## Summary Reduces boilerplate around installing native JSI functions in `RuntimeDecorator`. We need to install Host Functions into the JS runtimes that will be called by reanimated. However, these functions need to use the JSI calling convention, which means having a signature `jsi::Value(Runtime &runtime, jsi::Value const &thisValue, jsi::Value const *args, size_t count)`, but we want to write host functions with regular C++ functions (i.e. taking a specific number of typed and named arguments) instead. The current solution for this problem is to wrap the host functions in lambdas which take the JSI arguments and convert them to the C++ ones, for example ```C++ auto _scrollTo = [scrollTo]( jsi::Runtime &rt, const jsi::Value &thisValue, const jsi::Value *args, const size_t count) -> jsi::Value { int viewTag = static_cast<int>(args[0].asNumber()); double x = args[1].asNumber(); double y = args[2].asNumber(); bool animated = args[3].getBool(); scrollTo(viewTag, x, y, animated); return jsi::Value::undefined(); }; ``` However, since almost the same code has to be written for each host function, this introduces a lot of similar boilerplate functions in `RuntimeDecorator`, reducing readability. We'd like to instead have a generic solution, which can take a regular C++ `std::function<ReturnType(ArgumentTypes...)>` and create this wrapper for us. This PR accomplishes that by implementing a generic function `convertArgs<ArgumentTypes...>(jsi::Runtime &rt, const jsi::Value *args, const size_t count)` which walks over the `args`, calls appropriate functions to cast them to the native C++ types `ArgumentTypes...` and returns a tuple with the result. Then, this result is used by `createJsiFunction(function)` to wrap the `function` in a lambda which takes the JSI arguments, calls `convertArgs` on them, and calls the `function` with them. This approach has some limitations: - the conversion to native types is done using the `T get<T>(jsi::Value const *)` function, which is defined only for a few basic types. This restricts the argument types for host functions to only these types, but can easily be extended by specializing the `get<T>(...)` for the desired type. - if the host function wants to access the `Runtime`, it must take `Runtime &` as its first parameter. This simplifies the conversion code (since we don't have to check for each argument if it's a value passed through JSI or the runtime), and seems like an acceptable trade-off. - the host function cannot access the `thisValue` argument passed from JS. This isn't an issue in our use case, since the called functions are installed as globals. - the host function cannot access `args` and `count` explicitly. As such, it's impossible to implement host functions which take a non-fixed number of arguments by checking the number of arguments passed from JS. - the return type of the function must be either `void` (in which case `jsi::Value::undefined()`) or convertible to `jsi::Value`, in which case it is returned. ## Test plan Check that features depending on the host functions installed in `RuntimeDecorator` (i.e. basically everything) work like before.
fluiddot
pushed a commit
to wordpress-mobile/react-native-reanimated
that referenced
this pull request
Jun 5, 2023
## Summary This PR fixes formatting after changes in software-mansion#4027. ## Test plan Check if CI is green.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Reduces boilerplate around installing native JSI functions in
RuntimeDecorator
.We need to install Host Functions into the JS runtimes that will be called by reanimated.
However, these functions need to use the JSI calling convention, which means having a signature
jsi::Value(Runtime &runtime, jsi::Value const &thisValue, jsi::Value const *args, size_t count)
, but we want to write host functions with regular C++ functions (i.e. taking a specific number of typed and named arguments) instead.The current solution for this problem is to wrap the host functions in lambdas which take the JSI arguments and convert them to the C++ ones, for example
However, since almost the same code has to be written for each host function, this introduces a lot of similar boilerplate functions in
RuntimeDecorator
, reducing readability.We'd like to instead have a generic solution, which can take a regular C++
std::function<ReturnType(ArgumentTypes...)>
and create this wrapper for us.This PR accomplishes that by implementing a generic function
convertArgs<ArgumentTypes...>(jsi::Runtime &rt, const jsi::Value *args, const size_t count)
which walks over theargs
, calls appropriate functions to cast them to the native C++ typesArgumentTypes...
and returns a tuple with the result.Then, this result is used by
createJsiFunction(function)
to wrap thefunction
in a lambda which takes the JSI arguments, callsconvertArgs
on them, and calls thefunction
with them.This approach has some limitations:
T get<T>(jsi::Value const *)
function, which is defined only for a few basic types. This restricts the argument types for host functions to only these types, but can easily be extended by specializing theget<T>(...)
for the desired type.Runtime
, it must takeRuntime &
as its first parameter. This simplifies the conversion code (since we don't have to check for each argument if it's a value passed through JSI or the runtime), and seems like an acceptable trade-off.thisValue
argument passed from JS. This isn't an issue in our use case, since the called functions are installed as globals.args
andcount
explicitly. As such, it's impossible to implement host functions which take a non-fixed number of arguments by checking the number of arguments passed from JS.void
(in which casejsi::Value::undefined()
) or convertible tojsi::Value
, in which case it is returned.Test plan
Check that features depending on the host functions installed in
RuntimeDecorator
(i.e. basically everything) work like before.