Angular router canReuse() ignored?

[chalin]

Repro:

• Download https://github.com/dart-lang/site-webdev/archive/5-dev.zip, then
  • Unzip
  • cd to the root of the unzipped project
  • cd examples/ng/doc/router

The crisis component lib/src/crisis/crisis_component_5.dart, has canReuse() returning true: https://github.com/dart-lang/site-webdev/blob/ddf9ff0a8e6d2758d13899827879840bdb8a1f1b/examples/ng/doc/router/lib/src/crisis/crisis_component_5.dart#L90

• pub get
• pub run build_runner serve
• Open http://localhost:8080/#crises (Please ignore the warning about missing styles.css)
• Open the dev tools console
• In the crisis list:
  • Select crisis 1
  • Select crisis 2, and add an x to the start of the crisis name. Then click Save.

The console output should look like this:

[0] CrisisComponent created
[1] CrisisComponent created
>> [1] Activating crises (null) -> crises/1 ...
>> [1] Activating name = Dragon Burning Cities
>> [0] canDeactivate crises/1 -> crises/2; null == null
>> [0] Deactivating crises/1 (null) -> crises/2
[2] CrisisComponent created
>> [2] Activating crises/1 (null) -> crises/2 ...
>> [2] Activating name = Sky Rains Great White Sharks
>> [2]  save xSky Rains Great White Sharks (was Sky Rains Great White Sharks
>> [1] canDeactivate crises/2 -> crises; Dragon Burning Cities == Dragon Burning Cities
>> [1] Deactivating crises/2 (Dragon Burning Cities) -> crises

There are a few issues here actually:

1. CrisisComponent instances don't appear to be reused. As can be seen above, three instances are created.
2. Aside from the fact that reusing isn't happening, why is CrisisComponent [0] created in the first place? Why is it deactivated if it was never activated?

Environment:

• Dart SDK: 2.0.0-dev.40.0
• AngularDart: 5.0.0-alpha+8
• MacOSX

cc @leonsenft @kwalrath

[leonsenft]

First off, extend CanReuse rather than implement. The default implementation already returns true so you don't have to override the method. You only need to override the implementation if you conditionally return true.

The issue is that the parent route of CrisisComponent isn't reusable. If any parent routes aren't reusable, they'll be destroyed, which in turn will destroy the reusable component. It then must be recreated after the parent is recreated.

There's a terrible flaw in the router, where if you navigate to the child of a non-reusable parent that is active, you'll create the child during resolution of the route, then destroy the parent to recreate, meaning you need to recreate the child again.

The entire reuse/caching is done at the router-outlet level, so any time that router-outlet is destroyed, you lose the reuse. It really just seems like this was an oversight in the design, as there were no tests for this behavior until I added them. At this point we can't change that behavior without it requiring a major API refactor.

[leonsenft]

I'll look into why three instances are created, but otherwise this is WAI, or otherwise infeasible to change.

[chalin]

On Fri, Mar 23, 2018 at 6:28 PM, Leon Senft notifications@github.com wrote:

First off, extend CanReuse rather than implement. The default implementation already returns true so you don't have to override the method. You only need to override the implementation if you conditionally return true.

Right, that was my plan (and that's why there is an "extends ... with CanReuse" in comments). I just wanted it all to be explicit for this test.

The issue is that the parent route of CrisisComponent isn't reusable. If any parent routes aren't reusable, they'll be destroyed, which in turn will destroy the reusable component. It then must be recreated after the parent is recreated.

That makes sense. (But I don't recall that we had any such setup before under Angular 4, but I could be misremembering.)

There's a terrible flaw https://github.com/dart-lang/angular/issues/733 in the router, where if you navigate to the child of a non-reusable parent that is active, you'll create the child during resolution of the route, then destroy the parent to recreate, meaning you need to recreate the child again.

The entire reuse/caching is done at the router-outlet level, so any time that router-outlet is destroyed, you lose the reuse. It really just seems like this was an oversight in the design, as there were no tests for this behavior until I added them. At this point we can't change that behavior without it requiring a major API refactor.

Ok, thanks for the reference to the router issue.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/dart-lang/angular/issues/1127#issuecomment-375814891, or mute the thread https://github.com/notifications/unsubscribe-auth/AD8u-Zx5dtGv37i5ulMugUQeD_T2-7-Tks5thXckgaJpZM4S5VQD .

[leonsenft]

To answer 2, as I mentioned above, CrisisListComponent isn't reusable. What happens when you're at /crises/, and you navigate to /crises/1 is this:

1. Match /crises/1 against your root routes.
2. /crises partially matches, so we need to check if this route has any sub-routes that complete the match. In order to do this we need to create the component for this route, to check which routes are bound to its router outlet. Since this route is actually already active, we can check it's router outlet directly.
3. The unmatched portion of the URL, /1, indeed matches a sub-route. Again we need to create this component, CrisisListComponent so we can check if it can be activated, or has any default sub-routes.
4. Since CrisisListComponent doesn't have any default routes to attach, we're done. We've now completed the resolution phase of navigation. Up until this point we were just trying to find a route hierarchy that matched the URL.
5. Now we go to actually activate the route. For each router-outlet, we check if the current component is reusable, and if not, destroy it and replace it with our new one. This is where the issue occurs. Notice that CrisisListComponent is both the active root route, and the root of our resolved routes? Since it's not reusable, we need to destroy and detach it. This in turn destroys the cache in its router-outlet, meaning we lose our cached CrisisComponent that was built during resolution. So after it's rebuilt, we then again have to also rebuild CrisisComponent.

This is exactly why you need to perform any expensive actions in onActivate instead of the constructor or ngOnInit of your routed components. The router may destroy and create your components at will. Yes this is super unfortunate, and we'd prefer it wasn't the case, but this is actually the cost of supporting dynamic route configuration. If route configuration was static, we could resolve routes without needing to create anything, but the current design choose to instead allow binding at the router-outlet instead.

This is both WAI, and a known issue. We can't really fix it without redesigning the router, nor can we redesign it and ask everyone who just migrated to migrate again...

[zoechi]

What about a custom reuse strategy like in TS? That could just be a service that a parent component provides and the router-outlet injects, where previous instances can be cached and reused, and the router-outlet just delegates to this service (if provided) for the reuse decision.

I haven't even tried the TS reuse strategy myself yet and not looked at how it's implemented. Above suggestion is also just a thought I had right now without reasoning a lot about possible problems.

[chalin]

Thanks @zoechi, I'll let @leonsenft comment on your suggestion.

@leonsenft I've created #1133 as a reminder to update the router migration guide.

This is both WAI, and a known issue.

Thanks for the explanation. Closing this issue.

[leonsenft]

That could just be a service that a parent component provides and the router-outlet injects, where previous instances can be cached and reused, and the router-outlet just delegates to this service (if provided) for the reuse decision.

Wouldn't this have the same issue that if the parent component provided the service, and the parent component is destroyed then so is the service?

I could see this strategy working providing the reuse service is provided at the root level. I pondered just building such logic into the router, but it's not without its own issues. See https://github.com/dart-lang/angular/issues/1133#issuecomment-376244664 for details.

[zoechi]

@leonsenft I might not need to be the root level, at least high enough so that the providing component is not destroyed.

[leonsenft]

That still has the same issue though. As long as it's not root, it could be destroyed, meaning you'll inevitably have users with a configuration that hit that case and then wonder why their component isn't reused in oddly specific situations (if they even notice at all).

[zoechi]

Perhaps I didn't really understand when exactly a component is destroyed

[leonsenft]

If you navigate away from a component and it's not reusable it will be destroyed. The caveat is, that reusable components are cached on their router-outlet. If the component that hosts this router-outlet is itself a non-reusable route, then it, it's router-outlet, and the cache will all be destroyed when navigation routes to another component.

If that component instead provided a reuse service, it's exactly the same situation, since the service would be tired to the component's lifecycle as well.