react-navigation / rfcs

RFCs for changes to React Navigation
Other
88 stars 16 forks source link

Automatically prevent double navigation actions somehow #16

Closed brentvatne closed 6 years ago

brentvatne commented 6 years ago

You can do this manually with idempotent pushes now: https://github.com/react-navigation/react-navigation.github.io/issues/42 But it would be nice if we can somehow handle this automatically (without resorting to some kind of debouncing of the actions): https://github.com/react-navigation/react-navigation/pull/1348

dantman commented 6 years ago

I'm of two opinions.

Firstly, I don't think react-navigation should be responsible for preventing this on its own. It's an interesting extra feature, but I don't think react-navigation handling this should be the gold standard. If you double submit an ajax call you get 2 HTTP requests, if you double submit a "ADD_ITEM" redux dispatch you add two items, if you double submit a form you get 2 HTTP POSTs, if you double submit an INSERT row for a sqlite database you get 2 new rows. There are dozens of things other than a navigate dispatch you can do as part of a button press or other double submittable user action. And just about all of them will have similar issues of an unwanted extra being created. As such I think the gold standard here is providing a way to make preventing double submit for any user action event easy. My preference would be creating a recommended library for this functionality (ie: blocking subsequent calls to a handler until the task started by the first one finishes). And providing an easy to use signal in react-navigation to indicate when a navigate action's task completes (ie: when all queued navigate actions have finished their transitions). Like a TransitionManager.isTransitioning() => bool, TransitionManager.runAfterTransition(cb), and/or TransitionManager.waitForTransitions() => Promise

Secondly however, if you do want to handle this automatically as an extra feature I have already come up with a fairly unobtrusive way of making navigate actions idempotent automatically. It was in the second half of this comment: https://github.com/react-navigation/react-navigation/pull/2578#issuecomment-330835051 The basic idea is making navigate work like goBack where the key of the source (the route the action is being dispatched from, not the key of the route being created) is included and giving navigate a "exclusive push after" semantics (ie: fail with a warning if a route already exists after this route).

satya164 commented 6 years ago

Firstly, I don't think react-navigation should be responsible for preventing this on its own.

Native handles this automatically, and it's a common enough issue that react navigation should handle automatically IMO. I think it's unreasonable to manually handle it for every button in the app even if it was easy.

From my reply in the discussion on slack earlier:

I think we need several heuristics to block duplicate navigation actions:

  1. Both navigation actions should originate from the same screen (same key)
  2. Both navigation actions should be of the same type, e.g. both should be PUSH
  3. Both navigation actions should happen in short-interval, e.g. between the start and end of transition

Otherwise, it might block intentional navigations, e.g. user clicked another button or press back before the transition actually finished, or there is a RESET action after PUSH

Vishal1419 commented 6 years ago

When are you planning to release a new version which will have the capability to accept key in navigation function?

brentvatne commented 6 years ago

@Vishal1419 - this.props.navigation.navigate({routeName: 'abc', key: 'whatever'})

evanjmg commented 6 years ago

key works for most cases, but if you navigate from the side drawer and then open the sidedrawer again and navigate to the same key it breaks :(. These bugs need to be fixed, but yeah let's keep the key solution! My workaround for this situation is the following:

debounce(() => {
    this.props.navigation.navigate(PAYMENT_DETAILS_VIEW)
  }, 750, { leading: true,  trailing: false })
brentvatne commented 6 years ago

@evanjmg - can you post a demo of this on https://snack.expo.io? happy to investigate that

radiodario commented 6 years ago

I've been using the key solution, but i'm now getting an error on 1.2.1, which i think it's the same thing @evanjmg reported

should not push route with duplicated key fromMainSettings

push
    hashAssetFiles:105610:29
getStateForAction
    hashAssetFiles:110181:47
nav
    hashAssetFiles:104909:36
combination
    hashAssetFiles:92773:36
<unknown>
    hashAssetFiles:93532:31
dispatch
    hashAssetFiles:92542:36
<unknown>
    hashAssetFiles:99060:26
<unknown>
    hashAssetFiles:94050:26
navigate
    hashAssetFiles:105494:33
onPress
    hashAssetFiles:142823:27
touchableHandlePress
    hashAssetFiles:37322:45
_performSideEffectsForTransition
    hashAssetFiles:31600:34
_receiveSignal
    hashAssetFiles:31537:44
touchableHandleResponderRelease
    hashAssetFiles:31426:24
_invokeGuardedCallback
    hashAssetFiles:2707:23
invokeGuardedCallback
    hashAssetFiles:2681:41
invokeGuardedCallbackAndCatchFirstError
    hashAssetFiles:2684:60
executeDispatch
    hashAssetFiles:2767:132
executeDispatchesInOrder
    hashAssetFiles:2774:52
executeDispatchesAndRelease
    hashAssetFiles:6313:62
forEachAccumulated
    hashAssetFiles:6308:41
processEventQueue
    hashAssetFiles:6369:147
runEventQueueInBatch
    hashAssetFiles:6616:83
handleTopLevel
    hashAssetFiles:6620:33
<unknown>
    hashAssetFiles:6649:55
batchedUpdates
    hashAssetFiles:5748:26
batchedUpdatesWithControlledComponents
    hashAssetFiles:2865:34
_receiveRootNodeIDEvent
    hashAssetFiles:6648:50
receiveTouches
    hashAssetFiles:6662:249
__callFunction
    hashAssetFiles:2316:47
<unknown>
    hashAssetFiles:2145:29
__guard
    hashAssetFiles:2287:11
callFunctionReturnFlushedQueue
    hashAssetFiles:2144:19
brentvatne commented 6 years ago

will need you to post a repro case to https://snack.expo.io @radiodario. this works as expected for me: https://snack.expo.io/SyeO1SwOG

evanjmg commented 6 years ago

I have an example here when I go to B, I can't go back to A? Shouldn't it go back to it without using goBack or without the key? Without using key, I can go to a new view - but again that adds more to the stack. https://snack.expo.io/@evanjmg/idempotent-navigate . Unfortunately I cannot reproduce my sidebar bug - it's on a very large enterprise project, but I think nested navigators need more work.

Also, open the sidebar and click the one of buttons really fast. They will eventually stop working on slow taps, it's not an issue for without key.

brentvatne commented 6 years ago

@evanjmg - I re-organized that example to be a bit more clear: https://snack.expo.io/SJxu8wv_f

you can't navigate to A with the key UNIQKEY! because there is no route A with key UNIQKEY!. the difficulty here is that you can't specify the key for the initial route and it's dynamically generated. so if you want to navigate back to it, you need to use goBack(). there are two upcoming changes that make it so you can easily use navigate for this:

but I think nested navigators need more work.

what do you mean? it's not possible to act on feedback like this, we need more specific information ;)

Also, open the sidebar and click the one of buttons really fast. They will eventually stop working on slow taps, it's not an issue for without key.

can't repro this, can you post a video? what does stop working mean?

evanjmg commented 6 years ago

Some improvements:

brentvatne commented 6 years ago

the drawer stack navigator should have an option to enable headers.

~just put a stack inside or put a drawer inside a stack~ please open a rfc for this if you think it's important

routing from parent navigator to sub navigators without using initial route name and rather just using a key to reference the child navigator

see the rfc i mentioned

armanbabai commented 6 years ago

oh. not work on my device huawei y221 !

rbadr commented 6 years ago

I completely agree with @satya164 , it's really a big issue for a lot of users and especially in production apps (users have different mobile phones, old/laggy ...), and react-navigation should be able to handle it (checking the route name and params before pushing a new route).

@brentvatne I have tried idempotent pushes but in dev mode it display the red screen (should not push route with duplicated key) when you push a route with the same key, and sometimes it mounts the same screen and unmout it quickly. It also doesn't work with NavigationActions.navigate unfortunately.

We're using now debouncing on every button in our app to avoid duplicating screens (which is just a hack).

I hope that we can find a solution for this issue, and would love to help if necessary !

Xenc5 commented 6 years ago

@rbadr Are you able to use unique keys?

rbadr commented 6 years ago

@jeevantakhar I'm generating my own custom unique key

jakeacooley commented 6 years ago

I'm getting the same error as @radiodario. @brentvatne Here is my StackNavigator which is nested inside of a TabNavigator:

import React from 'react';
import { TouchableOpacity, View, Text, Button } from 'react-native';
import { StackNavigator } from 'react-navigation';
import { Icon } from 'native-base';

import Profile from './Profile';
import Settings from './Settings';

import NavBar from '../../Components/NavBar';

import Colors from '../../../constants/Colors';

export default StackNavigator(
  {
    Profile: {
      screen: Profile,
      navigationOptions: ({ navigation }) => ({
        headerRight: (
          <TouchableOpacity
            onPress={() =>
              navigation.navigate({
                routeName: 'Settings',
                key: 'SettingsScreen'
              })
            }
          >
            <Icon name="ios-settings" style={{ paddingRight: 10 }} />
          </TouchableOpacity>
        ),
        headerTitle: navigation.state.routeName
      })
    },
    Settings: {
      screen: Settings,
      navigationOptions: ({ navigation }) => ({
        headerLeft: (
          <TouchableOpacity onPress={() => navigation.goBack()}>
            <Icon name="ios-arrow-back" style={{ paddingLeft: 10 }} />
          </TouchableOpacity>
        ),
        headerTitle: navigation.state.routeName
      })
    }
  },
  {
    navigationOptions: ({ navigation }) => ({
      // header: props => <NavBar {...props} {...navigation} />
      headerStyle: {
        backgroundColor: Colors.tintColor,
        shadowOffset: { width: 0, height: 0 },
        shadowRadius: 3,
        shadowOpacity: 0.3
      },
      headerTitle: props => <Text style={props.style}>{props.children}</Text>,
      headerTitleStyle: {
        flex: 1,
        fontFamily: 'bold',
        textAlign: 'center',
        letterSpacing: -0.2
      },
      headerLeft: <View />, // Empty View's to Center Title on Android
      headerRight: <View />
    })
  }
);

Error: should not push route with duplicated key SettingsScreen

j-mendez commented 6 years ago

This issue occurs if you rapidly press on something to navigate it will try to push a duplicate scene. To solve this keep track of the scene once you call navigate and make sure its not === to the same route onPress.

Rutulpatel7077 commented 6 years ago

+1 should not push route with duplicated key

ericvicenti commented 6 years ago

This issue is fixed in recent versions of react-navigation. Just provide a unique key when you call navigation.navigate({ routeName: 'RouteName', key: 'unique-route' })

Rutulpatel7077 commented 6 years ago

I am using "react-navigation": "1.3.1", which version you are talking about ? @ericvicenti

ericvicenti commented 6 years ago

I think about 1.5. But you should switch to v2

Rutulpatel7077 commented 6 years ago

Thanks! It works!

MacKentoch commented 6 years ago

Still not fixed in V2 it can navigate twice to same scene even if using a unique key to navigate().

brentvatne commented 6 years ago

@MacKentoch - post an issue with a repro