#111 Transition hooks not called for redirects changes #119
Conversation
src/stores/TransitionState.ts
Outdated
|
|
||
| private async transition(toState: RouterState): Promise<RouterState> { | ||
| debug('transition from %o to %o)', this.fromState, toState); | ||
| if (valueEqual(this.transitionState, toState)) { |
There was a problem hiding this comment.
What if the loop doesn't involve the final state? E.g. the fromState is A and the toState is B, and A.onExit redirects to C, and C redirects to A.
I think to detect a loop you're going to have to keep track of all the transition states.
There's also the pathological case where a parameter is modified each time to a unique value, so there is never a loop but the recursion is infinite. If we wanted to guard against this we could introduce a maximum number of redirects.
There was a problem hiding this comment.
You're right, that should be changed. I agree with you that a simple counter will be more flexible. I will change it.
| private routerStore: RouterStore, | ||
| private readonly fromState: RouterState | ||
| ) { | ||
| // Create an object copy so we can modify hooks during transition. |
There was a problem hiding this comment.
There's a comment below to an issue where someone is using classes to implement their routes - wondering if this is compatible with that (#74).
There was a problem hiding this comment.
I don't see why not, but @nareshbhatia can confirm us.
src/stores/TransitionState.ts
Outdated
| this.routerStore | ||
| ); | ||
| this.fromRoute.beforeExit = undefined; | ||
| if (redirectState) { |
There was a problem hiding this comment.
For these redirect states, does it make sense to compare to the toState and only transition if they are not equal?
There was a problem hiding this comment.
Sure, that will reduce unnecessary transitions.
|
@dkolarev, changes look good. I did minor cleanup. Thanks for your contribution! |
|
mobx-state-router@6.0.0-beta.2 is published with tag "next". Please test to make sure all is working well. |
Related to the previously discussed issue with the transition hooks, I made several changes to the router implementation.
The
transition()function has been moved from theRouterStoreto the separateTransitionStateclass. A new instance of theTransitionStateobject is created whenever routing occurs.If we preserve a
fromStateduring a transition, calling exit hooks can be problematic and lead to an infinite loop, so we should check that they are executed only once.Still, we have to be aware that because of the recursive implementation of the transition, infinite loops and therefore, stack overflow, can still be caused by a developer. To avoid infinite loops, a simple check has been added and an exception will be thrown whenever an infinite loop is detected.
I have been using the new implementation for a few days on a moderately large project. All tests are passing, the old and new ones.