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

using setRoot fails to employ the router after the first call #212

Closed
davismj opened this Issue Sep 22, 2015 · 37 comments

Comments

Projects
None yet
@davismj
Member

davismj commented Sep 22, 2015

Aurelia.setRoot('a') // a is called, its router is instantiated properly
Aurelia.setRoot('b')
Aurelia.setRoot('a') // a is called but its router-view is empty
@WisdomCorp

This comment has been minimized.

Show comment
Hide comment
@WisdomCorp

WisdomCorp Oct 16, 2015

Any update on this issue?

WisdomCorp commented Oct 16, 2015

Any update on this issue?

@davismj

This comment has been minimized.

Show comment
Hide comment
@davismj

davismj Oct 22, 2015

Member

@EisenbergEffect @bryanrsmith if someone can point me in the right direction, i'd be glad to take a look

Member

davismj commented Oct 22, 2015

@EisenbergEffect @bryanrsmith if someone can point me in the right direction, i'd be glad to take a look

@EisenbergEffect

This comment has been minimized.

Show comment
Hide comment
@EisenbergEffect

EisenbergEffect Oct 22, 2015

Member

@davismj I'm not sure what the issue could be off the top of my head. Places to look would be the Aurelia type in the Aurelia-framework repo, the CompositionEngine in the Aurelia-templating repo, the Router in the Aurelia-router repo and the RouterView in the Aurelia-templating-router repo. It's probably not in the CompositionEngine though.

Member

EisenbergEffect commented Oct 22, 2015

@davismj I'm not sure what the issue could be off the top of my head. Places to look would be the Aurelia type in the Aurelia-framework repo, the CompositionEngine in the Aurelia-templating repo, the Router in the Aurelia-router repo and the RouterView in the Aurelia-templating-router repo. It's probably not in the CompositionEngine though.

@davismj

This comment has been minimized.

Show comment
Hide comment
@davismj

davismj Nov 11, 2015

Member

So there seems to be a lot of issues in a lot of different places here. If I were to fix it, I would likely disrupt the original design of the router if not entirely break components I'm not aware of. Instead of submitting a pull request, I will describe the situation and highlight the relevant source code.

Problem Overview

The high level is that app.setRoot() behaves as if it is simply hiding one root and displaying another in its place, as there is no deactivation of any existing viewModels, routers, or the history module before activating the new root. However, this is not what is actually happening the entire old root is completely removed from the DOM even though all of the removed components still believe they are actively composed into the DOM.

Relevant Code

The core issue is that, on first activate of a root with a router, the activate() and configRouter() are both called (from the registerViewPort() method) which then calls loadUrl() and adds a navigation instruction to the queue. On the second activate however, neither activate() nor configRouter() are called because both the isActive and isConfigured flags are active.

The next issue is that, assuming the above issue is fixed, the router has no idea that its contents have been removed from the DOM entirely. If the router were to receive a navigation instruction, it would often find that the instruction is currently activated, resulting in an activationStrategy.noChange set within _buildNavigationPlan().

Potential Solutions

  1. The most reasonable solution seems to be to process a full deactivation on the old root before activating the new root.
    Pros: This is consistent, predictable, logical, and maintainable.
    Cons: In place switching actually seems like it might be the desired behavior.
  2. If in place switching is desired, then it might be best to instruct Aurelia to keep a copy of each root-level app hidden in the DOM when setting root.
    Pros: This would work well in the use case where state would need to be preserved between various apps.
    Cons: Does nothing for apps with independent routers, which might be a more common use case, may lead to performance degradation when using multiple complex apps.
  3. Rewrite parts of the Aurelia framework to leverage the router, such that every app has a hidden root level router that sets the root.
    Pros: Consistent with expected behavior since the behavior of the router is well known, reduces complexity of the app.setRoot() since it leverages existing code, introduces all the extensibility of the router.
    Cons: Could be a substantial rewrite with breaking changes.
  4. Another alternative would be to introduce (a) new flag(s) into the application to differentiate between a routing navigation and a setRoot navigation.
    Pros: Has the least impact on the current functionality set of the framework as it would consist entirely of new additions.
    Cons: Hackish, probably time consuming, likely introduces a lot of complexity.
Member

davismj commented Nov 11, 2015

So there seems to be a lot of issues in a lot of different places here. If I were to fix it, I would likely disrupt the original design of the router if not entirely break components I'm not aware of. Instead of submitting a pull request, I will describe the situation and highlight the relevant source code.

Problem Overview

The high level is that app.setRoot() behaves as if it is simply hiding one root and displaying another in its place, as there is no deactivation of any existing viewModels, routers, or the history module before activating the new root. However, this is not what is actually happening the entire old root is completely removed from the DOM even though all of the removed components still believe they are actively composed into the DOM.

Relevant Code

The core issue is that, on first activate of a root with a router, the activate() and configRouter() are both called (from the registerViewPort() method) which then calls loadUrl() and adds a navigation instruction to the queue. On the second activate however, neither activate() nor configRouter() are called because both the isActive and isConfigured flags are active.

The next issue is that, assuming the above issue is fixed, the router has no idea that its contents have been removed from the DOM entirely. If the router were to receive a navigation instruction, it would often find that the instruction is currently activated, resulting in an activationStrategy.noChange set within _buildNavigationPlan().

Potential Solutions

  1. The most reasonable solution seems to be to process a full deactivation on the old root before activating the new root.
    Pros: This is consistent, predictable, logical, and maintainable.
    Cons: In place switching actually seems like it might be the desired behavior.
  2. If in place switching is desired, then it might be best to instruct Aurelia to keep a copy of each root-level app hidden in the DOM when setting root.
    Pros: This would work well in the use case where state would need to be preserved between various apps.
    Cons: Does nothing for apps with independent routers, which might be a more common use case, may lead to performance degradation when using multiple complex apps.
  3. Rewrite parts of the Aurelia framework to leverage the router, such that every app has a hidden root level router that sets the root.
    Pros: Consistent with expected behavior since the behavior of the router is well known, reduces complexity of the app.setRoot() since it leverages existing code, introduces all the extensibility of the router.
    Cons: Could be a substantial rewrite with breaking changes.
  4. Another alternative would be to introduce (a) new flag(s) into the application to differentiate between a routing navigation and a setRoot navigation.
    Pros: Has the least impact on the current functionality set of the framework as it would consist entirely of new additions.
    Cons: Hackish, probably time consuming, likely introduces a lot of complexity.
@EisenbergEffect

This comment has been minimized.

Show comment
Hide comment
@EisenbergEffect

EisenbergEffect Nov 11, 2015

Member

Thanks for looking into this @davismj I don't think we can fix it before next week's beta...but I think it can be fixed without breaking changes after the beta. Some changes in the setRoot to track some things and maybe some internal changes to the router's plugin configuration.

Member

EisenbergEffect commented Nov 11, 2015

Thanks for looking into this @davismj I don't think we can fix it before next week's beta...but I think it can be fixed without breaking changes after the beta. Some changes in the setRoot to track some things and maybe some internal changes to the router's plugin configuration.

@avitex

This comment has been minimized.

Show comment
Hide comment
@avitex

avitex Nov 23, 2015

Looking forward to the fix 👍
My application depends on this functionality.

avitex commented Nov 23, 2015

Looking forward to the fix 👍
My application depends on this functionality.

@wshayes

This comment has been minimized.

Show comment
Hide comment
@wshayes

wshayes Nov 23, 2015

Contributor

My application does too! Several sections of my application need a different root page. Thanks @davismj for digging into it. Hopefully the fix won't take long to sort out.

Contributor

wshayes commented Nov 23, 2015

My application does too! Several sections of my application need a different root page. Thanks @davismj for digging into it. Hopefully the fix won't take long to sort out.

@davismj

This comment has been minimized.

Show comment
Hide comment
@davismj

davismj Nov 24, 2015

Member

@bryanrsmith @EisenbergEffect My application depends on it as well. Perhaps we can get a conversation going about how we want to handle this.

Member

davismj commented Nov 24, 2015

@bryanrsmith @EisenbergEffect My application depends on it as well. Perhaps we can get a conversation going about how we want to handle this.

@WisdomCorp

This comment has been minimized.

Show comment
Hide comment
@WisdomCorp

WisdomCorp Nov 24, 2015

@EisenbergEffect In your channel 9 demo, you are going to main.js to change the root manually to show each demo. If this bug is fixed then all demo can be listed as a child route (as available in skeleton app) and you can just click on different option without resorting to change code manually :)

Now beta is released I hope you can look into it as mentioned by you above. As its a very powerful concept and will add lot of flexibility for us. Thank you

WisdomCorp commented Nov 24, 2015

@EisenbergEffect In your channel 9 demo, you are going to main.js to change the root manually to show each demo. If this bug is fixed then all demo can be listed as a child route (as available in skeleton app) and you can just click on different option without resorting to change code manually :)

Now beta is released I hope you can look into it as mentioned by you above. As its a very powerful concept and will add lot of flexibility for us. Thank you

@banazak

This comment has been minimized.

Show comment
Hide comment
@banazak

banazak Dec 10, 2015

Hi @EisenbergEffect Any update on this?

banazak commented Dec 10, 2015

Hi @EisenbergEffect Any update on this?

@davismj

This comment has been minimized.

Show comment
Hide comment
@davismj

davismj Dec 14, 2015

Member

Bump.

Member

davismj commented Dec 14, 2015

Bump.

@32graham

This comment has been minimized.

Show comment
Hide comment
@32graham

32graham Dec 17, 2015

I'm seeing this too. Using Polymer's <paper-drawer-panel> with a <router-view> as the main content to host our Aurelia app. There is no easy way to completely get rid of the drawer for login, so I thought I could use auerlia.setRoot("./src/accounts/user-login") to get rid of the navigation stuff. Works great until I log out and log back in.

32graham commented Dec 17, 2015

I'm seeing this too. Using Polymer's <paper-drawer-panel> with a <router-view> as the main content to host our Aurelia app. There is no easy way to completely get rid of the drawer for login, so I thought I could use auerlia.setRoot("./src/accounts/user-login") to get rid of the navigation stuff. Works great until I log out and log back in.

@EisenbergEffect

This comment has been minimized.

Show comment
Hide comment
@EisenbergEffect

EisenbergEffect Dec 29, 2015

Member

Can someone put together for me an absolutely simplest possible repro? @davismj If you can send me some src files that I can drop into the nav skele to replicate the problem, I can work on this this week myself.

Member

EisenbergEffect commented Dec 29, 2015

Can someone put together for me an absolutely simplest possible repro? @davismj If you can send me some src files that I can drop into the nav skele to replicate the problem, I can work on this this week myself.

@davismj

This comment has been minimized.

Show comment
Hide comment
@davismj

davismj Dec 29, 2015

Member

Take a look at http://davismj.github.io/sentry/. Source at http://github.com/davismj/sentry/

Edit
It's missing a router. But it's 75% of the way there. Want me to add a branch with a router? It should be fairly easier to copy and paste from skel nav into app.js of sentry.

Member

davismj commented Dec 29, 2015

Take a look at http://davismj.github.io/sentry/. Source at http://github.com/davismj/sentry/

Edit
It's missing a router. But it's 75% of the way there. Want me to add a branch with a router? It should be fairly easier to copy and paste from skel nav into app.js of sentry.

@EisenbergEffect

This comment has been minimized.

Show comment
Hide comment
@EisenbergEffect

EisenbergEffect Dec 29, 2015

Member

I want something really simple. Should be possible with four components: two roots, and two routed to pages, correct?

Member

EisenbergEffect commented Dec 29, 2015

I want something really simple. Should be possible with four components: two roots, and two routed to pages, correct?

@davismj

This comment has been minimized.

Show comment
Hide comment
@davismj

davismj Dec 29, 2015

Member

You only need one route on one of the roots. Yes.

On Tue, Dec 29, 2015 at 5:28 PM, Rob Eisenberg notifications@github.com
wrote:

I want something really simple. Should be possible with four components:
two roots, and two routed to pages, correct?


Reply to this email directly or view it on GitHub
#212 (comment).

Member

davismj commented Dec 29, 2015

You only need one route on one of the roots. Yes.

On Tue, Dec 29, 2015 at 5:28 PM, Rob Eisenberg notifications@github.com
wrote:

I want something really simple. Should be possible with four components:
two roots, and two routed to pages, correct?


Reply to this email directly or view it on GitHub
#212 (comment).

@EisenbergEffect

This comment has been minimized.

Show comment
Hide comment
@EisenbergEffect

EisenbergEffect Dec 30, 2015

Member

I believe I can make this work without too much trouble. Would it be acceptable to require the following code?

this.router.deactivate();
this.router.reset();
this.aurelia.setRoot('app2');

Where this.router references the AppRouter.
Let me know.

Member

EisenbergEffect commented Dec 30, 2015

I believe I can make this work without too much trouble. Would it be acceptable to require the following code?

this.router.deactivate();
this.router.reset();
this.aurelia.setRoot('app2');

Where this.router references the AppRouter.
Let me know.

@davismj

This comment has been minimized.

Show comment
Hide comment
@davismj

davismj Dec 30, 2015

Member

Routers are set on viewModels in the configureRouter function. Since they are intrinsic to viewModels, shouldn't they be automatically deactivated when the viewModel is deactivated?

Member

davismj commented Dec 30, 2015

Routers are set on viewModels in the configureRouter function. Since they are intrinsic to viewModels, shouldn't they be automatically deactivated when the viewModel is deactivated?

@EisenbergEffect

This comment has been minimized.

Show comment
Hide comment
@EisenbergEffect

EisenbergEffect Dec 30, 2015

Member

Router deactivation basically turns off routing, so no, I wouldn't think so.

Member

EisenbergEffect commented Dec 30, 2015

Router deactivation basically turns off routing, so no, I wouldn't think so.

@EisenbergEffect

This comment has been minimized.

Show comment
Hide comment
@EisenbergEffect

EisenbergEffect Dec 30, 2015

Member

Also, the view model doesn't need to retain a reference to the router. The routing system makes no assumption that there is a router property.

Member

EisenbergEffect commented Dec 30, 2015

Also, the view model doesn't need to retain a reference to the router. The routing system makes no assumption that there is a router property.

@schatekar

This comment has been minimized.

Show comment
Hide comment
@schatekar

schatekar Jan 3, 2016

@EisenbergEffect Is this issue still being fixed or can I use the following code from one of your previous comments?

this.router.deactivate();
this.router.reset();
this.aurelia.setRoot('app2');

schatekar commented Jan 3, 2016

@EisenbergEffect Is this issue still being fixed or can I use the following code from one of your previous comments?

this.router.deactivate();
this.router.reset();
this.aurelia.setRoot('app2');
@EisenbergEffect

This comment has been minimized.

Show comment
Hide comment
@EisenbergEffect

EisenbergEffect Jan 3, 2016

Member

It hasn't been fixed because the reset method doesn't exist. We think we can also remove the need to write the first two lines of code so only a call to setRoot is needed. It shouldn't take long to fix and we could get it into this week's release, but I didn't really get any feedback from other people here. I was waiting a bit to see if there were any other comments.

Member

EisenbergEffect commented Jan 3, 2016

It hasn't been fixed because the reset method doesn't exist. We think we can also remove the need to write the first two lines of code so only a call to setRoot is needed. It shouldn't take long to fix and we could get it into this week's release, but I didn't really get any feedback from other people here. I was waiting a bit to see if there were any other comments.

@schatekar

This comment has been minimized.

Show comment
Hide comment
@schatekar

schatekar Jan 3, 2016

It would be great if we can get it down to 1 line. I am anxiously waiting for this fix.

A related question - In my use case, I have a separate router for app2. Would calling setRoot('app2') also navigate to default route automatically or I would need to call navigateToRoute myself?

schatekar commented Jan 3, 2016

It would be great if we can get it down to 1 line. I am anxiously waiting for this fix.

A related question - In my use case, I have a separate router for app2. Would calling setRoot('app2') also navigate to default route automatically or I would need to call navigateToRoute myself?

@davismj

This comment has been minimized.

Show comment
Hide comment
@davismj

davismj Jan 3, 2016

Member

There isn't technically a default route. If you mean the '/' route, if you
navigate while at that route, it should activate that route for both roots.

If you have 'mapUnknownRoute' to your '/' route, that would cover almost
every case.
On Jan 3, 2016 07:12, "Suhas Chatekar" notifications@github.com wrote:

It would be great if we can get it down to 1 line. I am anxiously waiting
for this fix.

A related question - In my use case, I have a separate router for app2.
Would calling setRoot('app2') also navigate to default route
automatically or I would need to call navigateToRoute myself?


Reply to this email directly or view it on GitHub
#212 (comment).

Member

davismj commented Jan 3, 2016

There isn't technically a default route. If you mean the '/' route, if you
navigate while at that route, it should activate that route for both roots.

If you have 'mapUnknownRoute' to your '/' route, that would cover almost
every case.
On Jan 3, 2016 07:12, "Suhas Chatekar" notifications@github.com wrote:

It would be great if we can get it down to 1 line. I am anxiously waiting
for this fix.

A related question - In my use case, I have a separate router for app2.
Would calling setRoot('app2') also navigate to default route
automatically or I would need to call navigateToRoute myself?


Reply to this email directly or view it on GitHub
#212 (comment).

@32graham

This comment has been minimized.

Show comment
Hide comment
@32graham

32graham Jan 5, 2016

this.router.deactivate();
this.router.reset();
this.aurelia.setRoot('app2');

Seems acceptable to me @EisenbergEffect. For my use case I would want to use all 3 lines. It seems possible however that someone might not need/want the first two lines, so I think this is a good solution.

32graham commented Jan 5, 2016

this.router.deactivate();
this.router.reset();
this.aurelia.setRoot('app2');

Seems acceptable to me @EisenbergEffect. For my use case I would want to use all 3 lines. It seems possible however that someone might not need/want the first two lines, so I think this is a good solution.

EisenbergEffect added a commit to aurelia/router that referenced this issue Jan 5, 2016

@EisenbergEffect

This comment has been minimized.

Show comment
Hide comment
@EisenbergEffect

EisenbergEffect Jan 5, 2016

Member

The next set of patch releases, hopefully coming this week, will include this fix. There's no need to call deactivate or reset. Successive calls to setRoot will detect the presence of the router on the previous root view model and deactivate/reset it automatically.

Member

EisenbergEffect commented Jan 5, 2016

The next set of patch releases, hopefully coming this week, will include this fix. There's no need to call deactivate or reset. Successive calls to setRoot will detect the presence of the router on the previous root view model and deactivate/reset it automatically.

@davismj

This comment has been minimized.

Show comment
Hide comment
@davismj

davismj Jan 5, 2016

Member

@EisenbergEffect You're a rockstar. 🎸

Member

davismj commented Jan 5, 2016

@EisenbergEffect You're a rockstar. 🎸

@EisenbergEffect

This comment has been minimized.

Show comment
Hide comment
@EisenbergEffect

EisenbergEffect Jan 5, 2016

Member

Really sorry this took so long.

Member

EisenbergEffect commented Jan 5, 2016

Really sorry this took so long.

@schatekar

This comment has been minimized.

Show comment
Hide comment
@schatekar

schatekar Jan 6, 2016

Thanks @EisenbergEffect

It is things like these that made me fall in love with Aurelia instantly.

schatekar commented Jan 6, 2016

Thanks @EisenbergEffect

It is things like these that made me fall in love with Aurelia instantly.

@vcardins

This comment has been minimized.

Show comment
Hide comment
@vcardins

vcardins Mar 2, 2016

This issue seems to be happening again. Just having a similar situation after the latest update. I'll dig into and comment here.

vcardins commented Mar 2, 2016

This issue seems to be happening again. Just having a similar situation after the latest update. I'll dig into and comment here.

@davismj

This comment has been minimized.

Show comment
Hide comment
@davismj

davismj Mar 3, 2016

Member

Any further info, @vcardins ?

Member

davismj commented Mar 3, 2016

Any further info, @vcardins ?

@vcardins

This comment has been minimized.

Show comment
Hide comment
@vcardins

vcardins Mar 4, 2016

I'm putting in place a demo project as suggested by Rob, I should upload in a day or two.

vcardins commented Mar 4, 2016

I'm putting in place a demo project as suggested by Rob, I should upload in a day or two.

@cehoffman

This comment has been minimized.

Show comment
Hide comment
@cehoffman

cehoffman Mar 6, 2016

@vcardins I noticed setRoot wasn't reinitializing the router and the root cause came down to me not saving the router instance from configureRouter for the first setRoot module as this.router. Once I saved the instance in that property, the changes in 0f83b9b kicked in.

cehoffman commented Mar 6, 2016

@vcardins I noticed setRoot wasn't reinitializing the router and the root cause came down to me not saving the router instance from configureRouter for the first setRoot module as this.router. Once I saved the instance in that property, the changes in 0f83b9b kicked in.

@vcardins

This comment has been minimized.

Show comment
Hide comment
@vcardins

vcardins Mar 10, 2016

I out in place a small repo, based on skeleton-es2016 to reproduce my issue https://github.com/vcardins/skeleton-es2016-multiple-root.
(For some reason the AuthorizeStep is not working, so I redirected manually)
Steps ...

  1. main.js
    aurelia.start().then(() => aurelia.setRoot('bootstrapper'));
  2. bootstrapper.js
    2.1 Configure the routes: this.appRouterConfig.configure();
    2.2 Check's whether the user is logged in and set the proper root: this.aurelia.setRoot(root)
  3. login.js:
    After logging in, it sets again the root:
    this.authService.authenticate(this.username, this.password).then((response)=>{ self.aurelia.setRoot('app').then(() => { //self.router.navigate('', {}); }); }) .catch(err=>{ console.error(err.message); }); }
  4. logout.js
    this.authService.logout().then(response=>{ self.aurelia.setRoot('public').then(() => { //self.router.navigate('login', {}); }); });

vcardins commented Mar 10, 2016

I out in place a small repo, based on skeleton-es2016 to reproduce my issue https://github.com/vcardins/skeleton-es2016-multiple-root.
(For some reason the AuthorizeStep is not working, so I redirected manually)
Steps ...

  1. main.js
    aurelia.start().then(() => aurelia.setRoot('bootstrapper'));
  2. bootstrapper.js
    2.1 Configure the routes: this.appRouterConfig.configure();
    2.2 Check's whether the user is logged in and set the proper root: this.aurelia.setRoot(root)
  3. login.js:
    After logging in, it sets again the root:
    this.authService.authenticate(this.username, this.password).then((response)=>{ self.aurelia.setRoot('app').then(() => { //self.router.navigate('', {}); }); }) .catch(err=>{ console.error(err.message); }); }
  4. logout.js
    this.authService.logout().then(response=>{ self.aurelia.setRoot('public').then(() => { //self.router.navigate('login', {}); }); });
@vcardins

This comment has been minimized.

Show comment
Hide comment
@vcardins

vcardins Mar 10, 2016

@davismj Please, see my previous post. Thank you !!

vcardins commented Mar 10, 2016

@davismj Please, see my previous post. Thank you !!

@fabioluz

This comment has been minimized.

Show comment
Hide comment
@fabioluz

fabioluz Apr 11, 2016

Contributor

@cehoffman I had this same problem today. When I read your comment I realised that I was not saving the router instance in the login root component. Everything worked after saving the router instance in the this.router property. Thank you!

Contributor

fabioluz commented Apr 11, 2016

@cehoffman I had this same problem today. When I read your comment I realised that I was not saving the router instance in the login root component. Everything worked after saving the router instance in the this.router property. Thank you!

@HIRANO-Satoshi

This comment has been minimized.

Show comment
Hide comment
@HIRANO-Satoshi

HIRANO-Satoshi May 21, 2016

Thank you very much, @davismj

HIRANO-Satoshi commented May 21, 2016

Thank you very much, @davismj

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment