Fix idempotent pushes during transition. #2578
Conversation
| @@ -9,6 +9,8 @@ const INIT = 'Navigation/INIT'; | |||
| const NAVIGATE = 'Navigation/NAVIGATE'; | |||
| const RESET = 'Navigation/RESET'; | |||
| const SET_PARAMS = 'Navigation/SET_PARAMS'; | |||
| const SET_TRANSITION_END = 'Navigation/SET_TRANSITION_END'; | |||
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
| @@ -141,6 +141,48 @@ export default ( | |||
| }; | |||
| } | |||
|
|
|||
| // params can be set while in transition | |||
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
| @@ -141,6 +141,48 @@ export default ( | |||
| }; | |||
| } | |||
|
|
|||
| // params can be set while in transition | |||
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
|
@davepack and also, more important, now you are tight coupling the router to the Transitioner. I think they are suppose to be weak coupled. The router should reflect the route state not the transition state. The Transitioner should be smart enough to figure it out what's next not the other way around. My two cents.. |
|
And of course it's not react's way to do it like this. The state should flow in one direction alone, from parent to child |
|
If I understand correctly, you intend to ignore any dispatched action if you are in transition. How I am suppose to know when a transition is ended?!? because there is no callback fired when dispatch finishes. Right! dispatch is suppose to be synchronous and to return immediately the next state. It was designed like this in order to play nice with redux. With your solution I will not be able to navigate programmatically because I don't know when a transition is finished unless I tap into the onTransitionStart onTransitionEnd events. I recommend to take a look at #1206. There's a long talk in there about these issues |
|
Hi @rpopovici, thanks for weighing in, your solution was one I looked at while coming up with this solution.
I feel like this solution is less complex since there are fewer control flow statements and less repetition. It feels easier to understand the flow and what is happening. But obviously we'll both be biased to our own solutions as far as what is easier to understand :-)
This statement sounds like a "programming maxim" and I'm unfamiliar with the the reasons underlying it. Are there side-effects that I'm unaware of? It would be helpful to know the risks of making the solution "time-based".
I don't think this causes tighter coupling. It doesn't create any kind of dependency between the two. Both can still operate just as independently as previously.
The only way to accurately time the transition is by knowing exactly when it start and ends. Again, I don't exactly see the risk or problem you're trying to convey here, or why this statement is necessarily true. It would help if you could describe specific problems you're trying to avoid since I don't inherently see the problem with this like you do.
I'm not sure how this is being violated here. Please explain a little more.
I think this is a valid concern. See @rmevans9 comment above. We could log a warning in dev mode when trying to dispatch a navigate, back, or reset action during a transition, and encourage using onTransitionStart and onTransitionEnd events. Looking at that issue, batching would still work if the second action is a set_params. Is there a case you can think of where you'd want to do two navigate actions in a row? This would cause two transition animations in a row and I can't think of a reason we would need to allow that. |
|
Also, @dantman since you have been involved in discussions here, it would be good to get your take on this PR. |
|
@davepack Ok. I will try to explain better. By "time based" solution I meant some piece of code which involves delays, timeouts, debouncing or anything involving a timer. In our case, we have to wait until the transition animation finishes in order to be able to dispatch again. Right now in react-navigation the dispatch action is able to do only: navigation, reset, setProps and go back. But maybe in the future this will not be the case anymore. We want to have a replace action, the possibility to have transitions with/without animations, batched transitions with individual or unified transition animations, better support for web, etc. You solution is not allowing any kind of future development in this direction because it's ignoring any dispatch action, except for setProps, done during transitions. Maybe I have to reset the route in componentDidMount and the action will be ignored because it's happening during transition. Having a warning message will not help because I still don't know when the transition is ended. Another example: action dispatched after network request finish. What if this happens during transition. It's totally possible, especially if the animation runs in native thread. So, basically you are cutting out any way of doing programmatic navigation. By "react way" I was referring to the fact that react encourages data flow in one direction only. From parent to child, from dispatcher to transitioner. In your implementation you are adding a new action SET_TRANSITON which btw is not very SOLID because it's not a routing action, it's only reflecting the transitioner state. This is violating the Separation of concerns principle. Anyway, that's not the problem. The problem is the fact that you expose this action publicly and make it accessible to the user and you are creating a feedback loop between dispatcher and transitioner. They are both react components if you did not noticed. Any dispatch will trigger a render in transitioner even for setParams. Anyway, I am not saying that my solution is better. It was a quick hack I needed for my project, but so far I was unable to see anything superior to that. I think this should be simple. No complications.
|
I do think an API to wait for transitions to finish is a good idea. Then it's possible to write a general solution for rejecting user interactions while something is happening and it can just be told to also wait for transitions to finish. However I believe the correct api for this is something based of how |
|
@rpopovici @dantman thanks for your input, it's a Friday night, I want to be thoughtful about this so I'll pick this up again on Monday and respond. Have a good weekend! |
davepack
force-pushed
the
idempotent
branch
from
df3acc1
to
a13244c
Compare
|
So stepping back a bit, the main problem we're trying to fix is when somebody double taps a button that dispatches a navigation action to push a new route onto a stack navigator, two transitions will occur and two of the same route will be pushed onto the stack. I think this should be handled automatically in react-navigation, i.e. when devs upgrade react-nav this will just work without adding any code. I think arguments against handling this in react-nav usually speak to special cases that I think should be opt-in, i.e. a dev has to consciously add that feature into their code for their special case. Solutions that try to plan for numerous special cases usually end up having complicated logic. As the code stands right now, the transition is tracked in navigation state. I think this is okay. Yes, it's technically a side-effect, but maybe this is a case where it's okay to make an exception. As far as I can tell there isn't another way to get the timing exactly right and prevent multiple pushes to the stack. Logic based on this state only has to check if navigation is in a transition. Other solutions require indirect logic that feels fragile. The actions to set this state are dispatched from Transitioner. That is probably not so good. I am looking at other possibilities here. In the router, getStateForAction blocks navigate, back, and reset when in a transition. This does not prohibit future development from updating to allow for special cases, the only requirement would be that future changes not break idempotent pushes for vanilla navigate, goBack, and reset. With regards to performance and expensive re-rendering from state changes, this definitely needs to be looked at. Performance is probably something that should be looked at and fixed concurrently with or prior to fixing idempotent pushes. It's possible it will influence this solution, as you said @dantman. Thoughts @rpopovici and @dantman? |
|
@davepack I still think the opposite. There are two major faults with react-navigation trying to implicitly handle the double-submit issue automatically. First problem is determining what is and isn't a double submit.
The second problem is that double submit is not a react-navigation exclusive issue. Getting two route items because of a double submit is only one of the possible side effects. If your handler makes an api call, the same double submit ends up making two api calls. If those api calls are a POST to create something, you've created two things. If that handler creates something with an api and then navigates you to a view/edit screen, you've created 2 things and react-navigation would just discard one of those navigations silently hiding the fact that you created two things instead of solving the issue. If your handler creates a local database item, double submit creates two items. If your handler deletes/moves/copies a file with an async delay, double submit makes the filesystem action and then emits an IO error when you try to do it again. If your handler is a "send" button, you end up sending two messages over whatever the relevant protocol is. Idempotent navigation only solves the simplest possible double submit issue, an event handler that does absolutely nothing except navigate you to another screen. And leaves you SOL for everything more complex than that. This also feels somewhat out of scope. The button is not a react-navigation button, the event handler is not a react-navigation handler. The application developer has placed a button on their screen, the application developer has written an event handler to handle presses on that button, and in that event handler the application developer has told react-navigation that they want react-navigation to navigate to a screen. This issue should be fixed at its source, the event handler that causes the double submit. (Lets put this another way, double submit is also a problem with forms on the web. But no-one says that IMHO, the correct solution is to make it possible for any handler to avoid the double submit issue. Fixing this basically means making it so that the application developer can make an event handler block invocation of the event handler when the previous invocation has not finished processing (whether that is due to ajax, navigation transitions, file/database IO, or something else). The best way to do this would be an easy to use library (or a collection of them, since some developers might want to wrap their functions, others might want to call a function from inside their handler, others might want to use decorators, and some may want to use callbacks to signify processing is finished while others want to use promises; though one lib could also cover those methods together). If a library like that doesn't already exist, then it would be best to create one, publish it, and make react-navigation recommend it to developers. This will also be a benefit to the overall react-navigation community because it will be a tool that is useful outside of just react-navigation. The react-navigation specific portion of this issue is the transition. You can tell the library when an ajax request, file/database IO, or other async api has finished processing. But you can't tell when a transition is done. So the other thing we need is a simple API to tell you when any queued transitions have finished. I believe something roughly based on react-native's InteractionManager's prior art. Then this can be used in conjunction with the idempotent handlers library to quick and easily apply idempotent handling to any method with a navigate. Here's a bit of pseudo code for a decorator obsessed developer's event handler that includes one possible way an idempotent event handler could be written. import {Transitioner} from 'react-navigation';
// ...
@autobind // bind the event handler
@runAfterInteractions // Wait for button animations to complete before doing expensive api calls/transitions
@idempotent // only allow one invocation of the event handler to happen at a time, reject all other invocations until the promise returned by this method is settled
@waitForTransitions // automatically wrap the function so it waits for react-navigation transitions to finish
@handleNetworkErrors // automatically catch generic network errors and display a message
async function onSendButtonClicked() {
// Send the message
const {id} = this.props.api('/sendMessage', {message: this.state.message});
// View the sent message
this.props.navigation.navigate('MessageView', {id});
// `@waitForTransitions` already waits for transitions, or we could instead use `await Transitioner.waitForTransitions();` here
} |
|
@davepack @dantman I would suggest a different approach to this.
|
|
@dantman btw, there are lots of 3rd party libs out there which are doing more or less what you want |
Having a 3rd party lib does not increase complexity. Double submit is an issue that plagues more than just react-navigation, a 3rd party lib avoids NIH or putting out of scope things into react-navigation. You do realize that
I never said that decorators were a requirement. I explicitly stated "or a collection of them, since some developers might want to wrap their functions, others might want to call a function from inside their handler, others might want to use decorators, and some may want to use callbacks to signify processing is finished while others want to use promises; though one lib could also cover those methods together" and gave an example "for a decorator obsessed developer's event handler". There is absolutely nothing about this solution that requires decorators or fancy syntax, this is a fundamental JavaScript issue which solve using any syntax or programming pattern you want. Here are a bunch of samples for other possible ways you could write this pseudo code, you can bikeshed about what syntax or naming or code patterns you'd prefer all you want, this is possible in basically any one of them. import {Transitioner} from 'react-navigation';
// ...
onSendButtonClicked = idempotentHandler(() => {
return this.props.api('/sendMessage', {message: this.state.message})
.then((data) => {
this.props.navigation.navigate('MessageView', {id: data.id});
return Transitioner.waitForTransitions();
});
});
onSendButtonClicked = idempotentHandler((done) => {
this.props.api('/sendMessage', {message: this.state.message},
(err, data) => {
if ( err ) {
done(err);
return;
}
this.props.navigation.navigate('MessageView', {id: data.id});
Transitioner.runAfterTransitions(done);
});
});
function onSendButtonClicked() {
try {
const lock = takeInteractionLock(this); // this lock is instance-wide
this.props.api('/sendMessage', {message: this.state.message},
(err, data) => {
if ( err ) {
console.error(err);
lock.release();
return;
}
this.props.navigation.navigate('MessageView', {id: data.id});
Transitioner.runAfterTransitions(() => {
lock.release();
});
});
} catch ( e ) {
if ( e.code === 'INTERACTION_IS_LOCKED' ) {
return;
} else {
throw e;
}
}
}
function onSendButtonClicked() {
if ( takeInteractionLock(this, 'send') ) return; // this handler uses a lock category to make it per-handler
this.props.api('/sendMessage', {message: this.state.message},
(err, data) => {
if ( err ) {
releaseInteractionLock(this);
return;
}
this.props.navigation.navigate('MessageView', {id: data.id});
Transitioner.runAfterTransitions(() => {
releaseInteractionLock(this, 'send');
});
});
}
function onSendButtonClicked() {
if ( IdempotentHandler.isLocked(this) ) return;
IdempotentHandler.startInteraction();
this.props.api('/sendMessage', {message: this.state.message},
(err, data) => {
if ( err ) {
IdempotentHandler.endInteraction();
return;
}
this.props.navigation.navigate('MessageView', {id: data.id});
Transitioner.runAfterTransitions(() => {
IdempotentHandler.endInteraction();
});
});
}
// you can also skip a library if you want to write everything verbosely in your own project
function onSendButtonClicked() {
const that = this;
if ( that.pending ) return;
that.pending = true;
that.props.api('/sendMessage', {message: that.state.message},
function(err, data) {
if ( err ) {
that.pending = false;
return;
}
that.props.navigation.navigate('MessageView', {id: data.id});
Transitioner.runAfterTransitions(function() {
that.pending = false;
});
});
}
Files locks are a bit of a red herring, in the second half of my comment when I lumped file IO in with database IO I was talking about overall event handlers. Basic file operations like delete and move happen extremely quickly (<10ms), double submit is a human scale issue that takes longer between event handler triggers (>10ms). By the time the second handler is triggered the delete/move operation has already completed and the file lock is released. The problem is overall event handlers that take time to complete and file operations that are not idempotent. ie: You cannot execute a move operation twice. The first (A->B) move operation moves A to B, the second (A->B) move operation throws an IO error because A does not exist.
This is a fundamental breaking change to or a misunderstanding of what a StackNavigator is. TabRouter is a router that always has 1 route item per tab, you never have any more or less route items than the number of tabs you have. And navigate simply switches the active tab. The only reason TabRouter is idempotent is because all it does is change the active route. StackRouter is not that. A StackRouter is a router that allows you to push any number of route items onto the stack. You always have at least 1, but the number of route items is not fixed to the number of routeNames you have, you can have less than that number or more. And navigate does not The confusing thing here is that react-navigation uses a unified Here's another example of a non-idempotent operation,
Wait callbacks / runAfter callbacks always wait a tick first to allow dispatches to execute and queue up transitions (just like react-native's own runAfterInteractions), they are unaffected by the redux style dispatch logic. |
None of those libraries make event handlers idempotent or provide a way to wait for transitions to complete. |
|
@dantman I know what the StackNavigator does. The solution proposed by #135 is ideal because gives you idempotency and an option to diverge from it if you have a special case where you have to push same route twice. So nothing changes, except for the better. Pushing same screen multiple times in a row is very unusual. And this is the core problem here. We are satisfying this peculiar use case but the more broader use cases are being marginalized. The main goal of this project from the beginning was to be EASY to use. We are diverging a lot from that philosophy by introducing all kinds of shady libraries to handle it and increase the core complexity at the same time. |
|
And goBack() is idempotent except for the use case where you are trying to call it with param null. By default is binding the current route key |
The core problem of double submit is not route pushes, it's the double submit. And while pushing a screen multiple times is unusual, I haven't seen a single person bother to offer an actual use case that is supposedly being marginalized other than double submit. I'd be happy to accept links if you have any, I'm pretty certain that any of those are also a case of something that actually has a bigger issue that should be solved instead of working around it using #135.
There is nothing shady or complex about using a new library. The react-native ecosystem is built on this foundation (react-navigation is just one of the 3rd party libraries you'll be using, even in Expo). We can even put any library we create under the react-community org and allow anyone in the community to help maintain it. I also gave an example of doing this without using a library. The whole point of a library is to make fixing double submit easy by making it so the developer can just slap a decorator, function, or whatever other simple call they please to eliminate the code they need to write to handle their double submits themselves.
Ok, you're correct about goBack. But then
I still think double submit should be fixed by fixing double submit, but now that you bring up how The idempotent stacknavigator.navigate/push #135 appears to be describing is turning it into a command saying " But what if we flipped that around and made stacknavigator.navigate work like
I still think we should offer developers a solution for making it easy for to fix double submit in all use cases, other than just react-navigation. But I wouldn't oppose adding this put + push behaviour in parallel with that. |
|
@dantman finally we agree to somenthing. :) I like a similar solution to goBack for navigation |
|
@rpopovici Yay. What do you think about also creating some sort of double-submit/idempotent event handler/interaction lock library (with multiple ways of using it so it fits into people's coding styles) under react-community and recommending it, at least in examples like we do with redux? Also, if we're going with changing navigate to some sort of putAfter/putAfterExclusive, what do you think about combining that with adding in the various other actions missing from react-navigation (jump*, replace, etc...) and starting an RFC for it? |
|
@dantman many Pub/Sub libraries are available already for JS e.g. https://github.com/facebook/emitter As for locking(as in mutex/semaphores), I don't think it really makes sense to have one since JS is single threaded and anyway, nowadays immutability+async logic it's more trendy e.g. immutable.js, promises and other functional stuff. As for navigation actions, I think we should have a lot more than we have now:
|
I'm not talking about pub/sub, mutexes, semaphores, or threading. locking probably isn't the best name for it, but there isn't any other good name. I'm talking about the simple library to ensure an event handler doesn't get invoked again while it is still doing asynchronous handling (network requests or navigation transitions). Using whatever coding pattern is preferred (promises, callbacks, etc...). I'll put together an RFC and a comment based on what ex-navigation has but react-navigation is missing later. Done too much work today. |
|
@dantman I don't know what ex-navigator does there but it must be either something like a deferred as in jQuery.Deferred, or a debounce as in lodash.debounce. |
|
So @davepack I think you have your answer. The best way to approach this is to do it like goBack does it. You have to bind the key of the current route to the navigation action and fall through if it's not the last in the route stack. Thus there's a problem with this approach when you are trying to use dispatch directly. In that case you have to find another reference point to pass it as a key |
By RFC and ex-navigation I mean I'll make a comment with a list of missing navigation actions that ex-navigation's navigator has but react-navigation does not. Like the various jump and replace methods ex has.
Manual dispatches from a route could just add |
|
@davepack Any updates on this? |
|
I think this is not the best way to accomplish idempotent pushes. Instead, lets have the navigate action accept a |
This fixes #135. This solution attempts to make idempotent pushes automatic by setting nav state from within Transitioner when a transition starts and ends. While
inTransition === true,getStateForActioninStackRouterwill ignoreNAVIGATE,BACK, andRESETactions.Several different solutions have been proposed for this issue, this attempts to be zero work for devs to get it working, nail the transition timing, and also keep logic and flow simple.