Skip to content
Permalink
Browse files

fix(router): ensure navigations start with the current URL value inca…

…se redirect is skipped (angular#30344)

In some cases where multiple navigations happen to the same URL, the router will not process a given URL. In those cases, we fall into logic that resets state for the next navigation. One piece of this resetting is to set the `browserUrlTree` to the most recent `urlAfterRedirects`i.

However, there was bug in this logic because in some cases the `urlAfterRedirects` is a stale value. This happens any time a URL won't be processed, and the previous URL will also not be processed. This creates unpredictable behavior, not the least of which ends up being a broken `back` button.

This PR kicks off new navigations with the current value the router assumes is in the browser. All the logic around how to handle future navigations is based on this value compared to the current transition, so it's important to kick off all new navigations with the current value so in the edge case described above we don't end up with an old value being set into `browserUrlTree`.

Fixes angular#30340
Related to angular#30160

PR Close angular#30344
  • Loading branch information...
jasonaden authored and BioPhoton committed May 8, 2019
1 parent abbb25e commit 882e0db15aa6fab363fbecc6307363afef980830
Showing with 68 additions and 1 deletion.
  1. +8 −1 packages/router/src/router.ts
  2. +60 −0 packages/router/test/integration.spec.ts
@@ -773,7 +773,14 @@ export class Router {
this.routerState.root.component = this.rootComponentType;
}

private getTransition(): NavigationTransition { return this.transitions.value; }
private getTransition(): NavigationTransition {
const transition = this.transitions.value;
// This value needs to be set. Other values such as extractedUrl are set on initial navigation
// but the urlAfterRedirects may not get set if we aren't processing the new URL *and* not
// processing the previous URL.
transition.urlAfterRedirects = this.browserUrlTree;
return transition;
}

private setTransition(t: Partial<NavigationTransition>): void {
this.transitions.next({...this.getTransition(), ...t});
@@ -218,6 +218,39 @@ describe('Integration', () => {
})));
});


/**
* get/setTransition are private APIs. This test is needed though to guarantee the correct
* values are being used. Related to https://github.com/angular/angular/issues/30340 where
* stale transition data was being used when kicking off a new navigation.
*/
describe('get/setTransition', () => {
it('should provide the most recent NavigationTransition',
fakeAsync(inject([Router, Location], (router: Router, location: SpyLocation) => {
router.resetConfig([
{path: '', component: SimpleCmp}, {path: 'a', component: SimpleCmp},
{path: 'b', component: SimpleCmp}
]);

const fixture = createRoot(router, RootCmp);

const initialTransition = (router as any).getTransition();

// Confirm initial value
expect(initialTransition.urlAfterRedirects.toString()).toBe('/');


router.navigateByUrl('/a', {replaceUrl: true});

tick();

// After a navigation, we should see the URL after redirect
const nextTransition = (router as any).getTransition();
// Confirm initial value
expect(nextTransition.urlAfterRedirects.toString()).toBe('/a');
})));
});

describe('navigation warning', () => {
let warnings: string[] = [];

@@ -662,6 +695,33 @@ describe('Integration', () => {
expect(location.path()).toEqual('/login');
})));

it('should set browserUrlTree with urlUpdateStrategy="eagar" and false `shouldProcessUrl`',
fakeAsync(inject([Router, Location], (router: Router, location: Location) => {
const fixture = TestBed.createComponent(RootCmp);
advance(fixture);

router.urlUpdateStrategy = 'eager';

router.resetConfig([
{path: 'team/:id', component: SimpleCmp},
{path: 'login', component: AbsoluteSimpleLinkCmp}
]);

router.navigateByUrl('/team/22');
advance(fixture, 1);

expect((router as any).browserUrlTree.toString()).toBe('/team/22');

// Force to not process URL changes
router.urlHandlingStrategy.shouldProcessUrl = (url: UrlTree) => false;

router.navigateByUrl('/login');
advance(fixture, 1);

// Do not change locations
expect((router as any).browserUrlTree.toString()).toBe('/team/22');
})));

it('should eagerly update URL after redirects are applied with urlUpdateStrategy="eagar"',
fakeAsync(inject([Router, Location], (router: Router, location: Location) => {
const fixture = TestBed.createComponent(RootCmp);

0 comments on commit 882e0db

Please sign in to comment.
You can’t perform that action at this time.