fix(router): Fix arguments order for call to shouldReuseRoute (#26949)

The `createOrReuseChildren` function calls shouldReuseRoute with the
previous child values use as the future and the future child value used
as the current argument. This is incosistent with the argument order in
`createNode`. This inconsistent order can make it difficult/impossible
to correctly implement the `shouldReuseRoute` function. Usually this
order doesn't matter because simple equality checks are made on the
args and it doesn't matter which is which.

More detail can be found in the bug report: #16192.

Fix #16192

BREAKING CHANGE: This change corrects the argument order when calling
RouteReuseStrategy#shouldReuseRoute. Previously, when evaluating child
routes, they would be called with the future and current arguments would
be swapped. If your RouteReuseStrategy relies specifically on only the future
or current snapshot state, you may need to update the shouldReuseRoute
implementation's use of "future" and "current" ActivateRouteSnapshots.

PR Close #26949
This commit is contained in:
Mathieu Lemoine 2018-09-25 11:18:31 -04:00 committed by Andrew Kushnir
parent 171a0d0696
commit 3817e5f1df
2 changed files with 32 additions and 2 deletions

View File

@ -65,7 +65,7 @@ function createOrReuseChildren(
prevState: TreeNode<ActivatedRoute>) {
return curr.children.map(child => {
for (const p of prevState.children) {
if (routeReuseStrategy.shouldReuseRoute(p.value.snapshot, child.value)) {
if (routeReuseStrategy.shouldReuseRoute(child.value, p.value.snapshot)) {
return createNode(routeReuseStrategy, child, p);
}
}

View File

@ -16,7 +16,10 @@ import {DefaultUrlSerializer, UrlSegmentGroup, UrlTree} from '../src/url_tree';
import {TreeNode} from '../src/utils/tree';
describe('create router state', () => {
const reuseStrategy = new DefaultRouteReuseStrategy();
let reuseStrategy: DefaultRouteReuseStrategy;
beforeEach(() => {
reuseStrategy = new DefaultRouteReuseStrategy();
});
const emptyState = () =>
createEmptyState(new (UrlTree as any)(new UrlSegmentGroup([], {}), {}, null!), RootComponent);
@ -109,6 +112,33 @@ describe('create router state', () => {
// Verify the retrieve method has been called one more time
expect(reuseStrategy.retrieve).toHaveBeenCalledTimes(3);
});
it('should consistently represent future and current state', () => {
const config = [
{path: '', pathMatch: 'full', component: ComponentA},
{path: 'product/:id', component: ComponentB}
];
spyOn(reuseStrategy, 'shouldReuseRoute').and.callThrough();
const previousState = createRouterState(reuseStrategy, createState(config, ''), emptyState());
advanceState(previousState);
(reuseStrategy.shouldReuseRoute as jasmine.Spy).calls.reset();
createRouterState(reuseStrategy, createState(config, 'product/30'), previousState);
// One call for the root and one call for each of the children
expect(reuseStrategy.shouldReuseRoute).toHaveBeenCalledTimes(2);
const reuseCalls = (reuseStrategy.shouldReuseRoute as jasmine.Spy).calls;
const future1 = reuseCalls.argsFor(0)[0];
const current1 = reuseCalls.argsFor(0)[1];
const future2 = reuseCalls.argsFor(1)[0];
const current2 = reuseCalls.argsFor(1)[1];
// Routing from '' to 'product/30'
expect(current1._routerState.url).toEqual('');
expect(future1._routerState.url).toEqual('product/30');
expect(current2._routerState.url).toEqual('');
expect(future2._routerState.url).toEqual('product/30');
});
});
function advanceState(state: RouterState): void {