Skip to content

Commit

Permalink
refactor(router): Update Router to be providedIn: 'root' (#46914)
Browse files Browse the repository at this point in the history 
This commit updates the Router itself to be `providedIn: 'root'` with a
factory function rather than provided in the `RouterModule`.

PR Close #46914
  • Loading branch information
@atscott @AndrewKushnir
atscott authored and AndrewKushnir committed Jul 20, 2022
1 parent a8e9247 commit 0cbbd6a
Show file tree
Hide file tree
Showing 7 changed files with 363 additions and 342 deletions.
4 changes: 2 additions & 2 deletions goldens/public-api/router/index.md
View file
Expand Up @@ -733,11 +733,11 @@ export class RouterLinkWithHref implements OnChanges, OnDestroy {

// @public
export class RouterModule {
constructor(guard: any, router: Router);
constructor(guard: any);
static forChild(routes: Routes): ModuleWithProviders<RouterModule>;
static forRoot(routes: Routes, config?: ExtraOptions): ModuleWithProviders<RouterModule>;
// (undocumented)
static ɵfac: i0.ɵɵFactoryDeclaration<RouterModule, [{ optional: true; }, { optional: true; }]>;
static ɵfac: i0.ɵɵFactoryDeclaration<RouterModule, [{ optional: true; }]>;
// (undocumented)
static ɵinj: i0.ɵɵInjectorDeclaration<RouterModule>;
// (undocumented)
Expand Down
3 changes: 3 additions & 0 deletions packages/core/test/bundling/router/bundle.golden_symbols.json
View file
Expand Up @@ -1838,6 +1838,9 @@
{
"name": "setUpAttributes"
},
{
"name": "setupRouter"
},
{
"name": "shallowEqual"
},
Expand Down
3 changes: 2 additions & 1 deletion packages/router/src/index.ts
View file
Expand Up @@ -16,8 +16,9 @@ export {CanActivate, CanActivateChild, CanDeactivate, CanLoad, CanMatch, CanMatc
export {DefaultTitleStrategy, TitleStrategy} from './page_title_strategy';
export {BaseRouteReuseStrategy, DetachedRouteHandle, RouteReuseStrategy} from './route_reuse_strategy';
export {Navigation, NavigationExtras, Router, UrlCreationOptions} from './router';
export {ExtraOptions, InitialNavigation, ROUTER_CONFIGURATION} from './router_config';
export {ROUTES} from './router_config_loader';
export {ExtraOptions, InitialNavigation, provideRoutes, ROUTER_CONFIGURATION, ROUTER_INITIALIZER, RouterModule} from './router_module';
export {provideRoutes, ROUTER_INITIALIZER, RouterModule} from './router_module';
export {ChildrenOutletContexts, OutletContext} from './router_outlet_context';
export {NoPreloading, PreloadAllModules, PreloadingStrategy, RouterPreloader} from './router_preloader';
export {ActivatedRoute, ActivatedRouteSnapshot, RouterState, RouterStateSnapshot} from './router_state';
Expand Down
4 changes: 2 additions & 2 deletions packages/router/src/private_export.ts
View file
Expand Up @@ -8,6 +8,6 @@


export {ɵEmptyOutletComponent} from './components/empty_outlet';
export {RestoredState as ɵRestoredState} from './router';
export {assignExtraOptionsToRouter as ɵassignExtraOptionsToRouter, providePreloading as ɵprovidePreloading, ROUTER_PROVIDERS as ɵROUTER_PROVIDERS} from './router_module';
export {assignExtraOptionsToRouter as ɵassignExtraOptionsToRouter, RestoredState as ɵRestoredState} from './router';
export {providePreloading as ɵprovidePreloading, ROUTER_PROVIDERS as ɵROUTER_PROVIDERS} from './router_module';
export {flatten as ɵflatten} from './utils/collection';
84 changes: 69 additions & 15 deletions packages/router/src/router.ts
View file
Expand Up @@ -7,7 +7,7 @@
*/

import {Location} from '@angular/common';
import {Compiler, Injectable, Injector, NgModuleRef, NgZone, Type, ɵConsole as Console, ɵRuntimeError as RuntimeError} from '@angular/core';
import {Compiler, inject, Injectable, Injector, NgModuleRef, NgZone, Type, ɵConsole as Console, ɵRuntimeError as RuntimeError} from '@angular/core';
import {BehaviorSubject, combineLatest, EMPTY, Observable, of, Subject, SubscriptionLike} from 'rxjs';
import {catchError, defaultIfEmpty, filter, finalize, map, switchMap, take, tap} from 'rxjs/operators';

Expand All @@ -23,14 +23,16 @@ import {checkGuards} from './operators/check_guards';
import {recognize} from './operators/recognize';
import {resolveData} from './operators/resolve_data';
import {switchTap} from './operators/switch_tap';
import {TitleStrategy} from './page_title_strategy';
import {DefaultTitleStrategy, TitleStrategy} from './page_title_strategy';
import {DefaultRouteReuseStrategy, RouteReuseStrategy} from './route_reuse_strategy';
import {RouterConfigLoader} from './router_config_loader';
import {ErrorHandler, ExtraOptions, ROUTER_CONFIGURATION} from './router_config';
import {RouterConfigLoader, ROUTES} from './router_config_loader';
import {ChildrenOutletContexts} from './router_outlet_context';
import {ActivatedRoute, ActivatedRouteSnapshot, createEmptyState, RouterState, RouterStateSnapshot} from './router_state';
import {Params} from './shared';
import {DefaultUrlHandlingStrategy, UrlHandlingStrategy} from './url_handling_strategy';
import {containsTree, createEmptyUrlTree, IsActiveMatchOptions, isUrlTree, UrlSerializer, UrlTree} from './url_tree';
import {flatten} from './utils/collection';
import {standardizeConfig, validateConfig} from './utils/config';
import {Checks, getAllRouteGuards} from './utils/preactivation';

Expand Down Expand Up @@ -163,17 +165,6 @@ export interface UrlCreationOptions {
*/
export interface NavigationExtras extends UrlCreationOptions, NavigationBehaviorOptions {}

/**
* Error handler that is invoked when a navigation error occurs.
*
* If the handler returns a value, the navigation Promise is resolved with this value.
* If the handler throws an exception, the navigation Promise is rejected with
* the exception.
*
* @publicApi
*/
export type ErrorHandler = (error: any) => any;

function defaultErrorHandler(error: any): any {
throw error;
}
Expand Down Expand Up @@ -301,6 +292,66 @@ export const subsetMatchOptions: IsActiveMatchOptions = {
queryParams: 'subset'
};

export function assignExtraOptionsToRouter(opts: ExtraOptions, router: Router): void {
if (opts.errorHandler) {
router.errorHandler = opts.errorHandler;
}

if (opts.malformedUriErrorHandler) {
router.malformedUriErrorHandler = opts.malformedUriErrorHandler;
}

if (opts.onSameUrlNavigation) {
router.onSameUrlNavigation = opts.onSameUrlNavigation;
}

if (opts.paramsInheritanceStrategy) {
router.paramsInheritanceStrategy = opts.paramsInheritanceStrategy;
}

if (opts.relativeLinkResolution) {
router.relativeLinkResolution = opts.relativeLinkResolution;
}

if (opts.urlUpdateStrategy) {
router.urlUpdateStrategy = opts.urlUpdateStrategy;
}

if (opts.canceledNavigationResolution) {
router.canceledNavigationResolution = opts.canceledNavigationResolution;
}
}

export function setupRouter() {
const urlSerializer = inject(UrlSerializer);
const contexts = inject(ChildrenOutletContexts);
const location = inject(Location);
const injector = inject(Injector);
const compiler = inject(Compiler);
const config = inject(ROUTES, {optional: true}) ?? [];
const opts = inject(ROUTER_CONFIGURATION, {optional: true}) ?? {};
const defaultTitleStrategy = inject(DefaultTitleStrategy);
const titleStrategy = inject(TitleStrategy, {optional: true});
const urlHandlingStrategy = inject(UrlHandlingStrategy, {optional: true});
const routeReuseStrategy = inject(RouteReuseStrategy, {optional: true});
const router =
new Router(null, urlSerializer, contexts, location, injector, compiler, flatten(config));

if (urlHandlingStrategy) {
router.urlHandlingStrategy = urlHandlingStrategy;
}

if (routeReuseStrategy) {
router.routeReuseStrategy = routeReuseStrategy;
}

router.titleStrategy = titleStrategy ?? defaultTitleStrategy;

assignExtraOptionsToRouter(opts, router);

return router;
}

/**
* @description
*
Expand All @@ -313,7 +364,10 @@ export const subsetMatchOptions: IsActiveMatchOptions = {
*
* @publicApi
*/
@Injectable()
@Injectable({
providedIn: 'root',
useFactory: setupRouter,
})
export class Router {
/**
* Represents the activated `UrlTree` that the `Router` is configured to handle (through
Expand Down

11 comments on commit 0cbbd6a

@woppa684
Copy link

@woppa684 woppa684 commented on 0cbbd6a Aug 10, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AndrewKushnir
FYI, this change was not mentioned in the changelog for 14.1.1 and I have the feeling it breaks our component, After upgrading from 14.1.0 to 14.1.1, we suddenly get the following error:

Error: Uncaught (in promise): NullInjectorError: NullInjectorError: No provider for UrlSerializer!
Error: NullInjectorError: No provider for UrlSerializer!
    at NullInjector.get (core.mjs:6368:27) [angular]
    at R3Injector.get (core.mjs:6795:33) [angular]
    at R3Injector.get (core.mjs:6795:33) [angular]
    at R3Injector.get (core.mjs:6795:33) [angular]
    at injectInjectorOnly (core.mjs:4779:33) [angular]
    at ɵɵinject (core.mjs:4783:12) [angular]
    at inject (core.mjs:4876:12) [angular]
    at setupRouter (router.mjs:4583:33) [angular]
    at Object.factory (router.mjs:5517:144) [angular]
    at R3Injector.hydrate (core.mjs:6896:35) [angular]
    at R3Injector.get (core.mjs:6784:33) [angular]
    at ServicesInjector.get (services-injector.ts:36:24) [angular]
    at R3Injector.get (core.mjs:6795:33) [angular]
    at AppInstanceInjector.get (app-instance-injector.ts:127:37) [angular]
    at resolvePromise (zone.js:1262:35) [angular]
    at :4200/polyfills.js:52622:11 [angular]
    at :4200/polyfills.js:52639:27 [angular]
    at asyncGeneratorStep (asyncToGenerator.js:6:1) [angular]
    at _throw (asyncToGenerator.js:29:1) [angular]
    at Object.onInvoke (core.mjs:26380:33) [angular]
    at :4200/polyfills.js:52800:28 [angular]
    at Object.onInvokeTask (core.mjs:26367:33) [angular]) complains (app-instance-injector.ts:127:37

This is the only commit I could find in 14.1.1 that could cause this. Our special situation could probably have something to do with it as well. We have created a framework that hosts different microfrontends. The framework itself doesn't use routing, but app developers are allowed to use it in their own microfrontend (using a NullLocationStrategy for obvious reasons). They then call RouterModule.forRoot() in their app, which is in a lazy loaded module of course.

To my understanding, a call to RouterModule.forRoot() would provide this UrlSerializer, correct?
By hoisting the Router to the root of the framework application (where there is no RouterModule.forRoot) I get the above error I assume. When I now change it so that my framework calls forRoot and all the lazy modules class forChild I get rid of the error but the routes don't work anymore (since the child routes are not loaded through routing). In our use case, these micro frontends can be opened and closed continuously, and there can also be multiple instances of the same micro frontend in the same view.

I guess that, since the way we did it (see below) is no longer supported in 14.1.1 I now have to create a mechanism that calls resetConfig when opening or closing a micro frontend instance, but that's quite a lot of work :)

This is the relevant module code of one of our apps:

import { NgModule } from '@angular/core';
import { RouterModule, Route, ActivatedRoute, Router } from '@angular/router';
import { AppWithRouterComponent } from './app-with-router.component';
import { ComponentAComponent } from './component-a.component';
import { ComponentBComponent } from './component-b.component';
import {FormsModule} from '@angular/forms';

const routes: Route[] = [
  { path: '', component: ComponentAComponent},
  { path: 'next', component: ComponentBComponent }
];

@NgModule({
  imports: [
    RouterModule.forRoot(routes, { relativeLinkResolution: 'legacy' }),
    FormsModule
  ],
  declarations: [
    AppWithRouterComponent,
    ComponentAComponent,
    ComponentBComponent,
  ],
  exports: [AppWithRouterComponent]
})
export class AppWithRouterModule {
  constructor (route: ActivatedRoute, router: Router) {
    router.initialNavigation();
  }
}

@woppa684
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like that with this change, it is no longer possible to have different routers for each lazy loaded module. If this is intentional, then it would at least make sense to share this as a breaking change, and not deliver this in a patch-release.

@atscott
Copy link
Contributor Author

@atscott atscott commented on 0cbbd6a Aug 10, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

14.1.2 includes the fix for the NullInjector. This happened unintentionally due to other required commits not making it into the patch release.

Providing multiple Routers was never intentionally supported nor documented behavior. The forRoot naming and documentation indicate the intent and expected use be that it's provided in the root injector for the application.

It's never the intent for us to make breaking changes, especially in patch releases. It's difficult to anticipate undocumented and unintended use of the existing APIs. Note that avoiding a breaking change is why other providers were kept in the return value of forRoot even though they could otherwise be omitted since they are providedIn: 'root' now.

I think a quick solution for your applications would be to add Router to the providers list of AppWithRouterModule. I can't tell exactly how you're creating these applications, but if they are created with a new injector in the hierarchy, they can get a separate Router instance by providing it again.

Additionally, we'll investigate adding Router back to the ROUTER_PROVIDERS list so multiple calls to forRoot in separate injectors would still result in multiple Router instances. (Edit: It looks like Router is still listed in the providers). Again, this certainly isn't the intended use, but we're really not trying to make breaking changes here. This would be considered more of a bug introduced in the patch release rather than an intentional breaking change.

Edit: Does the 14.1.2 release work for you? It looks like Router is still in the providers list of forRoot so you'd get another instance if you called it multiple times. c7fed38 would fix the error for the root router instance.

@atscott
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note, there is absolutely already an attempt to prevent what you've found a way to do (call RouterModule.forRoot multiple times):

angular/packages/router/src/router_module.ts

Lines 120 to 123 in 6454974

{
provide: ROUTER_FORROOT_GUARD,
useFactory: provideForRootGuard,
deps: [[Router, new Optional(), new SkipSelf()]]

@woppa684
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the reply! Well, the guard only prevents calling forRoot twice in the same injector hierarchy of course, what we do is call it in two separate hierarchies.

I've had success in providing Router in the created apps, although I had to change to forChild in the apps, do a forRoot in the framework, copy most of the setupRouter function (it is not exported for obvious reasons) and also provide the outlet administration (the ChildrenOutletContexts) in each injector separately (to prevent the "can not activate the active outlet" error.

We knew that we were using undocumented behavior of course, and we intend to improve our code. We just weren't expecting this change in this patch release. Thanks for your answer and effort!

@atscott
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@woppa684 Not sure if you saw my edit about the 14.1.2 release. Does that release fix your issue?

Again, there was absolutely a bug in 14.1.1 because Router wasn't actually capable of working with providedIn: 'root' on its own.

@woppa684
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hadn't seen it yet, will give it a try tomorrow 👍🏼

@woppa684
Copy link

@woppa684 woppa684 commented on 0cbbd6a Aug 11, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, this does not solve my problem because now I get the RouterModule.forRoot() called twice error. The logical explanation would be that when I call forRoot from a micro frontend, it iperforms the check, but the check injects the Router, which will be provided by the providedIn: 'root' of course. So, technically I did NOT call forRoot twice in the same Injector tree, but I can understand why it thinks I did ;) So, if this is indeed the case then I suggest that the error message is adapted to include something like ... or forRoot not called from the root of the application.

So, with the latest changes I think I really need to implement something clever. I cannot override the check since its InjectionToken is not exported and providing my own RouterModule factory is probably alsno not easy (or desirable) since then I need to use / copy unexported details of the router.

It would probably be best if I create a mechanism that keeps the router in the root of the framework and resets the config when child nodes are added or removed. I can use the instance id of the children as path and then append their own routes probably. Of course this would mean a breaking change for my clients, but so be it. Or do you see a simpler solution?

EDIT: After some experimenting, I think it's not really feasible with just one router, since (apart from dynamically updating the config, which I have managed to do) I would also have to dynamically name the outlets (to be all different) and dynamically alter the routerLinks to go to the correct outlet. Especially these last two things look impossible.

EDIT2: As an easy fix for now, I've found that "'unproviding" the Router in my root framework module solves the current problems. This might not be a solution for the longer term but at least we can move along with the latest version of Angular for now.

{ provide: Router, useValue: undefined },

@atscott
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, this does not solve my problem because now I get the RouterModule.forRoot() called twice error.

Ah, indeed the change would have the effect of actually fixing the check to be as strict as intended.

So, if this is indeed the case then I suggest that the error message is adapted to include something like ... or forRoot not called from the root of the application.

Agreed, we should adjust the error message.

As an easy fix for now, I've found that "'unproviding" the Router in my root framework

👍

@woppa684
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@atscott Of course, this "easy fix" doesn't work anymore in Ng15. With the introduction of the extracted NavigationTransitions, unproviding the Router is no longer sufficient. I would also need to reprovide NavigationTransitions but unfortunately this token is not available in the public API, so it is always provided in the root and I cannot change that.
I thought I would just mention this here and not create a separate issue for this since I'm working with an unsupported usecase.

I have no clue how to support my use case now, any thoughts are welcome :)

@atscott
Copy link
Contributor Author

@atscott atscott commented on 0cbbd6a Apr 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@woppa684 Right, this is very much not supported. Your best path forward if you want to support this long term without constantly being broken by upstream changes would be to fork the Router code and maintain this use-case in-house.

Please sign in to comment.