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

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

PR Close #31168
This commit is contained in:
Jason Aden 2019-06-20 10:40:42 -07:00 committed by Alex Rickabaugh
parent f96a81a818
commit 15e397816f
2 changed files with 67 additions and 10 deletions

View File

@ -739,10 +739,26 @@ 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
@ -1056,7 +1072,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,
@ -1081,13 +1098,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({

View File

@ -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', () => {