address comments

Author: Feiyang1

packages/component/src/component.ts

@@ -14,16 +14,16 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-import { InstantiationMode, InstanceFactory, ComponentType } from './types';
+import { InstantiationMode, InstanceFactory, ComponentType, Dictionary } from './types';
 
 export class Component<T = unknown> {
-  multipleInstances: boolean = false;
+  multipleInstances = false;
   /**
    * Properties to be added to the service namespace
    */
-  serviceProps: { [key: string]: unknown } = {};
+  serviceProps: Dictionary = {};
 
-  instantiationMode: InstantiationMode = InstantiationMode.LAZY;
+  instantiationMode = InstantiationMode.LAZY;
 
   /**
    *
@@ -32,9 +32,9 @@ export class Component<T = unknown> {
    * @param type whehter the service provided by the component is public or private
    */
   constructor(
-    public readonly name: string,
-    public readonly instanceFactory: InstanceFactory<T>,
-    public readonly type: ComponentType
+    readonly name: string,
+    readonly instanceFactory: InstanceFactory<T>,
+    readonly type: ComponentType
   ) {}
 
   setInstantiationMode(mode: InstantiationMode): this {
@@ -47,7 +47,7 @@ export class Component<T = unknown> {
     return this;
   }
 
-  setServiceProps(props: { [key: string]: unknown }): this {
+  setServiceProps(props: Dictionary): this {
     this.serviceProps = props;
     return this;
   }

packages/component/src/component_container.test.ts

@@ -38,7 +38,7 @@ describe('Component Container', () => {
     const provider1 = container.getProvider('ship');
     const provider2 = container.getProvider('ship');
 
-    expect(provider1).to.eq(provider2);
+    expect(provider1).to.equal(provider2);
   });
 
   it('calls setComponent() on provider with the same name when registering a component', () => {

packages/component/src/component_container.ts

@@ -19,16 +19,18 @@ import { Provider } from './provider';
 import { Component } from './component';
 
 export class ComponentContainer {
-  private providers = new Map<string, Provider>();
+  private readonly providers = new Map<string, Provider>();
 
-  constructor(private name: string) {}
+  constructor(private readonly name: string) {}
 
   /**
    *
    * @param component Component being added
    * @param overwrite When a component with the same name has already been registered,
-   * if overwrite is true: overwrite the existing component with the new component and create a new provider with the new component
-   * if overwrite is false: throw an expection
+   * if overwrite is true: overwrite the existing component with the new component and create a new 
+   * provider with the new component. It can be useful in tests where you want to use different mocks
+   * for different tests.
+   * if overwrite is false: throw an exception
    */
   addComponent(component: Component, overwrite = false): void {
     let provider = this.getProvider(component.name);
@@ -62,6 +64,6 @@ export class ComponentContainer {
   }
 
   getProviders(): Provider[] {
-    return Array.from(this.providers, entry => entry[1]);
+    return Array.from(this.providers.values());
   }
 }

packages/component/src/provider.test.ts

@@ -302,7 +302,7 @@ describe('Provider', () => {
         // eslint-disable-next-line @typescript-eslint/no-floating-promises
         provider.delete();
 
-        expect(deleteFakes.length).to.eq(2);
+        expect(deleteFakes.length).to.equal(2);
         for (const f of deleteFakes) {
           expect(f).to.have.been.called;
         }

packages/component/src/provider.ts

@@ -26,10 +26,10 @@ import { Component } from './component';
  */
 export class Provider<T = unknown> {
   private component: Component<T> | null = null;
-  private instances: Map<string, T> = new Map();
-  private instancesDeferred: Map<string, Deferred<T>> = new Map();
+  private readonly instances: Map<string, T> = new Map();
+  private readonly instancesDeferred: Map<string, Deferred<T>> = new Map();
 
-  constructor(private name: string, private container: ComponentContainer) {}
+  constructor(private readonly name: string, private readonly container: ComponentContainer) {}
 
   get(identifier: string = DEFAULT_ENTRY_NAME): Promise<T> {
     // if multipleInstances is not supported, use the default name
@@ -114,10 +114,10 @@ export class Provider<T = unknown> {
 
   // app.delete() will call this method on every provider to delete the services
   // TODO: should we mark the provider as deleted?
-  delete(): Promise<unknown[]> {
+  async delete(): Promise<void> {
     const services = Array.from(this.instances.values());
 
-    return Promise.all(
+    await Promise.all(
       services
         .filter(service => 'INTERNAL' in service)
         // eslint-disable-next-line @typescript-eslint/no-explicit-any

packages/component/src/types.ts

@@ -30,7 +30,7 @@ export const enum InstantiationMode {
  * PRIVATE: A private component provides a set of private APIs that are used internally by other
  * Firebase SDKs. No serivce namespace is created in `firebase` namespace and customers have no way to get them.
  */
-export enum ComponentType {
+export const enum ComponentType {
   PUBLIC = 'PUBLIC',
   PRIVATE = 'PRIVARE'
 }
@@ -47,3 +47,7 @@ export type InstanceFactory<T = unknown> = (
   container: ComponentContainer,
   instanceIdentifier?: string
 ) => T;
+
+export type Dictionary = {
+  [key: string]: unknown
+};