93219: Support Router in InitService

For Router to work properly, APP_BASE_HREF must be resolved _before_ the APP_INITIALIZER factory is called (otherwise Angular will attempt to initialize APP_BASE_HREF too soon)
To fix this we add a pre-initialization hook to APP_CONFIG so BrowserInitService can resolve it before APP_INITIALIZER

src/app/app.module.ts

@@ -1,6 +1,6 @@
 import { APP_BASE_HREF, CommonModule } from '@angular/common';
 import { HTTP_INTERCEPTORS, HttpClientModule } from '@angular/common/http';
-import { APP_INITIALIZER, NgModule } from '@angular/core';
+import { NgModule } from '@angular/core';
 import { AbstractControl } from '@angular/forms';
 import { BrowserModule } from '@angular/platform-browser';
 
@@ -28,14 +28,8 @@ import { LocaleInterceptor } from './core/locale/locale.interceptor';
 import { XsrfInterceptor } from './core/xsrf/xsrf.interceptor';
 import { LogInterceptor } from './core/log/log.interceptor';
 import { EagerThemesModule } from '../themes/eager-themes.module';
-
 import { APP_CONFIG, AppConfig } from '../config/app-config.interface';
 import { RootModule } from './root.module';
-import { InitService } from './init.service';
-
-export function getConfig() {
-  return environment;
-}
 
 export function getBase(appConfig: AppConfig) {
   return appConfig.ui.nameSpace;
@@ -78,16 +72,6 @@ IMPORTS.push(
 );
 
 const PROVIDERS = [
-  {
-    provide: APP_INITIALIZER,
-    useFactory: (initService: InitService) => initService.init(),
-    deps: [ InitService ],
-    multi: true,
-  },
-  {
-    provide: APP_CONFIG,
-    useFactory: getConfig
-  },
   {
     provide: APP_BASE_HREF,
     useFactory: getBase,

src/app/init.service.spec.ts

@@ -0,0 +1,83 @@
+import { InitService } from './init.service';
+import { APP_CONFIG } from 'src/config/app-config.interface';
+import { APP_INITIALIZER } from '@angular/core';
+import objectContaining = jasmine.objectContaining;
+import createSpyObj = jasmine.createSpyObj;
+import SpyObj = jasmine.SpyObj;
+
+let spy: SpyObj<any>;
+
+export class ConcreteInitServiceMock extends InitService {
+  protected static resolveAppConfig() {
+    spy.resolveAppConfig();
+  }
+
+  protected init(): () => Promise<boolean> {
+    spy.init();
+    return async () => true;
+  }
+}
+
+
+describe('InitService', () => {
+  describe('providers', () => {
+    beforeEach(() => {
+      spy = createSpyObj('ConcreteInitServiceMock', {
+        resolveAppConfig: null,
+        init: null,
+      });
+    });
+
+    it('should throw error when called on abstract InitService', () => {
+      expect(() => InitService.providers()).toThrow();
+    });
+
+    it('should correctly set up provider dependencies', () => {
+      const providers = ConcreteInitServiceMock.providers();
+
+      expect(providers).toContain(objectContaining({
+        provide: InitService,
+        useClass: ConcreteInitServiceMock
+      }));
+
+      expect(providers).toContain(objectContaining({
+        provide: APP_CONFIG,
+      }));
+
+      expect(providers).toContain(objectContaining({
+        provide: APP_INITIALIZER,
+        deps: [ InitService ],
+        multi: true,
+      }));
+    });
+
+    it('should call resolveAppConfig() in APP_CONFIG factory', () => {
+      const factory = (
+        ConcreteInitServiceMock.providers()
+                               .find((p: any) => p.provide === APP_CONFIG) as any
+      ).useFactory;
+
+      // this factory is called _before_ InitService is instantiated
+      factory();
+      expect(spy.resolveAppConfig).toHaveBeenCalled();
+      expect(spy.init).not.toHaveBeenCalled();
+    });
+
+    it('should defer to init() in APP_INITIALIZER factory', () => {
+      const factory = (
+        ConcreteInitServiceMock.providers()
+                               .find((p: any) => p.provide === APP_INITIALIZER) as any
+      ).useFactory;
+
+      // we don't care about the dependencies here
+      // @ts-ignore
+      const instance = new ConcreteInitServiceMock(null, null, null);
+
+      // provider ensures that the right concrete instance is passed to the factory
+      factory(instance);
+      expect(spy.resolveAppConfig).not.toHaveBeenCalled();
+      expect(spy.init).toHaveBeenCalled();
+    });
+  });
+});
+

src/app/init.service.ts

@@ -6,30 +6,18 @@
  * http://www.dspace.org/license/
  */
 import { Store } from '@ngrx/store';
-import { AppState } from './app.reducer';
 import { CheckAuthenticationTokenAction } from './core/auth/auth.actions';
 import { CorrelationIdService } from './correlation-id/correlation-id.service';
 import { DSpaceTransferState } from '../modules/transfer-state/dspace-transfer-state.service';
+import { APP_INITIALIZER, Provider, Type } from '@angular/core';
+import { TransferState } from '@angular/platform-browser';
+import { APP_CONFIG } from '../config/app-config.interface';
+import { environment } from '../environments/environment';
+import { AppState } from './app.reducer';
 
 /**
  * Performs the initialization of the app.
- *
+ * Should be extended to implement server- & browser-specific functionality.
- * Should have distinct extensions for server & browser for the specifics.
- * Should be provided in AppModule as follows
- * ```
- * {
- *   provide: APP_INITIALIZER
- *   useFactory: (initService: InitService) => initService.init(),
- *   deps: [ InitService ],
- *   multi: true,
- * }
- * ```
- *
- * In order to be injected in the common APP_INITIALIZER,
- * concrete subclasses should be provided in their respective app modules as
- * ```
- * { provide: InitService, useClass: SpecificInitService }
- * ```
  */
 export abstract class InitService {
   protected constructor(
@@ -40,14 +28,63 @@ export abstract class InitService {
   }
 
   /**
-   * Main initialization method, to be used as the APP_INITIALIZER factory.
+   * The initialization providers to use in `*AppModule`
-   *
+   * - this concrete {@link InitService}
-   * Note that the body of this method and the callback it returns are called
+   * - {@link APP_CONFIG} with optional pre-initialization hook
-   * at different times.
+   * - {@link APP_INITIALIZER}
-   * This is important to take into account when other providers depend on the
+   * <br>
-   * initialization logic (e.g. APP_BASE_HREF)
+   * Should only be called on concrete subclasses of InitService for the initialization hooks to work
    */
-  abstract init(): () => Promise<boolean>;
+  public static providers(): Provider[] {
+    if (!InitService.isPrototypeOf(this)) {
+      throw new Error(
+        'Initalization providers should only be generated from concrete subclasses of InitService'
+      );
+    }
+    return [
+      {
+        provide: InitService,
+        useClass: this as unknown as Type<InitService>,
+      },
+      {
+        provide: APP_CONFIG,
+        useFactory: (transferState: TransferState) => {
+          this.resolveAppConfig(transferState);
+          return environment;
+        },
+        deps: [ TransferState ]
+      },
+      {
+        provide: APP_INITIALIZER,
+        useFactory: (initService: InitService) => initService.init(),
+        deps: [ InitService ],
+        multi: true,
+      },
+    ];
+  }
+
+  /**
+   * Optional pre-initialization method to ensure that {@link APP_CONFIG} is fully resolved before {@link init} is called.
+   *
+   * For example, Router depends on APP_BASE_HREF, which in turn depends on APP_CONFIG.
+   * In production mode, APP_CONFIG is resolved from the TransferState when the app is initialized.
+   * If we want to use Router within APP_INITIALIZER, we have to make sure APP_BASE_HREF is resolved beforehand.
+   * In this case that means that we must transfer the configuration from the SSR state during pre-initialization.
+   * @protected
+   */
+  protected static resolveAppConfig(
+    transferState: TransferState
+  ): void {
+    // overriden in subclasses if applicable
+  }
+
+  /**
+   * Main initialization method.
+   * @protected
+   */
+  protected abstract init(): () => Promise<boolean>;
+
+  // Common initialization steps
 
   protected checkAuthenticationToken(): void {
     this.store.dispatch(new CheckAuthenticationTokenAction());

src/config/app-config.interface.ts

@@ -36,6 +36,10 @@ interface AppConfig extends Config {
   mediaViewer: MediaViewerConfig;
 }
 
+/**
+ * Injection token for the app configuration.
+ * Provided in {@link InitService.providers}.
+ */
 const APP_CONFIG = new InjectionToken<AppConfig>('APP_CONFIG');
 
 const APP_CONFIG_STATE = makeStateKey('APP_CONFIG_STATE');

src/modules/app/browser-app.module.ts

@@ -27,8 +27,8 @@ import { LocaleService } from '../../app/core/locale/locale.service';
 import { GoogleAnalyticsService } from '../../app/statistics/google-analytics.service';
 import { AuthRequestService } from '../../app/core/auth/auth-request.service';
 import { BrowserAuthRequestService } from '../../app/core/auth/browser-auth-request.service';
-import { InitService } from 'src/app/init.service';
 import { BrowserInitService } from './browser-init.service';
+import { InitService } from '../../app/init.service';
 
 export const REQ_KEY = makeStateKey<string>('req');
 
@@ -63,10 +63,7 @@ export function getRequest(transferState: TransferState): any {
     AppModule
   ],
   providers: [
-    {
+    ...BrowserInitService.providers(),
-      provide: InitService,
-      useClass: BrowserInitService,
-    },
     {
       provide: REQUEST,
       useFactory: getRequest,

src/modules/app/browser-init.service.ts

@@ -31,10 +31,17 @@ export class BrowserInitService extends InitService {
     super(store, correlationIdService, dspaceTransferState);
   }
 
-  public init(): () => Promise<boolean> {
+  protected static resolveAppConfig(
-    // this method must be called before the callback because APP_BASE_HREF depends on it
+    transferState: TransferState,
-    this.loadAppConfigFromSSR();
+  ) {
+    if (transferState.hasKey<AppConfig>(APP_CONFIG_STATE)) {
+      const appConfig = transferState.get<AppConfig>(APP_CONFIG_STATE, new DefaultAppConfig());
+      // extend environment with app config for browser
+      extendEnvironmentWithAppConfig(environment, appConfig);
+    }
+  }
 
+  protected init(): () => Promise<boolean> {
     return async () => {
       await this.transferAppState();
       this.checkAuthenticationToken();
@@ -43,12 +50,4 @@ export class BrowserInitService extends InitService {
       return true;
     };
   }
-
-  private loadAppConfigFromSSR(): void {
-    if (this.transferState.hasKey<AppConfig>(APP_CONFIG_STATE)) {
-      const appConfig = this.transferState.get<AppConfig>(APP_CONFIG_STATE, new DefaultAppConfig());
-      // extend environment with app config for browser
-      extendEnvironmentWithAppConfig(environment, appConfig);
-    }
-  }
 }

src/modules/app/server-app.module.ts

@@ -30,7 +30,6 @@ import { ServerHardRedirectService } from '../../app/core/services/server-hard-r
 import { Angulartics2Mock } from '../../app/shared/mocks/angulartics2.service.mock';
 import { AuthRequestService } from '../../app/core/auth/auth-request.service';
 import { ServerAuthRequestService } from '../../app/core/auth/server-auth-request.service';
-import { InitService } from '../../app/init.service';
 import { ServerInitService } from './server-init.service';
 
 export function createTranslateLoader(transferState: TransferState) {
@@ -56,10 +55,7 @@ export function createTranslateLoader(transferState: TransferState) {
     AppModule
   ],
   providers: [
-    {
+    ...ServerInitService.providers(),
-      provide: InitService,
-      useClass: ServerInitService,
-    },
     {
       provide: Angulartics2,
       useClass: Angulartics2Mock

src/modules/app/server-init.service.ts

@@ -29,7 +29,7 @@ export class ServerInitService extends InitService {
     super(store, correlationIdService, dspaceTransferState);
   }
 
-  public init(): () => Promise<boolean> {
+  protected init(): () => Promise<boolean> {
     return async () => {
       this.checkAuthenticationToken();
       this.saveAppConfigForCSR();