Skip to content

Commit

Permalink
refactor(router): clean up unnecessary flag in restoreHistory funct…
Browse files Browse the repository at this point in the history
…ion (#43382)

This restores the `finalize` function to directly call
`resetUrlToCurrentUrlTree`, as it was before efb440e.
This allows us to simplify the `restoreHistory` function because it no
longer needs to handle the call from `finalize` where it should not
reset the internal router state.

PR Close #43382
  • Loading branch information
atscott authored and AndrewKushnir committed Sep 9, 2021
1 parent 141fde1 commit 2658219
Showing 1 changed file with 8 additions and 22 deletions.
30 changes: 8 additions & 22 deletions packages/router/src/router.ts
Expand Up @@ -897,19 +897,11 @@ export class Router {
// AngularJS sync code which looks for a value here in order to determine
// whether or not to handle a given popstate event or to leave it to the
// Angular router.
this.restoreHistory(t);
this.cancelNavigationTransition(t, cancelationReason);
} else {
// We cannot trigger a `location.historyGo` if the
// cancellation was due to a new navigation before the previous could
// complete. This is because `location.historyGo` triggers a `popstate`
// which would also trigger another navigation. Instead, treat this as a
// redirect and do not reset the state.
this.cancelNavigationTransition(t, cancelationReason);
// TODO(atscott): The same problem happens here with a fresh page load
// and a new navigation before that completes where we won't set a page
// id.
this.resetUrlToCurrentUrlTree();
}
// Note: Other `canceledNavigationResolution` strategies will not support
// the AngularJS use-case that's mentioned above.
this.cancelNavigationTransition(t, cancelationReason);
}
// currentNavigation should always be reset to null here. If navigation was
// successful, lastSuccessfulTransition will have already been set. Therefore
Expand Down Expand Up @@ -940,7 +932,7 @@ export class Router {
// This is only applicable with initial navigation, so setting
// `navigated` only when not redirecting resolves this scenario.
this.navigated = true;
this.restoreHistory(t, true);
this.restoreHistory(t);
}
const navCancel = new NavigationCancel(
t.id, this.serializeUrl(t.extractedUrl), e.message);
Expand Down Expand Up @@ -977,7 +969,7 @@ export class Router {
/* All other errors should reset to the router's internal URL reference to
* the pre-error state. */
} else {
this.restoreHistory(t, true);
this.restoreHistory(t);
const navError =
new NavigationError(t.id, this.serializeUrl(t.extractedUrl), e);
eventsSubject.next(navError);
Expand Down Expand Up @@ -1487,7 +1479,7 @@ export class Router {
* Performs the necessary rollback action to restore the browser URL to the
* state before the transition.
*/
private restoreHistory(t: NavigationTransition, restoringFromCaughtError = false) {
private restoreHistory(t: NavigationTransition) {
if (this.canceledNavigationResolution === 'computed') {
const targetPagePosition = this.currentPageId - t.targetPageId;
// The navigator change the location before triggered the browser event,
Expand Down Expand Up @@ -1515,13 +1507,7 @@ export class Router {
// there's no restoration needed.
}
} else if (this.canceledNavigationResolution === 'replace') {
// TODO(atscott): It seems like we should _always_ reset the state here. It would be a no-op
// for `deferred` navigations that haven't change the internal state yet because guards
// reject. For 'eager' navigations, it seems like we also really should reset the state
// because the navigation was cancelled. Investigate if this can be done by running TGP.
if (restoringFromCaughtError) {
this.resetState(t);
}
this.resetState(t);
this.resetUrlToCurrentUrlTree();
}
}
Expand Down

0 comments on commit 2658219

Please sign in to comment.