fix(router): adjust UrlTree redirect to replace URL if in eager update (#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
This commit is contained in:
Alison Gale 2019-10-03 11:16:37 -07:00 committed by Matias Niemelä
parent 6958d11d95
commit f6667f8281
2 changed files with 68 additions and 10 deletions

View File

@ -739,10 +739,27 @@ export class Router {
const navCancel = const navCancel =
new NavigationCancel(t.id, this.serializeUrl(t.extractedUrl), e.message); new NavigationCancel(t.id, this.serializeUrl(t.extractedUrl), e.message);
eventsSubject.next(navCancel); eventsSubject.next(navCancel);
t.resolve(false);
if (redirecting) { // When redirecting, we need to delay resolving the navigation
this.navigateByUrl(e.url); // 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 /* All other errors should reset to the router's internal URL reference to the
@ -1052,7 +1069,8 @@ export class Router {
private scheduleNavigation( private scheduleNavigation(
rawUrl: UrlTree, source: NavigationTrigger, restoredState: RestoredState|null, 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(); const lastNavigation = this.getTransition();
// If the user triggers a navigation imperatively (e.g., by using navigateByUrl), // If the user triggers a navigation imperatively (e.g., by using navigateByUrl),
// and that navigation results in 'replaceState' that leads to the same URL, // and that navigation results in 'replaceState' that leads to the same URL,
@ -1077,13 +1095,20 @@ export class Router {
return Promise.resolve(true); // return value is not used return Promise.resolve(true); // return value is not used
} }
let resolve: any = null; let resolve: any;
let reject: any = null; 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) => { } else {
promise = new Promise<boolean>((res, rej) => {
resolve = res; resolve = res;
reject = rej; reject = rej;
}); });
}
const id = ++this.navigationId; const id = ++this.navigationId;
this.setTransition({ this.setTransition({

View File

@ -2464,6 +2464,39 @@ describe('Integration', () => {
[NavigationEnd, '/'], [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', () => { describe('runGuardsAndResolvers', () => {