fix(router): Only retrieve stored route when reuse strategy indicates it should reattach #30263

dimakuba · 2019-05-04T18:33:32Z

When creating the router state, the RouteReuseStrategy#retrieve should
only be called when RouteReuseStrategy#shouldAttach returns true.
That is, we should only retrieve a stored route when the reuse strategy
indicates that there is one stored and that it should be reattached.

This now matches the behavior in the route activation:

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

Lines 170 to 172 in 1d12c50

    
           if (this.routeReuseStrategy.shouldAttach(future.snapshot)) { 
        
             const stored = 
        
                 (<DetachedRouteHandleInternal>this.routeReuseStrategy.retrieve(future.snapshot));

Fixes #23162

googlebot · 2019-05-10T18:44:24Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

atscott

Please update the commit message to describe what prompted this change. It doesn't appear to be just a "refactor" as it would change whether retrieve were called. Also, would this resolve #23162? If so, can you please add that as a note in the commit message as well?

packages/router/src/create_router_state.ts

packages/router/test/create_router_state.spec.ts

atscott

Approving but since I went ahead and applied fixes to this PR it should be reviewed by someone else as well.

atscott · 2021-04-13T21:56:57Z

presubmit

AndrewKushnir

The changes look good. I'm wondering if it's possible (and worthwhile) to add a test that would reproduce the original problem? It looks like the tests were updated to reflect the new behavior, should we also check that the retrieve function is never called in these scenarios?

… it should reattach When creating the router state, the `RouteReuseStrategy#retrieve` should only be called when `RouteReuseStrategy#shouldAttach` returns `true`. That is, we should only retrieve a stored route when the reuse strategy indicates that there is one stored and that it should be reattached. This now matches the behavior in the route activation: https://github.com/angular/angular/blob/1d12c50f63f90c91636185b2287e31e9c0291121/packages/router/src/operators/activate_routes.ts#L170-L172 Fixes angular#23162

atscott · 2021-04-13T22:10:08Z

@AndrewKushnir -- Oops! I had done that but didn't add it to the commit. PTAL

sliekens · 2021-04-13T22:17:29Z

Regarding #23162

It seems like routeReuseStrategy.shouldAttach is still called for the initial navigation. I might just be misinterpreting the code, I haven't tested it. If so, is it possible/preferable to skip that test and go straight to the createActivatedRoute path for the initial navigation?

AndrewKushnir

@atscott LGTM 👍

atscott · 2021-04-13T22:48:27Z

@StevenLiekens it may be possible but I'm wondering why it would be necessary. Shouldn't the RouteReuseStrategy implementation return false for shouldAttach on the initial navigation? I'm not completely familiar with this part of the router code and I'm wary of making changes beyond what I can determine to be safe. At least with the change as it is now, it can be supported by the existing implementation in the activate_routes code.

sliekens · 2021-04-14T07:51:29Z

Shouldn't the RouteReuseStrategy implementation return false for shouldAttach on the initial navigation?

Yep, that's why I'm thinking the method shouldn't even be tested (you already know the answer). But it would only be a minor optimization so leaving it as-is seems completely reasonable.

atscott · 2021-04-14T16:11:05Z

global presubmit
The initial presubmit brought up one test failure. The test assertion just needed to be updated and I don't believe it would affect the application functionality. Global presubmit should also run to verify there are no other failures.

edit: full global presubmit (green)

atscott · 2021-04-15T17:51:47Z

caretaker note: please merge and sync this one on its own. Global presubmit is green but it would still be good to do since this is a router change.

… it should reattach (#30263) When creating the router state, the `RouteReuseStrategy#retrieve` should only be called when `RouteReuseStrategy#shouldAttach` returns `true`. That is, we should only retrieve a stored route when the reuse strategy indicates that there is one stored and that it should be reattached. This now matches the behavior in the route activation: https://github.com/angular/angular/blob/1d12c50f63f90c91636185b2287e31e9c0291121/packages/router/src/operators/activate_routes.ts#L170-L172 Fixes #23162 PR Close #30263

angular-automatic-lock-bot · 2021-05-16T16:07:56Z

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

dimakuba requested a review from a team as a code owner May 4, 2019 18:33

This comment has been minimized.

Sign in to view

googlebot added the cla: no label May 4, 2019

kara added the area: router label May 6, 2019

ngbot bot added this to the needsTriage milestone May 6, 2019

dmkuba force-pushed the should-route-reattach branch from ae081b9 to 0641592 Compare May 10, 2019 18:44

googlebot added cla: yes and removed cla: no labels May 10, 2019

atscott requested changes Aug 4, 2020

View reviewed changes

packages/router/src/create_router_state.ts Outdated Show resolved Hide resolved

packages/router/test/create_router_state.spec.ts Outdated Show resolved Hide resolved

packages/router/test/create_router_state.spec.ts Show resolved Hide resolved

atscott force-pushed the should-route-reattach branch from 0641592 to f0c7f7b Compare April 13, 2021 21:51

atscott changed the title ~~refactor(router): determine if a route (and its subtree) should be reattached until detached route retrieving~~ fix(router): Only retrieve stored route when reuse strategy indicates it should reattach Apr 13, 2021

atscott mentioned this pull request Apr 13, 2021

Unnecessary calls to retrieve and shouldAttach in RouteReuseStrategy #23162

Closed

atscott approved these changes Apr 13, 2021

View reviewed changes

atscott requested a review from AndrewKushnir April 13, 2021 21:56

AndrewKushnir approved these changes Apr 13, 2021

View reviewed changes

atscott force-pushed the should-route-reattach branch from f0c7f7b to df2efd2 Compare April 13, 2021 22:09

AndrewKushnir approved these changes Apr 13, 2021

View reviewed changes

atscott added the target: patch This PR is targeted for the next patch release label Apr 14, 2021

ngbot bot modified the milestones: needsTriage, Backlog Apr 14, 2021

atscott added the action: presubmit The PR is in need of a google3 presubmit label Apr 14, 2021

atscott removed the action: presubmit The PR is in need of a google3 presubmit label Apr 15, 2021

atscott added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note freq2: medium labels Apr 15, 2021

AndrewKushnir closed this in 6bceb70 Apr 15, 2021

angular-automatic-lock-bot bot locked and limited conversation to collaborators May 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(router): Only retrieve stored route when reuse strategy indicates it should reattach #30263

fix(router): Only retrieve stored route when reuse strategy indicates it should reattach #30263

dimakuba commented May 4, 2019 •

edited by atscott

This comment has been minimized.

googlebot commented May 10, 2019

atscott left a comment

atscott left a comment

atscott commented Apr 13, 2021

AndrewKushnir left a comment

atscott commented Apr 13, 2021

sliekens commented Apr 13, 2021

AndrewKushnir left a comment

atscott commented Apr 13, 2021

sliekens commented Apr 14, 2021

atscott commented Apr 14, 2021 •

edited

atscott commented Apr 15, 2021

angular-automatic-lock-bot bot commented May 16, 2021

	if (this.routeReuseStrategy.shouldAttach(future.snapshot)) {
	const stored =
	(<DetachedRouteHandleInternal>this.routeReuseStrategy.retrieve(future.snapshot));

fix(router): Only retrieve stored route when reuse strategy indicates it should reattach #30263

fix(router): Only retrieve stored route when reuse strategy indicates it should reattach #30263

Conversation

dimakuba commented May 4, 2019 • edited by atscott

This comment has been minimized.

googlebot commented May 10, 2019

atscott left a comment

Choose a reason for hiding this comment

atscott left a comment

Choose a reason for hiding this comment

atscott commented Apr 13, 2021

AndrewKushnir left a comment

Choose a reason for hiding this comment

atscott commented Apr 13, 2021

sliekens commented Apr 13, 2021

AndrewKushnir left a comment

Choose a reason for hiding this comment

atscott commented Apr 13, 2021

sliekens commented Apr 14, 2021

atscott commented Apr 14, 2021 • edited

atscott commented Apr 15, 2021

angular-automatic-lock-bot bot commented May 16, 2021

dimakuba commented May 4, 2019 •

edited by atscott

atscott commented Apr 14, 2021 •

edited