Skip to content

Commit

Permalink
deep into child router after actions (fix for #110)
Browse files Browse the repository at this point in the history
  • Loading branch information
Pavlo Aksonov authored and Pavlo Aksonov committed Jan 13, 2016
1 parent 576ae2e commit bb6b965
Showing 1 changed file with 7 additions and 6 deletions.
13 changes: 7 additions & 6 deletions Actions.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,6 @@ class Actions {
let router: Router = this.currentRouter;

debug("Route to "+name+" current router="+this.currentRouter.name+ " current route="+this.currentRouter.currentRoute.name);
// deep into child router

while (router.currentRoute.childRouter){
router = router.currentRoute.childRouter;
debug("Switching to child router="+router.name);
}
while (!router.routes[name]){
const route = router.parentRoute;
if (!route || !route.parent){
Expand All @@ -58,6 +52,13 @@ class Actions {
debug("Switching to router="+router.name);
}
if (router.route(name, props)){

// deep into child router
while (router.currentRoute.childRouter){
router = router.currentRoute.childRouter;
debug("Switching to child router="+router.name);
}

this.currentRouter = router;
return true;
}
Expand Down

8 comments on commit bb6b965

@zebulgar
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't dug into this much but this breaks subrouters for me

@mangogogos
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 nested routers break

getting error:
Cannot find router for route="xxx" current router=undefined

@aksonov
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

combined both ways (before and after), please check now. We really need to add unit tests, who could do it, it will be very helpful...

@zebulgar
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good and yes I can look into some simple unit tests on the example project

@zebulgar
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll check out the fix once you publish to npm

@aksonov
Copy link
Owner

@aksonov aksonov commented on bb6b965 Jan 13, 2016 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zebulgar
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nested routers are no longer an issue, but #110 appears to still be an issue

@mangogogos
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im still running into issues with the 'deep into child router' in both places. For me the use case is:

<Router>
  <Route name="splashScreen"  .../>
  <Route name="loginContainer" ...>
    <Router>
      ...Login Routes
    </Router>
  </Route>
  <Route name="appContainer" ...>
    <Router>
      ...App Routes
    </Router>
  </Route>
</Router>

Consider the normal use case of an end user:
Open app, see splash screen, see login scene, type credentials and login, see dashboard, logout, see login scene. If I were to attempt to do any routing from then on (such as navigating to the create account scene which is also a Login Route) the app will crash. The issue is that at the end of the second 'deep into child router' block after the last transition (going back to the login scene) the currentRouter points to the FIRST LoginContainer router whereas it should point to the top level router as far as I can tell. The reason I emphasize first is that since I first mounted the login scene, then transitioned to the app container and the dashboard app scene, the first LoginContainer has been unmounted and so any navigator refs it has are garbage.

For me simply commenting out the second block is enough to fix the issue and since it apparently doesn't fix #110 according to @MITDelian it may be worth removing it altogether; however, in future modifications please consider the above use case, thanks!

Please sign in to comment.