fix(router): Router.createUrlTree should work with any ActivatedRoute

[atscott]

This change makes the createUrlTreeFromSnapshot added in #45877 the
default and only behavior in the Router. This now addreses #42191, #38276, #22763,
and #48472 without needing to create custom handling to
call createUrlTreeFromSnapshot (since it's now called by Router.createUrlTree).

BREAKING CHANGE: Tests which mock ActivatedRoute instances may need to be adjusted
because Router.createUrlTree now does the right thing in more
scenarios. This means that tests with invalid/incomplete ActivatedRoute mocks
may behave differently than before. Additionally, tests may now navigate
to a real URL where before they would navigate to the root. Ensure that
tests provide expected routes to match.
There is rarely production impact, but it has been found that relative
navigations when using an ActivatedRoute that does not appear in the
current router state were effectively ignored in the past. By creating
the correct URLs, this sometimes resulted in different navigation
behavior in the application. Most often, this happens when attempting to
create a navigation that only updates query params using an empty
command array, for example router.navigate([], {relativeTo: route, queryParams: newQueryParams}). In this case, the relativeTo property
should be removed.

[atscott]

caretaker note: This is already patched into g3. The patch should be updated since we have 1 team currently migrating off of the old strategy (cl/512727208)

[AndrewKushnir]

This PR was merged into the repository by commit 31f210b.

[justinrassier]

We started receiving the warning that the behavior will change, but are concerned that we are in a usecase that has not been tested/thought of.

We are not using the router just to update query params in place, but instead are heavily leveraging named router outlets (not at the root). We have a centralized call to update the named router outlets with a call like:

this.router.navigate([{ outlets }], { //updates the named router outlets to the new sub-routes
  relativeTo: this.activatedRoute
  queryParams: {
    layout: layout 
  },
  queryParamsHandling: 'merge',
});

This causes the following warning to be emitted:

The navigation to /case/justin/test/(library:library//appeal:appeal)?layout=[...] will instead go to /(library:library//appeal:appeal)?layout=[...] in an upcoming version of Angular. relativeTo might need to be removed from the UrlCreationOptions.

If this is indeed true and a breaking change, this will cause our whole routing solution to break. I feel like this may be an oversight. When I remove the call to update the outlets the warning goes away, but of course our whole routing solution also breaks.

Also, if I call navigate with the route path included:

this.router.navigate(['/case/justin/test', { outlets }], { //updates the named router outlets to the new sub-routes
  relativeTo: this.activatedRoute
  queryParams: {
    layout: layout 
  },
  queryParamsHandling: 'merge',
});

We receive the warning:

The navigation to /case/justin/test/(library:library)?layout=[...] will instead go to /(library:library)?layout=[...] in an upcoming version of Angular.

This leads me to believe that something is overlooked regarding using named router outlets.

Could you please advise?

[atscott]

@justinrassier Can you please create a stackblitz to demonstrate this?

[justinrassier]

@atscott my coworker and I will work on getting you one ASAP. It's a bit of a complicated setup so it may take a little effort to recreate. Thanks for the quick response!

[atscott]

@justinrassier Thanks. It's a bit hard to know your exact situation without having a reproduction. I was able to reproduce the exact warning based on my experience with debugging breakages internally but I can't really know if this is what your app is doing: https://stackblitz.com/edit/angular-jixxmu?file=src%2Fmain.ts

Look out for a navigation in ngOnDestroy or a navigation in a service where the injected ActivatedRoute isn't active anymore - in the past, the commands were pretty much completely ignored if the value in relativeTo wasn't active. Without knowing what your app is doing, I would guess that this is what's happening.

[justinrassier]

@atscott Thanks for the details, we really appreciate it! We have a stackblitz with what we thought was our situation, but are not able to reproduce the error. We will dig into the details you provided here and maybe it can give us some insights into what our real app has going wrong.

The good news is that our architecture/setup should continue to be supported! We just need to figure out what we could be doing to cause the warning to show.

One question, would this also crop up if you did not pass in a relatveTo activated route? For kicks, we commented out the relativeTo line and just hard-coded the absolute path in to the nested route. We received the same error, just without the warning about relativeTo

[atscott]

One question, would this also crop up if you did not pass in a relatveTo activated route? For kicks, we commented out the relativeTo line and just hard-coded the absolute path in to the nested route. We received the same error, just without the warning about relativeTo

Yes, it absolutely can come up without using relativeTo. Again, it's a bit hard to say what the exact situation would be but there are several situations where the current situation just gets it really wrong. Unfortunately, applications sometimes depend on the broken behavior so bug fixes turn out to be breaking changes a lot of the time.

It might be helpful to put a breakpoint at the place where the warning is getting created and check if the resulting URL makes sense based on the inputs:

angular/packages/router/src/create_url_tree_strategy.ts

Lines 25 to 35 in 35f4918

	if (NG_DEV_MODE) {
	const treeFromSnapshotStrategy = new CreateUrlTreeUsingSnapshot().createUrlTree(
	relativeTo, currentState, currentUrlTree, commands, queryParams, fragment);
	if (treeFromSnapshotStrategy.toString() !== tree.toString()) {
	let warningString = `The navigation to ${tree.toString()} will instead go to ${
	treeFromSnapshotStrategy.toString()} in an upcoming version of Angular.`;
	if (!!relativeTo) {
	warningString += ' `relativeTo` might need to be removed from the `UrlCreationOptions`.';
	}
	tree._warnIfUsedForNavigation = warningString;
	}

[justinrassier]

Great tips @atscott thanks. I will have to dig in more when I get back on Monday, but this is a top priority for me to figure out so we don't get stuck on an old version of Angular.

We aren't doing any navigation inside of ngOnDestory but we are using ngrx and in particular the navigation happens within an effect that leverages values from the ngrx router store. Our router store is configured with a particular PostActivation navigationActionTiming which makes me wonder if something with that whole timing is causing the ActivatedRoute to not be active anymore. Not sure if that actually makes sense, but it's the only thread I have to pull at the moment.

Again, I really appreciate your time and your troubleshooting tips. I'l see what I can find out!

[atscott]

@justinrassier The PostActivation timing happens after the NavigationEnd event: https://github.com/ngrx/platform/blob/e765031e3974c1655e31f813d34b8893031d0bb4/modules/router-store/src/store_router_connecting.service.ts#L159-L162

So it's pretty similar to a navigation in ngOnDestroy. If the ActivatedRoute used in relativeTo is in a component that no longer exists because the new navigation went somewhere else, this would trigger the problem. In this situation, the 15.2.x version of createUrlTree would entirely ignore the commands. In v16, the navigation always happens relative to the ActivatedRoute regardless of whether it appears in the current router state or not (which addressed these issues #38276, #22763).

[justinrassier]

I am unsure what component would no longer exist in our case. (most) of our navigation triggers from a shell component that always exists, which still gets the error. And the relativeTo, which we always provide as of now, is meant to always be pointing at that same shell component that always exists.

Another confusing part is this actually was originally found when I was fixing up some tests in the upgrade and saw the console warning there. In this case, we are not testing any of the components, just the services and effects that run from tirggered actions in ngrx land (and router events). We are also not mocking any router pieces as part of this test, we are using the full RouterTestingModule and setting up a routing structure and some fake components to mimic the routing structure of the real app.

So on Monday I will try taking the Stackblitz we have been working on and adding in ngrx and the router store to see if I can get a full mini implementation of all of our moving pieces.

[justinrassier]

@atscott finally got a reproduction. The addition of ngrx and router-store were all a red herring. It appears to be a problem that crops up when you have a router configuration that has a nested route with a path: '' inside of it. Here is a stackblitz showing the warning with such a configuration:

https://stackblitz.com/edit/angular-nssbq3?file=src/main.ts

If we are doing something wrong, please let us know so we can resolve it before this becomes a blocker. Although it does feel like we may have stumbled across something that was not caught or thought of.

Thanks for all your help!

[atscott]

@justinrassier This is indeed a bug in the new implementation - I should have a fix for this soon. By the way, I think your use of relativeTo might be a workaround for this issue that's resolved in the new implementation: #42191.

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}