Skip to content
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

Implement dispatchCommand on Paper #4586

Merged
merged 14 commits into from
Jul 20, 2023
Merged

Conversation

tomekzaw
Copy link
Member

Summary

Previously, dispatchCommand was available only on Fabric. This PR implements dispatchCommand for Android and iOS (Paper) as well as exports the function in react-native-reanimated.d.ts.

Android:

android.mp4

iOS:

ios.mp4

Test plan

DispatchCommandExample.tsx

new GuardedRunnable(mContext.getExceptionHandler()) {
@Override
public void runGuarded() {
mUIManager.dispatchCommand(viewTag, commandId, commandArgs);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, this will be run on the mqt_native_module thread so there may be a slight delay if we don't synchronize it properly with the UI thread. Instead, maybe we should push commands to the queue here and flush the queue in performOperations which is already synchronized with the UI thread?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this already works fine:

android2.mp4
ios2.mp4

@tomekzaw tomekzaw requested a review from piaskowyk June 20, 2023 06:53
ios/native/NativeProxy.mm Outdated Show resolved Hide resolved
Copy link
Member

@piaskowyk piaskowyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo dispatchCommand method by definition is an asynchronous operation, so on iOS we should use RCTExecuteOnUIManagerQueue instead of RCTUnsafeExecuteOnUIManagerQueueSync I think.

I just wondering if we should add even short information to the documentation about the possibility of using the dispatchCommand method from UI Thread. What do tho think?

The rest of PR looks good 👍

@tomekzaw tomekzaw requested a review from piaskowyk July 20, 2023 07:55
@tomekzaw tomekzaw added this pull request to the merge queue Jul 20, 2023
Merged via the queue into main with commit d650a01 Jul 20, 2023
18 checks passed
@tomekzaw tomekzaw deleted the @tomekzaw/dispatch-command-paper branch July 20, 2023 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants