[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

@@ -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: [
+  // Do not register your dependency here! Add it to the typesafeProviders array using the helper function
-    InitService,
+  providers: safeProviders,
-    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],
-    },
-  ],
 })
 export class CoreModule {
   constructor(@Optional() @SkipSelf() parentModule?: CoreModule) {

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<T extends { deps: any }> = Omit<T, "deps"> & {
+  useAngularDecorators: true;
+};
+
+/**
+ * Represents a type with a deps array that may optionally be overridden with useAngularDecorators
+ */
+type AllowAngularDecorators<T extends { deps: any }> = T | UseAngularDecorators<T>;
+
 /**
  * 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<ConstructorParameters<IConcrete>>,
 >(
   provider:
-    | SafeClassProvider<AClass, IClass, DClass>
+    | AllowAngularDecorators<SafeClassProvider<AClass, IClass, DClass>>
     | SafeValueProvider<AValue, VValue>
-    | SafeFactoryProvider<AFactory, IFactory, DFactory>
+    | AllowAngularDecorators<SafeFactoryProvider<AFactory, IFactory, DFactory>>
     | SafeExistingProvider<AExisting, IExisting>
-    | SafeConcreteProvider<IConcrete, DConcrete>
+    | AllowAngularDecorators<SafeConcreteProvider<IConcrete, DConcrete>>
     | Constructor<unknown>,
 ): SafeProvider => provider as SafeProvider;

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,
+});

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