diff --git a/src/app/app.module.ts b/src/app/app.module.ts index e3f5c46adb..b3a5872722 100755 --- a/src/app/app.module.ts +++ b/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, diff --git a/src/app/init.service.spec.ts b/src/app/init.service.spec.ts new file mode 100644 index 0000000000..7bbd50e4b7 --- /dev/null +++ b/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; + +export class ConcreteInitServiceMock extends InitService { + protected static resolveAppConfig() { + spy.resolveAppConfig(); + } + + protected init(): () => Promise { + 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(); + }); + }); +}); + diff --git a/src/app/init.service.ts b/src/app/init.service.ts index c5bd6dddb0..d8ecf8d23a 100644 --- a/src/app/init.service.ts +++ b/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 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 } - * ``` + * Should be extended to implement server- & browser-specific functionality. */ 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. - * - * Note that the body of this method and the callback it returns are called - * at different times. - * This is important to take into account when other providers depend on the - * initialization logic (e.g. APP_BASE_HREF) + * The initialization providers to use in `*AppModule` + * - this concrete {@link InitService} + * - {@link APP_CONFIG} with optional pre-initialization hook + * - {@link APP_INITIALIZER} + *
+ * Should only be called on concrete subclasses of InitService for the initialization hooks to work */ - abstract init(): () => Promise; + 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, + }, + { + 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; + + // Common initialization steps protected checkAuthenticationToken(): void { this.store.dispatch(new CheckAuthenticationTokenAction()); diff --git a/src/config/app-config.interface.ts b/src/config/app-config.interface.ts index 62d0be7216..5b2f0b1eeb 100644 --- a/src/config/app-config.interface.ts +++ b/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('APP_CONFIG'); const APP_CONFIG_STATE = makeStateKey('APP_CONFIG_STATE'); diff --git a/src/modules/app/browser-app.module.ts b/src/modules/app/browser-app.module.ts index e50018b51a..cd3feedad8 100644 --- a/src/modules/app/browser-app.module.ts +++ b/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('req'); @@ -63,10 +63,7 @@ export function getRequest(transferState: TransferState): any { AppModule ], providers: [ - { - provide: InitService, - useClass: BrowserInitService, - }, + ...BrowserInitService.providers(), { provide: REQUEST, useFactory: getRequest, diff --git a/src/modules/app/browser-init.service.ts b/src/modules/app/browser-init.service.ts index 1b050af82a..5c55795383 100644 --- a/src/modules/app/browser-init.service.ts +++ b/src/modules/app/browser-init.service.ts @@ -31,10 +31,17 @@ export class BrowserInitService extends InitService { super(store, correlationIdService, dspaceTransferState); } - public init(): () => Promise { - // this method must be called before the callback because APP_BASE_HREF depends on it - this.loadAppConfigFromSSR(); + protected static resolveAppConfig( + transferState: TransferState, + ) { + if (transferState.hasKey(APP_CONFIG_STATE)) { + const appConfig = transferState.get(APP_CONFIG_STATE, new DefaultAppConfig()); + // extend environment with app config for browser + extendEnvironmentWithAppConfig(environment, appConfig); + } + } + protected init(): () => Promise { 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(APP_CONFIG_STATE)) { - const appConfig = this.transferState.get(APP_CONFIG_STATE, new DefaultAppConfig()); - // extend environment with app config for browser - extendEnvironmentWithAppConfig(environment, appConfig); - } - } } diff --git a/src/modules/app/server-app.module.ts b/src/modules/app/server-app.module.ts index f61712ccb0..2b0462e9a0 100644 --- a/src/modules/app/server-app.module.ts +++ b/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: [ - { - provide: InitService, - useClass: ServerInitService, - }, + ...ServerInitService.providers(), { provide: Angulartics2, useClass: Angulartics2Mock diff --git a/src/modules/app/server-init.service.ts b/src/modules/app/server-init.service.ts index d5814f10f3..11fcc482ca 100644 --- a/src/modules/app/server-init.service.ts +++ b/src/modules/app/server-init.service.ts @@ -29,7 +29,7 @@ export class ServerInitService extends InitService { super(store, correlationIdService, dspaceTransferState); } - public init(): () => Promise { + protected init(): () => Promise { return async () => { this.checkAuthenticationToken(); this.saveAppConfigForCSR();