From 26226c4090edcc2f2ee876a4298e724aba8d48fe Mon Sep 17 00:00:00 2001 From: Thomas Rittson <31796059+eliykat@users.noreply.github.com> Date: Mon, 8 Apr 2024 07:59:12 +1000 Subject: [PATCH] [AC-2356] Use safeProvider in web core services module (#8521) * Also add tests * Exclude type (compile-time) tests from jest config --- apps/web/src/app/core/core.module.ts | 203 ++++++++++-------- .../src/platform/utils/safe-provider.ts | 24 ++- .../platform/utils/safe-provider.type.spec.ts | 111 ++++++++++ libs/shared/jest.config.angular.js | 5 + 4 files changed, 251 insertions(+), 92 deletions(-) create mode 100644 libs/angular/src/platform/utils/safe-provider.type.spec.ts diff --git a/apps/web/src/app/core/core.module.ts b/apps/web/src/app/core/core.module.ts index bd514b1d18..9d53bc39f0 100644 --- a/apps/web/src/app/core/core.module.ts +++ b/apps/web/src/app/core/core.module.ts @@ -1,6 +1,7 @@ import { CommonModule } from "@angular/common"; import { APP_INITIALIZER, NgModule, Optional, SkipSelf } from "@angular/core"; +import { SafeProvider, safeProvider } from "@bitwarden/angular/platform/utils/safe-provider"; import { SECURE_STORAGE, STATE_FACTORY, @@ -12,6 +13,7 @@ import { OBSERVABLE_DISK_STORAGE, OBSERVABLE_DISK_LOCAL_STORAGE, WINDOW, + SafeInjectionToken, } from "@bitwarden/angular/services/injection-tokens"; import { JslibServicesModule } from "@bitwarden/angular/services/jslib-services.module"; import { ModalService as ModalServiceAbstraction } from "@bitwarden/angular/services/modal.service"; @@ -58,97 +60,122 @@ import { Account, GlobalState, StateService } from "./state"; import { WebFileDownloadService } from "./web-file-download.service"; import { WebPlatformUtilsService } from "./web-platform-utils.service"; +/** + * Provider definitions used in the ngModule. + * Add your provider definition here using the safeProvider function as a wrapper. This will give you type safety. + * If you need help please ask for it, do NOT change the type of this array. + */ +const safeProviders: SafeProvider[] = [ + safeProvider(InitService), + safeProvider(RouterService), + safeProvider(EventService), + safeProvider(PolicyListService), + safeProvider({ + provide: APP_INITIALIZER as SafeInjectionToken<() => void>, + useFactory: (initService: InitService) => initService.init(), + deps: [InitService], + multi: true, + }), + safeProvider({ + provide: STATE_FACTORY, + useValue: new StateFactory(GlobalState, Account), + }), + safeProvider({ + provide: STATE_SERVICE_USE_CACHE, + useValue: false, + }), + safeProvider({ + provide: I18nServiceAbstraction, + useClass: I18nService, + deps: [SYSTEM_LANGUAGE, LOCALES_DIRECTORY, GlobalStateProvider], + }), + safeProvider({ provide: AbstractStorageService, useClass: HtmlStorageService, deps: [] }), + safeProvider({ + provide: SECURE_STORAGE, + // TODO: platformUtilsService.isDev has a helper for this, but using that service here results in a circular dependency. + // We have a tech debt item in the backlog to break up platformUtilsService, but in the meantime simply checking the environment here is less cumbersome. + useClass: process.env.NODE_ENV === "development" ? HtmlStorageService : MemoryStorageService, + deps: [], + }), + safeProvider({ + provide: MEMORY_STORAGE, + useClass: MemoryStorageService, + deps: [], + }), + safeProvider({ + provide: OBSERVABLE_MEMORY_STORAGE, + useClass: MemoryStorageServiceForStateProviders, + deps: [], + }), + safeProvider({ + provide: OBSERVABLE_DISK_STORAGE, + useFactory: () => new WindowStorageService(window.sessionStorage), + deps: [], + }), + safeProvider({ + provide: PlatformUtilsServiceAbstraction, + useClass: WebPlatformUtilsService, + useAngularDecorators: true, + }), + safeProvider({ + provide: MessagingServiceAbstraction, + useClass: BroadcasterMessagingService, + useAngularDecorators: true, + }), + safeProvider({ + provide: ModalServiceAbstraction, + useClass: ModalService, + useAngularDecorators: true, + }), + safeProvider(StateService), + safeProvider({ + provide: BaseStateServiceAbstraction, + useExisting: StateService, + }), + safeProvider({ + provide: FileDownloadService, + useClass: WebFileDownloadService, + useAngularDecorators: true, + }), + safeProvider(CollectionAdminService), + safeProvider({ + provide: WindowStorageService, + useFactory: () => new WindowStorageService(window.localStorage), + deps: [], + }), + safeProvider({ + provide: OBSERVABLE_DISK_LOCAL_STORAGE, + useExisting: WindowStorageService, + }), + safeProvider({ + provide: StorageServiceProvider, + useClass: WebStorageServiceProvider, + deps: [OBSERVABLE_DISK_STORAGE, OBSERVABLE_MEMORY_STORAGE, OBSERVABLE_DISK_LOCAL_STORAGE], + }), + safeProvider({ + provide: MigrationRunner, + useClass: WebMigrationRunner, + deps: [AbstractStorageService, LogService, MigrationBuilderService, WindowStorageService], + }), + safeProvider({ + provide: EnvironmentService, + useClass: WebEnvironmentService, + deps: [WINDOW, StateProvider, AccountService], + }), + safeProvider({ + provide: ThemeStateService, + useFactory: (globalStateProvider: GlobalStateProvider) => + // Web chooses to have Light as the default theme + new DefaultThemeStateService(globalStateProvider, ThemeType.Light), + deps: [GlobalStateProvider], + }), +]; + @NgModule({ declarations: [], imports: [CommonModule, JslibServicesModule], - providers: [ - InitService, - RouterService, - EventService, - PolicyListService, - { - provide: APP_INITIALIZER, - useFactory: (initService: InitService) => initService.init(), - deps: [InitService], - multi: true, - }, - { - provide: STATE_FACTORY, - useValue: new StateFactory(GlobalState, Account), - }, - { - provide: STATE_SERVICE_USE_CACHE, - useValue: false, - }, - { - provide: I18nServiceAbstraction, - useClass: I18nService, - deps: [SYSTEM_LANGUAGE, LOCALES_DIRECTORY, GlobalStateProvider], - }, - { provide: AbstractStorageService, useClass: HtmlStorageService }, - { - provide: SECURE_STORAGE, - // TODO: platformUtilsService.isDev has a helper for this, but using that service here results in a circular dependency. - // We have a tech debt item in the backlog to break up platformUtilsService, but in the meantime simply checking the environment here is less cumbersome. - useClass: process.env.NODE_ENV === "development" ? HtmlStorageService : MemoryStorageService, - }, - { - provide: MEMORY_STORAGE, - useClass: MemoryStorageService, - }, - { provide: OBSERVABLE_MEMORY_STORAGE, useClass: MemoryStorageServiceForStateProviders }, - { - provide: OBSERVABLE_DISK_STORAGE, - useFactory: () => new WindowStorageService(window.sessionStorage), - }, - { - provide: PlatformUtilsServiceAbstraction, - useClass: WebPlatformUtilsService, - }, - { provide: MessagingServiceAbstraction, useClass: BroadcasterMessagingService }, - { provide: ModalServiceAbstraction, useClass: ModalService }, - StateService, - { - provide: BaseStateServiceAbstraction, - useExisting: StateService, - }, - { - provide: FileDownloadService, - useClass: WebFileDownloadService, - }, - CollectionAdminService, - { - provide: OBSERVABLE_DISK_LOCAL_STORAGE, - useFactory: () => new WindowStorageService(window.localStorage), - }, - { - provide: StorageServiceProvider, - useClass: WebStorageServiceProvider, - deps: [OBSERVABLE_DISK_STORAGE, OBSERVABLE_MEMORY_STORAGE, OBSERVABLE_DISK_LOCAL_STORAGE], - }, - { - provide: MigrationRunner, - useClass: WebMigrationRunner, - deps: [ - AbstractStorageService, - LogService, - MigrationBuilderService, - OBSERVABLE_DISK_LOCAL_STORAGE, - ], - }, - { - provide: EnvironmentService, - useClass: WebEnvironmentService, - deps: [WINDOW, StateProvider, AccountService], - }, - { - provide: ThemeStateService, - useFactory: (globalStateProvider: GlobalStateProvider) => - // Web chooses to have Light as the default theme - new DefaultThemeStateService(globalStateProvider, ThemeType.Light), - deps: [GlobalStateProvider], - }, - ], + // Do not register your dependency here! Add it to the typesafeProviders array using the helper function + providers: safeProviders, }) export class CoreModule { constructor(@Optional() @SkipSelf() parentModule?: CoreModule) { diff --git a/libs/angular/src/platform/utils/safe-provider.ts b/libs/angular/src/platform/utils/safe-provider.ts index 7c19a280d6..e7547f9b82 100644 --- a/libs/angular/src/platform/utils/safe-provider.ts +++ b/libs/angular/src/platform/utils/safe-provider.ts @@ -85,9 +85,25 @@ type SafeConcreteProvider< deps: D; }; +/** + * If useAngularDecorators: true is specified, do not require a deps array. + * This is a manual override for where @Injectable decorators are used + */ +type UseAngularDecorators = Omit & { + useAngularDecorators: true; +}; + +/** + * Represents a type with a deps array that may optionally be overridden with useAngularDecorators + */ +type AllowAngularDecorators = T | UseAngularDecorators; + /** * A factory function that creates a provider for the ngModule providers array. - * This guarantees type safety for your provider definition. It does nothing at runtime. + * This (almost) guarantees type safety for your provider definition. It does nothing at runtime. + * Warning: the useAngularDecorators option provides an override where your class uses the Injectable decorator, + * however this cannot be enforced by the type system and will not cause an error if the decorator is not used. + * @example safeProvider({ provide: MyService, useClass: DefaultMyService, deps: [AnotherService] }) * @param provider Your provider object in the usual shape (e.g. using useClass, useValue, useFactory, etc.) * @returns The exact same object without modification (pass-through). */ @@ -113,10 +129,10 @@ export const safeProvider = < DConcrete extends MapParametersToDeps>, >( provider: - | SafeClassProvider + | AllowAngularDecorators> | SafeValueProvider - | SafeFactoryProvider + | AllowAngularDecorators> | SafeExistingProvider - | SafeConcreteProvider + | AllowAngularDecorators> | Constructor, ): SafeProvider => provider as SafeProvider; diff --git a/libs/angular/src/platform/utils/safe-provider.type.spec.ts b/libs/angular/src/platform/utils/safe-provider.type.spec.ts new file mode 100644 index 0000000000..6fe6d0d0b6 --- /dev/null +++ b/libs/angular/src/platform/utils/safe-provider.type.spec.ts @@ -0,0 +1,111 @@ +/* eslint-disable @typescript-eslint/ban-ts-comment */ +// This rule bans @ts-expect-error comments without explanation. In this file, we use it to test our types, and +// explanation is provided in header comments before each test. + +import { safeProvider } from "./safe-provider"; + +class FooFactory { + create() { + return "thing"; + } +} + +abstract class FooService { + createFoo: (str: string) => string; +} + +class DefaultFooService implements FooService { + constructor(private factory: FooFactory) {} + + createFoo(str: string) { + return str ?? this.factory.create(); + } +} + +class BarFactory { + create() { + return 5; + } +} + +abstract class BarService { + createBar: (num: number) => number; +} + +class DefaultBarService implements BarService { + constructor(private factory: BarFactory) {} + + createBar(num: number) { + return num ?? this.factory.create(); + } +} + +abstract class FooBarService {} + +class DefaultFooBarService { + constructor( + private fooFactory: FooFactory, + private barFactory: BarFactory, + ) {} +} + +// useClass happy path with deps +safeProvider({ + provide: FooService, + useClass: DefaultFooService, + deps: [FooFactory], +}); + +// useClass happy path with useAngularDecorators +safeProvider({ + provide: FooService, + useClass: DefaultFooService, + useAngularDecorators: true, +}); + +// useClass: expect error if implementation does not match abstraction +safeProvider({ + provide: FooService, + // @ts-expect-error + useClass: DefaultBarService, + deps: [BarFactory], +}); + +// useClass: expect error if deps type does not match +safeProvider({ + provide: FooService, + useClass: DefaultFooService, + // @ts-expect-error + deps: [BarFactory], +}); + +// useClass: expect error if not enough deps specified +safeProvider({ + provide: FooService, + useClass: DefaultFooService, + // @ts-expect-error + deps: [], +}); + +// useClass: expect error if too many deps specified +safeProvider({ + provide: FooService, + useClass: DefaultFooService, + // @ts-expect-error + deps: [FooFactory, BarFactory], +}); + +// useClass: expect error if deps are in the wrong order +safeProvider({ + provide: FooBarService, + useClass: DefaultFooBarService, + // @ts-expect-error + deps: [BarFactory, FooFactory], +}); + +// useClass: expect error if no deps specified and not using Angular decorators +// @ts-expect-error +safeProvider({ + provide: FooService, + useClass: DefaultFooService, +}); diff --git a/libs/shared/jest.config.angular.js b/libs/shared/jest.config.angular.js index a0dcc27516..689a04d858 100644 --- a/libs/shared/jest.config.angular.js +++ b/libs/shared/jest.config.angular.js @@ -6,6 +6,11 @@ const { defaultTransformerOptions } = require("jest-preset-angular/presets"); module.exports = { testMatch: ["**/+(*.)+(spec).+(ts)"], + testPathIgnorePatterns: [ + "/node_modules/", // default value + ".*.type.spec.ts", // ignore type tests (which are checked at compile time and not run by jest) + ], + // Workaround for a memory leak that crashes tests in CI: // https://github.com/facebook/jest/issues/9430#issuecomment-1149882002 // Also anecdotally improves performance when run locally