- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Add optional key to navigate action, allowing idempotent pushes #135
Comments
In my own in-dev redux app (admittedly not thoroughly tested yet) I've implemented the following to try enforce idempotency: Action creation helper functions:
Navigation reducer:
As can be seen, I've not bothered pouring through react-navigation just yet in order to see how this will affect nested navigation / subrouters. However, if my solution does make sense, then it may be worth adopting in react-navigation. |
I totally agree it makes sense to make them idempotent. TabRouter should already behave this way, and we should add a test to prove it. If you want to submit a PR to make StackRouter also respect this case, I'd support it. |
Any updates on this? I'm noticing if you tap something several times to bring up a new view, it pushes that view several times. |
Merging #802 here because a navigate action key would also enable |
@ericvicenti Hi, Thank you for your supporting, but I really need a mini example how to not re-render an existent screen in stack :( Thank you. |
Any updates on this? |
@ericvicenti How can we use |
I wonder if there's a way react could have some sort of interaction Very crudely: onTap() {
const {locked} = this.state; // Can also be used in render to disable buttons, etc...
if ( locked ) return;
this.lock(async () => {
return await myAction();
});
} Presuming that 2 taps happen in one tick and 1 more happens after the setState tick.
The difficult part in react-navigation would probably be providing something like async navigation actions, ie: actions that are not fire-and-forge like the current model but instead do not resolve until the navigation transition has completed. Or something similar to |
Here is a crude solution to this problem if anyone needs a quick fix: #1303 I'll close out the PR when there is another solution available. |
|
As far as I'm concerned this has absolutely nothing to do with button presses. Pushing a navigation route is possible a plethora of ways. Attempting to prevent duplicate pushes all the various ways that a navigation route can possibly be pushed sounds truly awful. That said, I'm not indicating support for existing PR, I haven't looked it it closely. Nonetheless, idempotent pushes is clearly a good generic solution to this issue. Idempotency is a good principal to follow in software in general, where it makes sense. There is no legitimate situation ever where an app wants to push the same route twice. Yes, you may want the same screen twice, but you'll want different parameters for each screen, ergo they're not the same route. |
That doesn't really change anything. Using something like The point is that it's the responsibility of the code telling react-navigation to navigate to another screen to know whether it should be changing the navigation state of the app (or making an API call). Not the responsibility of the dispatcher to try and guess whether 2 calls to
react-navigation/react-native has no concept of idempotency in the way you are describing and can't. To know if params are the same react-navigation would have to do a deep comparison of params, something which is inherently unreliable (the moment a non-plain object like a Date or a custom class gets throw in there things fall apart) and undesirable performance wise (react-native only does shallow comparison of objects). You'd also run into false negatives – the moment you add some sort of state to the params (put a callback handler into params so you can call it from a save button in the header, take a params.id to fetch associated data from an api then use setParams to put it in params so it can be used in the header, etc...) params are no longer equal to the params you'd use when navigating to a new scene. The PR being discussed just sets the For any app that has any screen that is used multiple times with different params, this is a breaking change. And if you also have one of those screens where it's really difficult to derive a unique key when you are trying to navigate to a new screen, you're SOL. |
@Benjamin-Dobell this PR #2334 is not about double pushes. It's trying to solve the idempotency problem by defaulting the key to the route's name and allowing an optional key to be specified by the user if he wants the same transition twice in a row. |
I think you may misunderstand this issue and/or idempotency. To implement idempotency none of what you described is necessary, I'm already doing it perfectly fine in my own app - the solution is already described in this issue, in fact in my initial comment. We're just waiting on someone (possibly myself) to submit a clean PR.
Are your routes being persisted anywhere? Because if they're not simple serialisable data structures that isn't going to work either. My solution in my own project does deep comparison on params just fine (in addition to a route key). Also, yes, react-navigation is idempotent (in some areas), and should be idempotent. I refer you @ericvicenti's response, the third comment in this very thread. |
react-navigation's current "idempotency" is different, TabNavigator and DrawerNavigator do not have a stack, they have a fixed set of items and
Please see #145 as one example of people actively putting non-serializable data into params. Also you don't have to use non-serializable data to break a non-custom deep compare algorithm.
Are you referring to comment 2 in this issue? That algorithm doesn't even do a deep compare, it's completely shallow. Before breaking on non-serializable data idempotency will disappear the moment you do something like Is no-one thinking about the code that makes the call to dispatch? Some sort of handler is triggered multiple times (doesn't matter what it is, something in your code gets called twice when it should only be called once) it probably does something and then uses |
No, I'm not, that comment is 6 months old. I don't know why you're opposed to idempotency, if you don't think it solves any problems, then so be it, although it does make me question how you're building your apps and APIs. Nonetheless, if implemented properly it's not going to cause any API breakage or negatively impact you or your project in any way. If you have a problem with a particular implementation, then comment on the PR, not this issue. |
@dantman By the way, I suggest looking into how React Native is implemented. It uses an asynchronous bridge between native code (UI thread) and a JavaScript environment. If you're building your app under the assumption you can stop double execution across the entirety of your app then you're fighting an impossible battle. This assumption just isn't going to hold true; you should build your app and back-end accordingly. |
I know how react-native is structured, the async bridge doesn't stop you from dealing with double execution. Unless you're redefining my use of double execution to refer to accidental additional executions of the logic inside whatever handlers you'd put a |
From the OP:
Has this approach been investigated further? |
Reworked my initial PR #2334 and added a second one concerning dispatch and transition synchronization #2400
|
Is there an ETA for this? |
@adrianboimvaser there are a couple of PRs open with different proposals on how to allow idempotent pushes. You are encouraged to provide your feedback in the discussion on those PRs. You are also welcome (and encouraged) to open a PR as well! 😄 |
Come on, still no ETA? |
A few of us came to the consensus that the best technique would be to use a "source key" like how We talked about bundling this into an RFC on new navigator actions like a newer push, replaceAt, and jump navigation actions like ex-navigation has, but I haven't had the time to write up an RFC for it. But someone is welcome to work on writing a PR to give navigate this type of behaviour. I believe I gave enough of an explanation in how it would work for someone else to wrap their head around it and come up with an implementation. I can't use client time to work on this, because while I agree this change is a good behaviour to give to react-navigation and this double-navigate bug does affect the app I'm working on for them, this isn't the right fix for that app. We do other things beside navigate in our actions, so we need to work on a general purpose interaction lock rather than being able to just work around that with making navigator.navigate idempotent. |
it's merged in #3393 |
Co-Authored-By: Satyajit Sahoo <[email protected]>
Due to the fact react native generally doesn't block the native run loop, navigating to a new screen causes a state change resulting in an async render. Whilst this occurs the screen for the previous state can still be interacted with, so quickly tapping a button twice can easily cause duplicate route pushes resulting in a navigation stack that doesn't necessarily make any sense in the context of the app.
Also, because react navigation does animations in JS rather than using native transitions (iOS), user interaction is incorrectly enabled whilst rendering the new state. So even post (virtual DOM) render, during the transition the previous screen still has interaction enabled, allowing double taps as described above.
The ideal solution would be to disable interaction in response to a navigation action,
however as the communication between JS-land and native-world is async, this is virtually impossible.EDIT: Come to think of it, it's not at all impossible, it's just a bit of a pain to implement.
As such, the most logical solution is to ensure all state changes are idempotent. In the case of redux, this means that all actions should be idempotent (not just those related to navigation transitions).
Whilst the responsibility of idempotency of non-navigation actions is clearly up to app developers, react-navigation should probably ensure its own actions (and helper functions) help enforce idempotency.
The text was updated successfully, but these errors were encountered: