Skip to content

Commit

Permalink
fix(router): adjust UrlTree redirect to replace URL if in eager update (
Browse files Browse the repository at this point in the history
#32988)

Resubmit #31168 now that google3 tests can pass. This requires http://cl/272696717 to be patched.
Original description from jasonaden:

Without this change when using UrlTree redirects in urlUpdateStrategy="eager", the URL would get
updated to the target location, then redirected. This resulted in having an additional entry in the
history and thus the back button would be broken (going back would land on the URL causing a new
redirect).

Additionally, there was a bug where the redirect, even without urlUpdateStrategy="eager", could
create a history with too many entries. This was due to kicking off a new navigation within the
navigation cancelling logic. With this PR the new navigation is pushed to the next tick with a
setTimeout, allowing the page being redirected from to be cancelled before starting a new
navigation.

Related to #27148

fix(router): adjust UrlTree redirect to replace URL if in eager update

Fix lint errors

PR Close #32988
  • Loading branch information
Alison Gale authored and matsko committed Oct 18, 2019
1 parent 6958d11 commit f6667f8
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 10 deletions.
45 changes: 35 additions & 10 deletions packages/router/src/router.ts
Expand Up @@ -739,10 +739,27 @@ export class Router {
const navCancel =
new NavigationCancel(t.id, this.serializeUrl(t.extractedUrl), e.message);
eventsSubject.next(navCancel);
t.resolve(false);

if (redirecting) {
this.navigateByUrl(e.url);
// When redirecting, we need to delay resolving the navigation
// promise and push it to the redirect navigation
if (!redirecting) {
t.resolve(false);
} else {
// setTimeout is required so this navigation finishes with
// the return EMPTY below. If it isn't allowed to finish
// processing, there can be multiple navigations to the same
// URL.
setTimeout(() => {
const mergedTree = this.urlHandlingStrategy.merge(e.url, this.rawUrlTree);
const extras = {
skipLocationChange: t.extras.skipLocationChange,
replaceUrl: this.urlUpdateStrategy === 'eager'
};

return this.scheduleNavigation(
mergedTree, 'imperative', null, extras,
{resolve: t.resolve, reject: t.reject, promise: t.promise});
}, 0);
}

/* All other errors should reset to the router's internal URL reference to the
Expand Down Expand Up @@ -1052,7 +1069,8 @@ export class Router {

private scheduleNavigation(
rawUrl: UrlTree, source: NavigationTrigger, restoredState: RestoredState|null,
extras: NavigationExtras): Promise<boolean> {
extras: NavigationExtras,
priorPromise?: {resolve: any, reject: any, promise: Promise<boolean>}): Promise<boolean> {
const lastNavigation = this.getTransition();
// If the user triggers a navigation imperatively (e.g., by using navigateByUrl),
// and that navigation results in 'replaceState' that leads to the same URL,
Expand All @@ -1077,13 +1095,20 @@ export class Router {
return Promise.resolve(true); // return value is not used
}

let resolve: any = null;
let reject: any = null;
let resolve: any;
let reject: any;
let promise: Promise<boolean>;
if (priorPromise) {
resolve = priorPromise.resolve;
reject = priorPromise.reject;
promise = priorPromise.promise;

const promise = new Promise<boolean>((res, rej) => {
resolve = res;
reject = rej;
});
} else {
promise = new Promise<boolean>((res, rej) => {
resolve = res;
reject = rej;
});
}

const id = ++this.navigationId;
this.setTransition({
Expand Down
33 changes: 33 additions & 0 deletions packages/router/test/integration.spec.ts
Expand Up @@ -2464,6 +2464,39 @@ describe('Integration', () => {
[NavigationEnd, '/'],
]);
})));

it('replaces URL when URL is updated eagerly so back button can still work',
fakeAsync(inject([Router, Location], (router: Router, location: SpyLocation) => {
router.urlUpdateStrategy = 'eager';
router.resetConfig([
{path: '', component: SimpleCmp},
{path: 'one', component: RouteCmp, canActivate: ['returnUrlTree']},
{path: 'redirected', component: SimpleCmp}
]);
const fixture = createRoot(router, RootCmp);
router.navigateByUrl('/one');

tick();

expect(location.path()).toEqual('/redirected');
expect(location.urlChanges).toEqual(['replace: /', '/one', 'replace: /redirected']);
})));

it('should resolve navigateByUrl promise after redirect finishes',
fakeAsync(inject([Router, Location], (router: Router, location: SpyLocation) => {
let resolvedPath = '';
router.urlUpdateStrategy = 'eager';
router.resetConfig([
{path: '', component: SimpleCmp},
{path: 'one', component: RouteCmp, canActivate: ['returnUrlTree']},
{path: 'redirected', component: SimpleCmp}
]);
const fixture = createRoot(router, RootCmp);
router.navigateByUrl('/one').then(v => { resolvedPath = location.path(); });

tick();
expect(resolvedPath).toBe('/redirected');
})));
});

describe('runGuardsAndResolvers', () => {
Expand Down

0 comments on commit f6667f8

Please sign in to comment.