mattermost / mattermost-mobile

Next generation iOS and Android apps for Mattermost in React Native
https://about.mattermost.com/
Apache License 2.0
2.25k stars 1.36k forks source link

Do not use awat Navigation.[action] #6892

Closed Romick2005 closed 1 year ago

Romick2005 commented 1 year ago

https://github.com/mattermost/mattermost-mobile/blob/459801d10bc761067f0acd9d75f31af9c9fe6ce8/app/actions/navigation/index.js#L347

Suggested use Navigation.dismissOverlay(componentId);

enahum commented 1 year ago

Can you elaborate on your reasoning? We need to wait until the screen is dismissed in various places around the code to prevent race conditions, but I'm intrigued with your suggestion

Romick2005 commented 1 year ago

So yeah, react-native-navigation does not have good documentation on function returning values: https://wix.github.io/react-native-navigation/api/overlay#dismissoverlay Even in their examples they do not use await to demonstrate that it is async https://github.com/wix/react-native-navigation/blob/57750961a9ad6b17d8c0e8f1ae247436116bd406/playground/src/screens/Alert.js#L25 Suggested change there

Navigation.dismissOverlay(this.props.componentId).then(
  () => { console.log('Successful navigation'); }
).catch(
  (error) => { console.error('Cannot navigate', error); }
);

Also check other code sample and seems that you have the most correct one! Congrats! https://snyk.io/advisor/npm-package/react-native-navigation/functions/react-native-navigation.Navigation.dismissOverlay

Romick2005 commented 1 year ago

Just checked iOS native part and it seems that there actually no native async code. It's just simulate async execution.

- (void)dismissOverlay:(NSString *)componentId
             commandId:(NSString *)commandId
            completion:(RNNTransitionCompletionBlock)completion
             rejection:(RNNTransitionRejectionBlock)reject {
    [self assertReady];
    RNNAssertMainQueue();

    UIViewController *viewController = [_layoutManager findComponentForId:componentId];
    if (viewController) {
        [_overlayManager dismissOverlay:viewController];
        [_eventEmitter sendOnNavigationCommandCompletion:dismissOverlay commandId:commandId];
        completion();
    } else {
        [RNNErrorHandler reject:reject
                  withErrorCode:1010
               errorDescription:@"ComponentId not found"];
    }
}

https://github.com/wix/react-native-navigation/blob/f54deef1bf53cda2809ae38b19b62d47d2166572/lib/ios/RNNCommandsHandler.m#L475

enahum commented 1 year ago

The nature of most react methods on native modules are async, unless:

  1. Stated otherwise as a blocking method
  2. Using the new architecture of react native, which the navigation library does not support yet