Less pushy navigate

[brentvatne]

No description provided.

[dantman]

Make me refactor my code to use PUSH instead of NAVIGATE if you must. But in my opinion "works like push 95% of the time, but instead works like FOCUS the other 5% of the time you already have a route by that name in the stack" is not an intuitive behaviour.
Instead of people making "I get two routes pushed when a user accidentally double taps my button because by event handler doesn't properly handle the mistake" bugs you'll probably get people surprised and making "When I call navigate('Profile', {user: 'Bob'}) from Bob's avatar on Alice's comments page it acts like the back button putting me on Alice's profile page, but with "Bob" in the title" questions/bugs.

[brentvatne]

@dantman - why refactor from navigate to push? why not add a key to navigate action which adds some identifying information for the screen?

But in my opinion "works like push 95% of the time, but instead works like FOCUS the other 5% of the time you already have a route by that name in the stack" is not an intuitive behaviour.

I think of it a bit differently -- navigate to the route 100% of the time, and instantiate an instance of the route if it doesn't exist. Does this framing make more sense?

Instead of people making "I get two routes pushed when a user accidentally double taps my button because by event handler doesn't properly handle the mistake" bugs you'll probably get people surprised and making "When I call navigate('Profile', {user: 'Bob'}) from Bob's avatar on Alice's comments page it sends me back to Alice's profile page, but with "Bob" in the title" questions/bugs.

I'm not too concerned about this. The solution to the question is super easy: add a key.

[dantman]

why not add a key to navigate action which adds some identifying information for the screen?

Fixing the double event submission fixes the problem in 100% of my use cases. Adding a key doesn't fix 100% of my use cases. And on top of it it adds unnecessary code to every single one of my navigation calls.

An idempotent navigation doesn't fix my buttons tied to AJAX requests. And a foo-edit-${itemId} key is worthless for buttons that do things like "create item, navigate to edit/view for that item".

[brentvatne]

Fixing the double event submission fixes the problem in 100% of my use cases

what do you mean by this? debouncing the handler or what?

Adding a key doesn't fix 100% of my use cases
An idempotent navigation doesn't fix my buttons tied to AJAX requests. And a foo-edit-${itemId} key is worthless for buttons that do things like "create item, navigate to edit/view for that item".

not sure I follow this either, can you elaborate?

[dantman]

what do you mean by this? debouncing the handler or what?

Not a timed debounce. When an async event handler starts lock it (reject further calls to the event handler) until the event handler has finished its work. ie: Any ajax requests have finished, any promises have finalized, and any navigation transitions have completed.

My personal plan for handling this in the app(s) I work on is to write a decorator for this, and see if I can get the necessary "wait for any pending navigation transitions to complete" method in React Navigation to make another decorator.

@lockEventHandlerWhileRunning // I'll come up with a better name later
@waitForNavigationTransitions // maybe shorten this name
async onSubmit() {
  const {data, navigation} = this.props;
  const res = await this.makeAnApiCall(data);
  if ( res.ok ) {
    navigation.goBack(); // exit full-screen dialog
  } else {
    this.showError();
  }
}

not sure I follow this either, can you elaborate?

Say you have a button/list item that creates a document (maybe based on a template) and then sends you to the document page that is used to view and edit that document. Here's some simplified mock code.

onPressDocumentTemplateItem(templateId) {
  const templateDocument = await getTemplateDocument(templateId)
  const document = {
    ...templateDocument,
    id: uuid(),
  };

  await createDraft(document);

  navigate('Document', {key: `document-${document.id}`, id: document.id});
}

If you skip the key and documents have links to other documents, then you replace the Document route and can't return to where you navigated from. If you use the document's ID to differentiate between documents, then you haven't solved the double submit issue because each press creates a different draft with a different id.

[brentvatne]

My personal plan for handling this in the app(s) I work on is to write a decorator for this, and see if I can get the necessary "wait for any pending navigation transitions to complete" method in React Navigation to make another decorator.

interesting, I'd be curious to understand the use case for this. in the case you describe it would already be handled properly because the goBack navigation helper now pre-populates the action with the current key, so if there is no route with that key in the state then it noops.

If you use the document's ID to differentiate between documents, then you haven't solved the double submit issue because each press creates a different draft with a different id.

would a user potentially have two drafts open at the same time? if not, then just use a 'draft' key, otherwise you'd have to add some handling to debounce the button press. I think this is an uncommon edge case. for example, if you go to compose a tweet in twitter you can only have one of those open in a stack at any given time. same for writing a post on your wall in fb, writing a draft in medium, editing a photo in photos, creating a post on instagram, composing an email in any mail app, etc. am I miss something that makes this similar to common use cases?

[dantman]

interesting, I'd be curious to understand the use case for this. in the case you describe it would already be handled properly because the goBack navigation helper now pre-populates the action with the current key, so if there is no route with that key in the state then it noops.

Perhaps not the best example. These decorators are intended for all event handlers, whether they use goBack, navigate, or another action. So they work equally well in the avatar onPress in a comment section. Also, goBack being idempotent doesn't help fix the fact that when the submit button is double-submitted this.makeAnApiCall(data); gets called twice, and depending on how the API works I either get a duplicate item or one of the API calls returns an error.

would a user potentially have two drafts open at the same time? if not, then just use a 'draft' key, otherwise you'd have to add some handling to debounce the button press. I think this is an uncommon edge case. for example, if you go to compose a tweet in twitter you can only have one of those open in a stack at any given time. same for writing a post on your wall in fb, writing a draft in medium, editing a photo in photos, creating a post on instagram, composing an email in any mail app, etc. am I miss something that makes this similar to common use cases?

Besides the draft no longer being a draft if it's saved, I dislike the idea of using different keys depending on which event handler opened the route. I'm working on an app that works pretty much like this; this is just how the API works, I have to create a draft on the server with an API call, and then I have to use the resulting ID to push the same route used to view one that's already been saved; I cannot open a route and then save drafts from it email style. And yet again, double submit of the API call. I suppose the most similar use case would be a text editor that works with a 3rd party API that limits what it is able to do.

[brentvatne]

it's not clear to me that the problems you're describing are related to react-navigation ‐ they seem like more of a concern of either userspace logic around blocking interactions until an async action completes or react-native (if there is some bug preventing you from implementing that logic). for example, the pattern used in this snack would solve the problem for you: https://snack.expo.io/By9N3KAPM (although you probably would want to have some loading indicator or state to indicate that the button is blocked from further interaction).

i can't see how we would handle this kind of case automatically with react-navigation. further, i'm not convinced that it has bearing on the viability of this rfc. i am of course very open to being shown incorrect!

[antsmo]

As someone fairly inexperienced with react-navigation I like this proposal. I do feel like the attraction for me is having to deal with keys less. That may be a good thing or not, perhaps it points out a flaw in my mental model around keys and how react-navigation works under the hood. I don’t know how routers or navigators do what they do and what role key plays in that.

Is it just like having to set a key when rendering multiple React component in that it must be unique? And if I push a route with a key that already exists it will replace it with the new route? If I’m right then I only realised that after reading this proposal 😄

[ericvicenti]

I'm definitely in support of this change. It is more in line with the intended idempotent behavior of navigate. Now that we have an explicit push action which always pushes, I think it makes sense to change the semantics of navigate in this way.

Unfortunately, it is a significant breaking change. I think is is probably worthwhile to push forward with this, but the re-education efforts will definitely be more work than the change to the code.

[ericvicenti]

Also, (this is kind of a side note), for @dantman's use case of creating a draft before pushing the screen, I would recommend pushing immediately rather than waiting on a network request, to give the user a snappy experience. Also I would note that the double-tap behavior is not specifically a navigation problem. We probably want to use debounced buttons for many cases in our apps, especially when firing network requests.

[brentvatne]

Is it just like having to set a key when rendering multiple React component in that it must be unique? And if I push a route with a key that already exists it will replace it with the new route? If I’m right then I only realised that after reading this proposal 😄

for a stacknavigator, if you use navigate and provide a routeName and key that already exist, then that routes will become focused. if not, then it'll be pushed to the stack.

[RobIsHere]

This is a great proposal. I would have expected it to work this way, so it feels more natural and would solve some issues for me.

Overlaying some load animation or another distracting in-screen animation on willFocus should be easy in user space.

[browniefed]

So to accomplish some of the demoes that are out there to highlight infinite pushability of screens on the stack. Would they all need to generate a unique key to identify the screen otherwise they would all cease to function?

[RobIsHere]

navigate(...) reuse an existing screen if a screen already exists, comparing routename and if given the key
push(...) pushes a new screen

That's how I understand the RFC: "If you want to be able to push an endless number of screens here, you would need to either 1) call push explicitly, or 2) pass in a unique key for each screen"

[browniefed]

Ah gotcha yeah, misread that.
Currently push only operates with in the current stack, will it now operate on the entire set of stacks?

[brentvatne]

@browniefed - nope, it would just work on the current stack, although we could make it accept a key to push to another stack (pr welcome for this, should be pretty easy, look at what i did recently for pop to top).

the examples that show pushing an unlimited number of screens are pretty contrived, if you wanted to re-create those examples you would use push, or you can use navigate with a key as in the code example in rfc. would love to hear about cases where this behavior is used in the real world and doesn't have any unique data associated with the screen -- i can't think of any. i can think of one weird situation though:

1. you're on your facebook home screen
2. you go to jason's profile
3. from jason's profile, you go to brent's profile
4. from brent's profile, you go to jason's profile
5. what should happen here? should it go "back" to jason's or should it push a new screen? what if we added more screens between 4 and 5?

[browniefed]

That is a slightly common situation in my app.

Home page of interest.
Click on a Stock Symbol like TWTR. Click on Facebook related stock, click on TWTR related stock.

Either way I think I'm on board with the changes. Trying to wrap my head around what the transitions would look like with high level StackNavigators that are modal, and various levels deep of stacks.

[dantman]

I don't care about idempotent NAVIGATE with keys, they just add work for me that is 100% unnecessary. And I have a bunch of StackNavigators because its the only way to make the navigation structure of our app in react-navigation work.

I was happy to just refactor to use PUSH instead, presuming that it just worked the same as NAVIGATE used to minus the navigate back behaviour I never needed. But if you're telling me that PUSH isn't even a working replacement for the current NAVIGATE if you have different navigator levels, then I have a problem with this RFC. The use of keys in order to have multiple entries of the same route should not be made mandatory until there is a viable option for people to opt-out of that and just PUSH routes.

[brentvatne]

@dantman - can you provide some code that demonstrates the problems you see? it's not clear to me

[dantman]

@brentvatne I'm referring to your comment suggesting that PUSH doesn't work outside of the current StackNavigator like NAVIGATE does.

#25 (comment)

[brentvatne]

@dantman - right, I follow that. I want to understand why that is a problem though. it's unclear to me what the use case is for having places all over your app where all of the following constraints apply:

1. you need to push a route that has no data that you can use for key
2. you need to push the route to a stack other than the currently active one
3. providing a key that isn't based on some data is too cumbersome
4. specifying a key for the stack you want to push too is also too cumbersome

I don't doubt that I could just be lacking in imagination here but I'm unable to map this to the flows in any app that I know of after some time thinking about it. I really want to be sure I'm not missing some important use case here

[brentvatne]

I'm open to adding an idempotent: false option to navigate or some other kind of solution to make it work more in line with your use case but I just want to understand the use case better first :)

[ericvicenti]

It’s hard to evaluate this without concrete use cases. The FB profile use case that Brent mentioned does have a solution. The correct thing in my mind, which is also how the real FB app behaves, is to always push a new profile, even if it is already in the stack. The push action would be appropriate here, but you’d need to denounce your button to prevent the double navigation

[brentvatne]

@ericvicenti - that makes sense. removing scoping for push from the deepest navigator to instead make it 'bubble' back up the router stack would resolve the problem i suppose. eagerly awaiting info about the use case @dantman has in mind though!

[dantman]

@brentvatne
Many of my event handlers do things other than just pushing a route, so I have to fix double submit in all of them anyways. So idempotent navigate doesn't help me since there won't be any double submit issues in my app.

Meanwhile the 85% of my routes are for Profile-like things where I pass ids/data to them and would not want navigate to just focus an existing screen. And topping it off at some point in the future some of those may have the circular Profile<->Comments like navigation pattern where I would not want any of the buttons to accidentally pop off a bunch of routes just because it happened to link back to the same "Profile".

So, fixing double submit at the event handler level for me is non-negotiable; idempotent NAVIGATE is not necessary. So with that out of the way, if PUSH worked like NAVIGATE without the focus, I can just refactor my code with a few find-and-replace operations to use PUSH instead of NAVIGATE because I don't need NAVIGATE. Otherwise if I have to keep NAVIGATE because PUSH sometimes doesn't work, I have to go to 85% of my navigate calls and instead of just doing a navigate->push I have to add a key to the navigate call, doing manual work for every one to identify the best ID to use for that call. And I don't have a use for those keys, this manual work and extra code in nearly every navigate call is not in service of making my app better (because I don't need it to fix double navigation issues), this manual work is just to undo the fact that react-navigation has changed so these 100% useless to me keys are now mandatory for my app to work like normal.

But I have an unfortunately complex navigator hierarchy. It's actually worse than the example I'm about to give, right now the DrawerNavigator has separate routes for each of the different Inbox types and Clients/Firms/etc... where each of those has its own StackNavigator inside of them because that was how DrawerNavigator works for making drawer items. I'm in the process of refactoring it to look like the following example where I just put a single StackNavigator inside the Drawer and do all the drawer items myself and just RESET/NAVIGATE the Stack as necessary to reduce the necessary StackNavigators.

Stack = StackNavigator[+drawer menu button handling] {
	Inbox,
	...
	Firms,
	Firm,
	Tags,
	Tag,
	Transaction,
	TransactionViewer,
}

Drawer = DrawerNavigator {
	Stack: Stack,
	/**
	 * The actual drawer items are something like:
	 * Workspace: Inbox{bundle=myworkspace}
	 * Upcoming: Inbox{bundle=upcoming}
	 * Snoozed: Inbox{bundle=snoozed}
	 * Drafts: Inbox{...draft search query params...}
	 * ...
	 * Firms: Firms{}
	 * Tags: Tags{}
	 */
}

ModalNavigator = StackNavigator[mode=modal, headerMode=screen] {
	NoModal: Drawer,
	DialogFoo,
	DialogBar,
	...
	DialogN,
}

Navigator = StackNavigator[header=null] {
	Login,
	Logout,
	ForgotPassword,
	Main: ModalNavigator,
}

Stack is the normal StackNavigator where the normal navigation happens. As I said I'm working on a refactor to put them all into one navigator. Previously Drawer had something like Workspace: StackNavigator{Inbox, Transaction, TransactionViewer}, Upcoming: StackNavigator{Inbox, Transaction, TransactionViewer}, ..., Firms: StackNavigator{Firms, Firm, Transaction, TransactionViewer}, Tags: StackNavigator{Tags, Tag, Transaction, TransactionViewer}.

Drawer is a DrawerNavigator needed to provide the main drawer navigation for navigating between the screens of the app. (react-navigation/react-navigation#2963)

ModalNavigator is a StackNavigator used for full-screen dialogs. This has to be separate because we can't put full-screen dialogs in Stack and just turn on mode=modal and headerMode=screen per-route.

And the top level Navigator is necessary because we have screens like Login and Logout. These screens are not only headerless, but the Drawer navigation is a logged-in only thing, the drawer shouldn't even exist unless you are logged in.

So that's the answer to "why so many StackNavigators?". StackNavigator has a bunch of different modes that can only be set at the Navigator level so you need to create multiple to get some types of UI to work. And sometimes interaction with/encapsulation of other types of navigators necessitates another StackNavigator layer.

(Also keep in mind that this app is incomplete, the web service this app accesses has piles more functionality that in the future I will likely be working on adding to the app)

Right now navigate works pretty nicely, as long as you keep route names unique a navigate('routeName') will always push a route in the correct navigator. A route in Stack can navigate('DialogFoo') to push a dialog onto the ModalNavigator. Drawer can both navigate to a new Inbox, Firms, etc... screen in Stack and can also do a RESET and navigate to Logout in Navigator to logout. And a CopyTransaction dialog in ModalNavigator can goBack() to exit the dialog while using navigate('Transaction', {...}) to push a viewer onto Stack for the transaction that was just created.

If PUSH can't do the same thing, then I can't use it in half my app. And as I said the necessary keys to use the new NAVIGATE are completely worthless to me and just add more work and potential unexpected bugs down the road. Having to write idempotent: false in every single navigate call (or actually key: null would probably be a better interface) would technically mean I don't need to manually figure out keys everywhere, but it does mean that I still have to add extra code to every navigate call just to opt-out of a feature that is unhelpful and counterproductive for me.

---

To illustrate the "circular Profile<->Comments like navigation pattern". Imagine you have a Profile route with a comments area, and all the comments have an avatar that push on a Profile route.

Imagine you visit Alice's profile, tap the avatar of one of Bob's comments, then tap on one of Joe's comment avatars. You're now on "Home > Profile{Alice} > Profile{Bob} > Profile{Joe}" (though in reality it could be much much deeper). Now imagine there is a reply from Alice and you tap on her avatar. Which do you think is the correct behaviour? Navigate back to "Profile{Alice}" so now the back button returns you to "Home" instead of back to the "Profile{Joe}" profile you were on. Or push a second "Profile{Alice}" on so you have a stack of "Home > Profile{Alice} > Profile{Bob} > Profile{Joe} > Profile{Alice}".

I have a scenario like this with my Transaction screen that will likely become possible soon, and as I already mentioned, some of the dialogs not part of the Stack navigator have completely valid reasons to push a Transaction.

[brentvatne]

thank you for the detailed explanation! it sounds like the case where you may end up wanting navigation to a new route with a key based on data doesn't make sense in this significant case we've both identified (which you describe in the "circular Profile<->Comments like navigation pattern"). I think the solution here is to have push behave like navigate does currently, rather than being scoped to the deepest navigator. I think this makes a lot more sense with the changes to navigate described in this RFC. I will update the RFC to explain that we will make StackNavigator actions bubble up in the same way that navigate does