Skip to content

Commit 0fd9d08

Browse files
jasonadenalxhub
authored andcommitted
fix(router): ensure navigations start with the current URL value incase redirect is skipped (#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 #30340 Related to #30160 PR Close #30344
1 parent 4f9b167 commit 0fd9d08

2 files changed

Lines changed: 68 additions & 1 deletion

File tree

‎packages/router/src/router.ts‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -773,7 +773,14 @@ export class Router {
773773
this.routerState.root.component = this.rootComponentType;
774774
}
775775

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

778785
private setTransition(t: Partial<NavigationTransition>): void {
779786
this.transitions.next({...this.getTransition(), ...t});

‎packages/router/test/integration.spec.ts‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -218,6 +218,39 @@ describe('Integration', () => {
218218
})));
219219
});
220220

221+
222+
/**
223+
* get/setTransition are private APIs. This test is needed though to guarantee the correct
224+
* values are being used. Related to https://github.com/angular/angular/issues/30340 where
225+
* stale transition data was being used when kicking off a new navigation.
226+
*/
227+
describe('get/setTransition', () => {
228+
it('should provide the most recent NavigationTransition',
229+
fakeAsync(inject([Router, Location], (router: Router, location: SpyLocation) => {
230+
router.resetConfig([
231+
{path: '', component: SimpleCmp}, {path: 'a', component: SimpleCmp},
232+
{path: 'b', component: SimpleCmp}
233+
]);
234+
235+
const fixture = createRoot(router, RootCmp);
236+
237+
const initialTransition = (router as any).getTransition();
238+
239+
// Confirm initial value
240+
expect(initialTransition.urlAfterRedirects.toString()).toBe('/');
241+
242+
243+
router.navigateByUrl('/a', {replaceUrl: true});
244+
245+
tick();
246+
247+
// After a navigation, we should see the URL after redirect
248+
const nextTransition = (router as any).getTransition();
249+
// Confirm initial value
250+
expect(nextTransition.urlAfterRedirects.toString()).toBe('/a');
251+
})));
252+
});
253+
221254
describe('navigation warning', () => {
222255
let warnings: string[] = [];
223256

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

698+
it('should set browserUrlTree with urlUpdateStrategy="eagar" and false `shouldProcessUrl`',
699+
fakeAsync(inject([Router, Location], (router: Router, location: Location) => {
700+
const fixture = TestBed.createComponent(RootCmp);
701+
advance(fixture);
702+
703+
router.urlUpdateStrategy = 'eager';
704+
705+
router.resetConfig([
706+
{path: 'team/:id', component: SimpleCmp},
707+
{path: 'login', component: AbsoluteSimpleLinkCmp}
708+
]);
709+
710+
router.navigateByUrl('/team/22');
711+
advance(fixture, 1);
712+
713+
expect((router as any).browserUrlTree.toString()).toBe('/team/22');
714+
715+
// Force to not process URL changes
716+
router.urlHandlingStrategy.shouldProcessUrl = (url: UrlTree) => false;
717+
718+
router.navigateByUrl('/login');
719+
advance(fixture, 1);
720+
721+
// Do not change locations
722+
expect((router as any).browserUrlTree.toString()).toBe('/team/22');
723+
})));
724+
665725
it('should eagerly update URL after redirects are applied with urlUpdateStrategy="eagar"',
666726
fakeAsync(inject([Router, Location], (router: Router, location: Location) => {
667727
const fixture = TestBed.createComponent(RootCmp);

0 commit comments

Comments
 (0)