fix(router): cache route handle if found #22475

sliekens · 2018-02-27T14:03:30Z

When asking the route reuse strategy to retrieve a detached route handle, store the
return value in a local variable for further processing instead of asking again later.

resolves #22474

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #22474

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

googlebot · 2018-02-27T14:03:33Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

googlebot · 2018-02-27T14:05:34Z

CLAs look good, thanks!

systemlord01 · 2018-02-27T14:27:30Z

best pull request ever

jasonaden

Just a quick review for now. Can you please provide a test? You should be able to run the router tests using:

./test.sh router

This will set up a watcher, and you can go ahead and add a test to create_router_state.spec.ts.

Overall looks like a great change. Thanks!

sliekens · 2018-03-07T09:00:08Z

Hello @jasonaden, what should the test do? Assert that RouteReuseStrategy.retrieve only gets called once when creating the router state?

Overall looks like a great change. Thanks!

Thanks 😊

sliekens · 2018-03-19T16:31:29Z

@jasonaden I'm really not sure what the test should do?

My commit removes an unnecessary call to the retrieve function. Should I write a test that asserts that the function is called only once?

sliekens · 2018-03-26T11:16:51Z

@jasonaden I'm looking at the create_router_state.spec.ts file and I have no idea of what's going on here.

The spec file itself contains 5 helper functions that are used in almost every test. I don't know what they do but they look so complicated that it makes me wonder why they aren't under test.

I'm gonna need some pointers if you want me to write a test for this change. Basically all my commit does is reorder some brackets so that the retrieve function doesn't need to be called twice in the same code path. What should my test do and which functions do I need to use?

When asking the route reuse strategy to retrieve a detached route handle, store the return value in a local variable for further processing instead of asking again later. resolves #22474

sliekens · 2018-04-03T16:00:18Z

@jasonaden I spent about 2 hours debugging and looking through the sources. I have a pretty good idea now of how createRouterState works. I still can't write a test for this code for two reasons:

I'm still not sure what the test should assert exactly
This particular code path is only used if there is a DetachedRouteHandle to retrieve. I tried mocking a route handle but that turns out to be pretty difficult.

What I tried:

  it('should retrieve detached route handles', () => {
    const config = [
      {path: 'a', component: ComponentA},
      {path: 'b', component: ComponentB, outlet: 'left'},
      {path: 'c', component: ComponentC, outlet: 'left'}
    ];

    const fakeDetachedHandle: DetachedRouteHandle = {};
    spyOn(reuseStrategy, 'retrieve').and.callFake((route: ActivatedRouteSnapshot) => {
      return fakeDetachedHandle;
    });

    let routerState = emptyState();
    let snapshot1 = createState(config, 'a(left:b)');
    let snapshot2 = createState(config, 'a(left:c)');

    routerState = createRouterState(reuseStrategy, snapshot1, routerState);
    advanceState(routerState);
    routerState = createRouterState(reuseStrategy, snapshot2, routerState);

    expect(reuseStrategy.retrieve).toHaveBeenCalledTimes(1);
  });

This crashes because createNode assumes that my fakeDetachedHandle contains a tree node and probably some other stuff.

jasonaden · 2018-04-20T21:40:51Z

No problem. I created a test for this and I'll push it to your branch.

ngbot · 2018-04-20T21:47:58Z

I see that you just added the PR action: merge label, but the following checks are still failing:
     missing required label: "PR target: *"
     missing required status "code-review/pullapprove"
     status "ci/circleci: build" is pending
     status "google3" is pending
     status "continuous-integration/travis-ci/pr" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

jasonaden · 2018-04-20T21:51:03Z

Presubmit

sliekens · 2018-04-21T10:17:27Z

packages/router/test/create_router_state.spec.ts

+      {path: 'a', component: ComponentA}, {path: 'b', component: ComponentB, outlet: 'left'},
+      {path: 'c', component: ComponentC, outlet: 'left'}
+    ];
+    spyOn(reuseStrategy, 'retrieve').and.callThrough();


Won't that return null or undefined? Because then it skips the code path that I changed. (see line 36)

Sorry @StevenLiekens, I've been out for a little bit.

The real test here is line 110 below, where we verify that after the activated route has been created, the previously created version gets used the next time reuseStrategy.retrieve gets called.

@jasonaden but how? The DefaultRouteReuseStrategy doesn't ever retrieve anything.

retrieve(route: ActivatedRouteSnapshot): DetachedRouteHandle|null { return null; }
https://github.com/angular/angular/blob/master/packages/router/src/route_reuse_strategy.ts#L63-L73

The code path that I changed only runs when retrieve returns a truthy value so this test would pass without my fix.

When asking the route reuse strategy to retrieve a detached route handle, store the return value in a local variable for further processing instead of asking again later. resolves #22474 PR Close #22475

When asking the route reuse strategy to retrieve a detached route handle, store the return value in a local variable for further processing instead of asking again later. resolves angular#22474 PR Close angular#22475

thorn0 · 2018-11-27T05:00:51Z

Can somebody tell me why this retrieve call is needed there in the first place? Isn't retrieve supposed to be called only after shouldAttach returns true?

sliekens · 2018-11-27T07:40:11Z

Retrieve is called once to determine if there is a component to attach. Then shouldAttach is called to determine if the component should be attached or destroyed. The real question is why retrieve is called again after that.
/edit ^ false assumptions ^

thorn0 · 2018-11-27T08:27:40Z

Then shouldAttach is called to determine if the component should be attached or destroyed

My experiments show that shouldAttach is always called regardless of what the first retrieve call returned. Do you think this is a bug?

Wouldn't this sequence make more sense?

// pseudocode
if (shouldDetach()) store();
else destroy();
if (shouldAttach()) retrieve();
else create();

It seems to work like this now, except that it additionaly calls retrieve in the very begining. Overall, this route reuse strategy API feels super confusing if not broken.

sliekens · 2018-11-27T09:00:18Z

Makes more sense to me, yes.

It seems to work like this now

Well did they change it since I opened this issue? I haven't been following this very closely.

/edit: no, they haven't changed this: if shouldReuseRoute returns false then the very next thing that happens is a call to retrieve()

/edit2 apparently all this does is update the _futureSnapshot of the retrieved route and all of its children.

angular/packages/router/src/create_router_state.ts

Lines 32 to 39 in f45aedc

    
             // retrieve an activated route that is used to be displayed, but is not currently displayed 
        
           } else { 
        
             const detachedRouteHandle = 
        
                 <DetachedRouteHandleInternal>routeReuseStrategy.retrieve(curr.value); 
        
             if (detachedRouteHandle) { 
        
               const tree: TreeNode<ActivatedRoute> = detachedRouteHandle.route; 
        
               setFutureSnapshotsOfActivatedRoutes(curr, tree); 
        
               return tree;

The actual activation of the detached routes happens elsewhere.

angular/packages/router/src/operators/activate_routes.ts

Lines 159 to 191 in 9523991

    
           } else { 
        
             if (future.component) { 
        
               // if we have a normal route, we need to place the component into the outlet and recurse. 
        
               const context = parentContexts.getOrCreateContext(future.outlet); 
        
               if (this.routeReuseStrategy.shouldAttach(future.snapshot)) { 
        
                 const stored = 
        
                     (<DetachedRouteHandleInternal>this.routeReuseStrategy.retrieve(future.snapshot)); 
        
                 this.routeReuseStrategy.store(future.snapshot, null); 
        
                 context.children.onOutletReAttached(stored.contexts); 
        
                 context.attachRef = stored.componentRef; 
        
                 context.route = stored.route.value; 
        
                 if (context.outlet) { 
        
                   // Attach right away when the outlet has already been instantiated 
        
                   // Otherwise attach from `RouterOutlet.ngOnInit` when it is instantiated 
        
                   context.outlet.attach(stored.componentRef, stored.route.value); 
        
                 } 
        
                 advanceActivatedRouteNodeAndItsChildren(stored.route); 
        
               } else { 
        
                 const config = parentLoadedConfig(future.snapshot); 
        
                 const cmpFactoryResolver = config ? config.module.componentFactoryResolver : null; 
        
                 context.attachRef = null; 
        
                 context.route = future; 
        
                 context.resolver = cmpFactoryResolver; 
        
                 if (context.outlet) { 
        
                   // Activate the outlet when it has already been instantiated 
        
                   // Otherwise it will get activated from its `ngOnInit` when instantiated 
        
                   context.outlet.activateWith(future, cmpFactoryResolver); 
        
                 } 
        
                 this.activateChildRoutes(futureNode, null, context.children); 
        
               }

thorn0 · 2018-11-27T14:05:58Z

/edit2 apparently all this does is update the _futureSnapshot of the retrieved route

Not sure why this is needed. I ignore those retrieve calls (by checking that shouldAttach should be called first) and nothing seems to break. I have to ignore those calls because I use retrieve to implement something like #25521 (ngOnAttach/ngOnDetach). Otherwise ngOnAttach is called too early and twice.

denu5 · 2018-12-15T16:54:51Z

@thorn0 how do you check if shouldAttach is called first? trying to do exactly the same thing as you with ngOnAttach/Detach!

thorn0 · 2018-12-17T12:24:21Z

@denu5 see https://stackblitz.com/github/thorn0/angular-router-experiments

thorn0 · 2018-12-17T15:23:31Z

@denu5 Please keep in mind that that code is just experiments. In no way is it production ready. E.g. I've just discovered that reused components sometimes don't get fresh resolved data (route.data) even though the resolvers are called. ~~No idea how to fix this.~~ Fix: thorn0/angular-router-experiments@6941f45

angular-automatic-lock-bot · 2019-09-14T10:52:53Z

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.}

googlebot added the cla: no label Feb 27, 2018

googlebot added cla: yes and removed cla: no labels Feb 27, 2018

alexeagle added the area: router label Feb 27, 2018

jasonaden suggested changes Mar 7, 2018

View reviewed changes

fix(router): cache route handle if found

b84c446

When asking the route reuse strategy to retrieve a detached route handle, store the return value in a local variable for further processing instead of asking again later. resolves #22474

fixup! fix(router): cache route handle if found

e6aed2b

jasonaden approved these changes Apr 20, 2018

View reviewed changes

jasonaden added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Apr 20, 2018

sliekens commented Apr 21, 2018

View reviewed changes

vicb closed this in 4cfa571 Apr 23, 2018

thorn0 mentioned this pull request Nov 27, 2018

Refine RouteReuseStrategy #27290

Open

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(router): cache route handle if found #22475

fix(router): cache route handle if found #22475

sliekens commented Feb 27, 2018

googlebot commented Feb 27, 2018

googlebot commented Feb 27, 2018

systemlord01 commented Feb 27, 2018

jasonaden left a comment

sliekens commented Mar 7, 2018 •

edited

Loading

sliekens commented Mar 19, 2018

sliekens commented Mar 26, 2018 •

edited

Loading

sliekens commented Apr 3, 2018 •

edited

Loading

jasonaden commented Apr 20, 2018

ngbot bot commented Apr 20, 2018

jasonaden commented Apr 20, 2018

sliekens Apr 21, 2018 •

edited

Loading

jasonaden May 2, 2018

sliekens May 4, 2018 •

edited

Loading

thorn0 commented Nov 27, 2018

sliekens commented Nov 27, 2018 •

edited

Loading

thorn0 commented Nov 27, 2018 •

edited

Loading

sliekens commented Nov 27, 2018 •

edited

Loading

thorn0 commented Nov 27, 2018

denu5 commented Dec 15, 2018 •

edited

Loading

thorn0 commented Dec 17, 2018

thorn0 commented Dec 17, 2018 •

edited

Loading

angular-automatic-lock-bot bot commented Sep 14, 2019

fix(router): cache route handle if found #22475

fix(router): cache route handle if found #22475

Conversation

sliekens commented Feb 27, 2018

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

googlebot commented Feb 27, 2018

googlebot commented Feb 27, 2018

systemlord01 commented Feb 27, 2018

jasonaden left a comment

Choose a reason for hiding this comment

sliekens commented Mar 7, 2018 • edited Loading

sliekens commented Mar 19, 2018

sliekens commented Mar 26, 2018 • edited Loading

sliekens commented Apr 3, 2018 • edited Loading

jasonaden commented Apr 20, 2018

ngbot bot commented Apr 20, 2018

jasonaden commented Apr 20, 2018

sliekens Apr 21, 2018 • edited Loading

Choose a reason for hiding this comment

jasonaden May 2, 2018

Choose a reason for hiding this comment

sliekens May 4, 2018 • edited Loading

Choose a reason for hiding this comment

thorn0 commented Nov 27, 2018

sliekens commented Nov 27, 2018 • edited Loading

thorn0 commented Nov 27, 2018 • edited Loading

sliekens commented Nov 27, 2018 • edited Loading

thorn0 commented Nov 27, 2018

denu5 commented Dec 15, 2018 • edited Loading

thorn0 commented Dec 17, 2018

thorn0 commented Dec 17, 2018 • edited Loading

angular-automatic-lock-bot bot commented Sep 14, 2019

sliekens commented Mar 7, 2018 •

edited

Loading

sliekens commented Mar 26, 2018 •

edited

Loading

sliekens commented Apr 3, 2018 •

edited

Loading

sliekens Apr 21, 2018 •

edited

Loading

sliekens May 4, 2018 •

edited

Loading

sliekens commented Nov 27, 2018 •

edited

Loading

thorn0 commented Nov 27, 2018 •

edited

Loading

sliekens commented Nov 27, 2018 •

edited

Loading

denu5 commented Dec 15, 2018 •

edited

Loading

thorn0 commented Dec 17, 2018 •

edited

Loading