Skip to content
This repository has been archived by the owner on Aug 8, 2024. It is now read-only.

Automatically prevent double navigation actions somehow #16

Closed
brentvatne opened this issue Feb 9, 2018 · 25 comments · Fixed by akveo/kittenTricks#180
Closed

Automatically prevent double navigation actions somehow #16

brentvatne opened this issue Feb 9, 2018 · 25 comments · Fixed by akveo/kittenTricks#180

Comments

@brentvatne
Copy link
Member

brentvatne commented Feb 9, 2018

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

@dantman
Copy link

dantman commented Feb 10, 2018

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: react-navigation/react-navigation#2578 (comment)
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
Copy link
Member

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
Copy link

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

@brentvatne
Copy link
Member Author

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

@evanjmg
Copy link

evanjmg commented Feb 27, 2018

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
Copy link
Member Author

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

@radiodario
Copy link

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
Copy link
Member Author

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
Copy link

evanjmg commented Mar 2, 2018

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
Copy link
Member Author

@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
Copy link

evanjmg commented Mar 3, 2018

Some improvements:

  • the drawer stack navigator should have an option to enable headers.
  • routing from parent navigator to sub navigators without using initial route name and rather just using a key to reference the child navigator

@brentvatne
Copy link
Member Author

brentvatne commented Mar 3, 2018

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
Copy link

oh. not work on my device huawei y221 !

@rbadr
Copy link

rbadr commented Mar 27, 2018

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
Copy link

Xenc5 commented Mar 27, 2018

@rbadr Are you able to use unique keys?

@rbadr
Copy link

rbadr commented Mar 29, 2018

@jeevantakhar I'm generating my own custom unique key

@jakeacooley
Copy link

jakeacooley commented May 2, 2018

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
Copy link

j-mendez commented May 4, 2018

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
Copy link

Rutulpatel7077 commented Jun 18, 2018

+1
should not push route with duplicated key

@ericvicenti
Copy link
Contributor

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
Copy link

Rutulpatel7077 commented Jun 18, 2018

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

@ericvicenti
Copy link
Contributor

I think about 1.5. But you should switch to v2

@Rutulpatel7077
Copy link

Thanks! It works!

@MacKentoch
Copy link

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

@brentvatne
Copy link
Member Author

@MacKentoch - post an issue with a repro

@react-navigation react-navigation locked and limited conversation to collaborators Jun 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.