Skip to content
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

Parameterized routes do not cause component and router links to update #10207

Closed
poke opened this issue Jul 21, 2016 · 28 comments
Closed

Parameterized routes do not cause component and router links to update #10207

poke opened this issue Jul 21, 2016 · 28 comments
Labels
area: router feature Issue that requests a new feature freq1: low

Comments

@poke
Copy link

poke commented Jul 21, 2016

When using parameterized routes, changes to the parameter are correctly recognized using the observable on the activated route, but router links that are on the component’s view are not updated. This appears to be a bug.

See a plunker with an example here.

In that example, I have a ContainerComponent at the root which takes an id parameter and displays a child router outlet. The child routes are available as router links on the container component:

template: `
    <p>Container {{id}}</p>
    <a [routerLink]="['foo']" routerLinkActive="active">foo</a>
    <a [routerLink]="['bar']" routerLinkActive="active">bar</a>
    <hr />
    <router-outlet></router-outlet>
`,

Assuming I start at the URL /example1/foo, I can then use the router links in the parent component to switch between ids. For example, I can go to /example2/foo by clicking the “Example 2” link. On that route, the ContainerComponent correctly recognizes the change of the id parameter and updates the view to display that id. The links in the ContainerComponent however still link to /example1/foo and /example1/bar respectively. So the change of the active route to /example2/ was never recognized there.

So it seems that the relative routes do not take route changes into account. They are rendered once at the very beginning. This appears to be because the RouterLink directive only uses the ActivatedRoute that’s injected at construction.

The RouterLinkActive directive works differently by listening to router events and always using the current URL.

The RouterLink directive could obviously be changed to listen to those events too but I personally have a similar problem with my components in my application too. Because the v3 component router actively reuses components, switching between two different routes which happen to use the same components will not cause those components to go through the lifecycle events. As such, initialization that would happen in ngOnInit only happens for the very first route.

To properly update the component, I would have to actually listen to all router events (since you can only listen to all router events) and figure out when the current component would be affected and then perform my own lifecycle—all manually.

This all feels very wrong to me, that I have to take care of so much things and listen to so many low-level events when I just want to use the router by configuration.


  • Angular version: 2.0.0-rc.4
  • Router version: 3.0.0-beta.2
@klewar2
Copy link

klewar2 commented Jul 21, 2016

Hi,
I have the same issue ! The configuration is the same.

+1

@zoechi
Copy link
Contributor

zoechi commented Jul 21, 2016

Please use the add reaction feature on the initial comment instead +1 or me too

@brandonroberts
Copy link
Contributor

The issue with the routerLink not updating has already been fixed in master: http://plnkr.co/edit/liQKV0?p=preview

For the second issue, you don't have to listen to all router events to achieve what you want. Since the parameters, data and url are Observables, you will get new values pushed to them when changes occur. You can subscribe to the ActivatedRoute.url to get updates when the url changes in the component that's being reused.

@poke
Copy link
Author

poke commented Jul 21, 2016

has already been fixed in master

That’s good. Is there any ETA on the next npm release for the router? This is currently a blocking issue for me, and prevents me from updating to the v3 router.

you don't have to listen to all router events

But everywhere in the source, this is what is being done. For example the fix you mention does it like this instead of listening to an event of ActivatedUrl. That being said, I think it’s somewhat weird to listen to either ActivatedRoute.url, ActivatedRoute.params, or ActivatedRoute.data just to get notified of when a route is updated.

That being said, I think that fix was only done for the RouterLinkWithHref directive, not the other RouterLink that applies to non a-tags.

Also, I still think that this is far too complicated. You have to listen to the same router change event all over the place just to fix what Angular itself does not manage to do: maintain an actual lifecycle of a component within the scope of a route. You have to know exactly which component might be reused by the router and fix it up with additional events just to avoid seeing weird side effects.

Is there a way to turn off the component reuse for different routes? To make navigating from /example1/foo to /example2/foo construct a new ContainerComponent in my example and use the normal component lifecycle here?

@brandonroberts
Copy link
Contributor

No ETA on the next npm releasse. If you need the fix now you'd need to use the incremental build repos.

Yes, its being done in other places because those aren't single components. If you want to use the router events, you subscribe and can filter out all events but the NavigationEnd event. Even with the old router, the parent component would have been reused, you were just listening in routerOnReuse instead of the ActivatedRoute.url. In this case your ContainerComponent has no need to be reloaded, since the only thing changing is your params, which you get notified of.

The router doesn't need to maintain a separate lifecycle outside of a component itself. This was one of the issues with the deprecated router as your component couldn't just be a component. There isn't a way to turn of component reuse currently.

@poke
Copy link
Author

poke commented Jul 22, 2016

In this case your ContainerComponent has no need to be reloaded, since the only thing changing is your params, which you get notified of.

It’s not a problem for the ContainerComponent since it’s actively listening to the parameter change anyway. It is already fully aware of the router presence as it gets the parameter value, so it knows that it has to handle changes and can properly handle it.

However, it’s a problem further down the line. Suppose FooComponent in my example requires a service to be injected, so it has a provider set:

@Component({
    template: `<p>Foo</p>`,
    providers: [MyService]
})
export class FooComponent {
    constructor(svc: MyService) {}
}

Now, this component is completely unaware of the router. Following separation of concerns it does not need nor should know in what context it is being used. It just knows that it requires a MyService when its being constructed and everything is configured so that’s the case.

Now, when switching between foo and bar routes, this is no problem. The component gets properly created, a service instance is created and injected. However, when switching between various foo routes, e.g. /example1/foo and /example2/foo not only is the ContainerComponent being reused (as expected), but also the FooComponent (and everything that’s inside). So the FooComponent does not realize it now lives in a completely separate context. It does not know that the route changed, and that it’s just being used because some router decided to.

Although we are in a completely different context (example2 instead of example1), the FooComponent is not able to realize that without actively listening to router events. And even if it did, it wouldn’t be able to receive a new and clean MyService instance. It would be required to somehow reset the service. This would also require the service to be able to do so.

So just because the router is reusing the components, we are adding a huge complexity to the application that all components and dependencies below a reused component need to be aware and capable of possible reuse as well.

@poke
Copy link
Author

poke commented Jul 22, 2016

Note that this is a problem for every component that has a state. For example, if you have a component that has an input element where the user can enter something, then that’s already state and if the component isn’t recreated, something would have to actively reset the state.

See for example this plunker for an example. You can’t tell me that it should be the responsibility of MyDataComponent to listen to router events and reset its state.

@brandonroberts
Copy link
Contributor

In the example provided, no you don't want to reset the state that way. In the old router, the component would be reset through a tree of decisions, one being the parameters changing, which caused a cascading effect. In the new router, reusability is emphasized but I do agree there are instances where you want to force a reset of a route component and its hierarchy.

I think that should be opened as a separate issue instead of debating back and forth over whether you favor reusability over reactivity.

@bennadel
Copy link

I just ran into this also.

@withjam
Copy link

withjam commented Aug 22, 2016

Was there a different ticket opened for this or is this still being looked at? Maybe I've just missed it but I can't find a way to force a route to reload when just the parameters are changing. This seems to be most related ticket.

@poke
Copy link
Author

poke commented Aug 22, 2016

I haven’t created a new issue for this, as I am of the opinion that this is all closely related (the “fix” in RouterActiveLink is just a bad workaround in my opinion). This appears to be a design flaw of the new router, and I decided against updating to it for now since this is a blocking issue for us.

@brandonroberts
Copy link
Contributor

@poke there is a comment from Victor on component reuse status here: #7757 (comment)

@vsavkin vsavkin added feature Issue that requests a new feature freq1: low labels Oct 10, 2016
@DzmitryShylovich
Copy link
Contributor

@poke is this still an issue? can u provide a small repro in plunkr?

@poke
Copy link
Author

poke commented Dec 18, 2016

@DzmitryShylovich Yeah, the situation still hasn’t changed. I’ve updated the Plunker from above to current versions: https://plnkr.co/edit/Hq4QM9gTvecVZ6K7Muhm?p=preview

@DzmitryShylovich
Copy link
Contributor

@poke can u pls add steps to reproduce? there's no ContainerComponent from the issue description

@poke
Copy link
Author

poke commented Dec 18, 2016

@DzmitryShylovich The Plunker is an update from the example I gave in this comment. It’s a more simplified situation than the one I used in the original post.

Basically, within that example, you can switch between different sub-routes which all have the same input field. If you enter something on one route and then switch to another route, the state from the input persists. And there is no way to avoid this outside of making the MyDataComponent aware of its position within the route setup, breaking separation of concerns and making the component not reusable outside of that exact route.

@DzmitryShylovich
Copy link
Contributor

Ok, thx.

all have the same input field.

this is because component is reused as described here https://angular.io/docs/ts/latest/guide/router.html#!#reuse

So u have several options:

  1. subscribe to route.params and reset component state on updates.
  2. u can implement custom reuse strategy and always re-create the component

@DzmitryShylovich
Copy link
Contributor

@poke did u solve your issues? can this be closed?

@poke
Copy link
Author

poke commented Dec 21, 2016

@DzmitryShylovich Oh, sorry I never got back to you about this.

Implementing a custom reuse strategy seems to be what I was looking for here. That way, I can disable the router’s component reusing completely (or even use custom logic to do it selectively) and avoid the problems I have.

I’m not completely happy with it since it still leaves the underlying problem inside the default case (I am still of the opinion that this is an odd default behavior for the router), but at least we have an actual way to get around it now (unlike at 2.0 release). I’m glad that this made it into the release eventually.

The option 1 to subscribe to the router events is not really a solution in my eyes as it still moves the responsibility to reset the state to components that should not have any knowledge about their locality within the router, but I get that this made sense as a solution to the RouterLink problematic as fixed in f65ebec.

But yeah, I’m happy with the custom reuse strategy and this issue can be closed I guess. Thank you for your help!


For those interested or running into the situation themselves: To disable the router reusing, you basically have to implement a custom RouteReuseStrategy. You can use the DefaultRouteReuseStrategy as a base and then just change the shouldReuseRoute to return false whenever you want to disable the route reusing.

Note that you cannot just return false permanently here as this will break the router completely. In cases where curr.routeConfig and future.routeConfig are null, you have to return true instead.

So basically, assuming no custom logic to actually reuse any routes, your implementation would look like this:

export class CustomRouteReuseStrategy implements RouteReuseStrategy {
  shouldDetach(route: ActivatedRouteSnapshot): boolean { return false; }
  store(route: ActivatedRouteSnapshot, detachedTree: DetachedRouteHandle): void {}
  shouldAttach(route: ActivatedRouteSnapshot): boolean { return false; }
  retrieve(route: ActivatedRouteSnapshot): DetachedRouteHandle { return null; }
  shouldReuseRoute(future: ActivatedRouteSnapshot, curr: ActivatedRouteSnapshot): boolean {
    return curr.routeConfig === null && future.routeConfig === null;
  }
}

You can then provide that class as a RouteReuseStrategy using { provide: RouteReuseStrategy, useClass: CustomRouteReuseStrategy } in your NgModule and everything should work just fine.

You can see this in action in this Plunker that builds upon the previously linked example.

@poke poke closed this as completed Dec 21, 2016
@pantonis
Copy link

@poke is there any way to apply this on specific route and not the entire module?

@poke
Copy link
Author

poke commented Apr 20, 2017

@pantonis You can only set the reuse strategy globally, but in the implementation of it, you can check what route you’re navigating to and then decide dynamically whether to use the default reuse behavior, or to not reuse the components. So you can apply this on specific routes only by changing the reuse logic for those specific routes. Unfortunately, this is not something that can be configured with the route configuration.

@pantonis
Copy link

pantonis commented Apr 20, 2017

@poke

You mean in the code you posted above (CustomRouteReuseStrategy) I should hardcode specific components?

@poke
Copy link
Author

poke commented Apr 20, 2017

@pantonis Not components, but routes, yes. You could do this based on their URL for example.

@pantonis
Copy link

pantonis commented Apr 20, 2017

@poke OK can you post a sample of how you would do it. Forgive me but I am a beginner in angular and don't really want to override something so important and do it wrong.

Thanks

@poke
Copy link
Author

poke commented Apr 20, 2017

@pantonis I’ve attempted to create an example, but I’ve encountered a bug (#16192) that makes it a bit difficult. Theoretically, the implementation of shouldReuseRoute could look like this:

shouldReuseRoute(future: ActivatedRouteSnapshot, curr: ActivatedRouteSnapshot): boolean {
    // always reuse null => null route, or the router breaks
    if (curr.routeConfig === null && future.routeConfig === null) {
      return true;
    }

    // never reuse routes with incompatible configurations
    if (future.routeConfig !== curr.routeConfig) {
      return false;
    }

    // handle special routes separately that should not be reused
    if (future.routeConfig.path === 'some/path') {
        return false;
    }
    else if (future.routeConfig.path === 'other/:id') {
        return future.path[1] !== '4';
    }

    // per default, reuse routes with the same configuration that we didn’t handle before
    // (this is the default behavior)
    return true;
}

This example would match the default behavior and reuse all compatible routes, except when navigating to some/path or other/4. This example expects the route configuration to have the routes some/path and other/:id configured.

If the issue I mentioned above turns out to be a bug, and the issue is fixed, I can also provide a plunker to show you how this looks like in practice.

@tramel-woodard
Copy link

tramel-woodard commented Oct 21, 2017

@pantonis and @poke this worked for me:

app.component.ts (setting up method for scroll to top of page)

import { Component, Renderer, OnInit } from '@angular/core';
import { Router } from '@angular/router';
...
export class AppComponent {

    constructor(
        private renderer: Renderer
    ) { }

    ngOnInit() { }

    // on page reload, scroll to top of window
    onDeactivate() {
        this.renderer.setElementProperty(document.body, "scrollTop", 0);
    }

}

app.component.html

< app-navbar >
< router-outlet (deactivate)="onDeactivate()" >
< footer >

NOTE: Added "scroll to top" functionality because it's largely requested and coupled with param change

app.routes.ts (setting up route parameter that will be updated)

...
import { ProductItemComponent } from './components/product-item/product-item.component.ts
...
export const routes: routes = [
...
{
    path: 'product/:id',
    component: ProductItemComponent
}
...
];

product-item.component.ts (where param id subscription is initiated)

import { Component, OnInit } from '@angular/core';
import { Router, ActivatedRoute, Params } from '@angular/router';

import { ProductService } from '../../services/product.service';
import { Item } from '../../models/Item';
...
export class ProductItemComponent implements OnInit {
...
id: string;
item: Item;

constructor(
    public productService: ProductService,
    public route: ActivatedRoute,
    public router: Router
) {
    // reset item object based on params id change
    this.route.params
        .subscribe(params => {
            this.id = params['id'];

            // reuse route, state refreshes, page scrolls to top
            this.router.routeReuseStrategy.shouldReuseRoute = function() {
                return false;
            }

            // get item object
            this.productService.getItem(this.id)
                .subscribe(item => {
                    this.item = item;
                });

            });
        }

    ngOnInit() {
        // get item id from url
        this.id = this.route.snapshotparams['id'];

        // get item object
        this.productService.getItem(this.id)
            .subscribe(item => {
                this.item = item;
            });
        }
    }

NOTE: There are two calls for the item object. The first one (in ngOnInit), is called as the page is constructed. The second one (in constructor) is called as param change is recognized.

@sabernaeeni
Copy link

sabernaeeni commented Jun 17, 2019

@poke Hey I used
`
this._router.routeReuseStrategy.shouldReuseRoute = function(){
return false;
};

this._router.events.subscribe((evt) => {
if (evt instanceof NavigationEnd) {
this._router.navigated = false;
window.scrollTo(0, 0);
}
});
`
in the constructor of one of my components. Now the component and the parent component don't work anymore. I don't get any data from the backend, nothing. Could you please help me with that issue, that would be great.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: router feature Issue that requests a new feature freq1: low
Projects
None yet
Development

No branches or pull requests