core-api: identify ApiRefs based on ID instead of reference

Co-authored-by: Juan Lulkin <jmaiz@spotify.com>
Signed-off-by: Patrik Oldsberg <poldsberg@gmail.com>

packages/core-api/src/apis/system/ApiFactoryRegistry.test.ts

@@ -67,4 +67,25 @@ describe('ApiFactoryRegistry', () => {
     expect(registry.get(cRef)).toBe(cFactory);
     expect(registry.getAllApis()).toEqual(new Set([aRef, bRef, cRef]));
   });
+
+  it('should identify ApiRefs by id but still return the correct factory ref when listing all apis', () => {
+    const ref1 = createApiRef<number>({ id: 'a', description: 'ref1' });
+    const ref2 = createApiRef<number>({ id: 'a', description: 'ref2' });
+
+    const factory1 = { api: ref1, deps: {}, factory: () => 3 };
+    const factory2 = { api: ref2, deps: {}, factory: () => 3 };
+
+    const registry = new ApiFactoryRegistry();
+    expect(registry.register('default', factory1)).toBe(true);
+    expect(registry.register('default', factory2)).toBe(false);
+    expect(registry.get(ref1)).toEqual(factory1);
+    expect(registry.get(ref2)).toEqual(factory1);
+    expect(registry.getAllApis()).toEqual(new Set([ref1]));
+
+    expect(registry.register('app', factory2)).toBe(true);
+    expect(registry.get(ref1)).toEqual(factory2);
+    expect(registry.get(ref2)).toEqual(factory2);
+    expect(registry.getAllApis()).toEqual(new Set([ref2]));
+    expect(registry.getAllApis()).not.toEqual(new Set([ref1]));
+  });
 });

packages/core-api/src/apis/system/ApiFactoryRegistry.ts

@@ -46,7 +46,7 @@ type FactoryTuple = {
  * higher priority scopes override ones with lower priority.
  */
 export class ApiFactoryRegistry implements ApiFactoryHolder {
-  private readonly factories = new Map<AnyApiRef, FactoryTuple>();
+  private readonly factories = new Map<string, FactoryTuple>();
 
   /**
    * Register a new API factory. Returns true if the factory was added
@@ -60,19 +60,19 @@ export class ApiFactoryRegistry implements ApiFactoryHolder {
     factory: ApiFactory<Api, Impl, Deps>,
   ) {
     const priority = ScopePriority[scope];
-    const existing = this.factories.get(factory.api);
+    const existing = this.factories.get(factory.api.id);
     if (existing && existing.priority >= priority) {
       return false;
     }
 
-    this.factories.set(factory.api, { priority, factory });
+    this.factories.set(factory.api.id, { priority, factory });
     return true;
   }
 
   get<T>(
     api: ApiRef<T>,
   ): ApiFactory<T, T, { [x: string]: unknown }> | undefined {
-    const tuple = this.factories.get(api);
+    const tuple = this.factories.get(api.id);
     if (!tuple) {
       return undefined;
     }
@@ -80,6 +80,10 @@ export class ApiFactoryRegistry implements ApiFactoryHolder {
   }
 
   getAllApis(): Set<AnyApiRef> {
-    return new Set(this.factories.keys());
+    const refs = new Set<AnyApiRef>();
+    for (const { factory } of this.factories.values()) {
+      refs.add(factory.api);
+    }
+    return refs;
   }
 }

packages/core-api/src/apis/system/ApiRegistry.test.ts

@@ -18,8 +18,9 @@ import { ApiRegistry } from './ApiRegistry';
 import { createApiRef } from './ApiRef';
 
 describe('ApiRegistry', () => {
-  const x1Ref = createApiRef<number>({ id: 'x', description: '' });
-  const x2Ref = createApiRef<string>({ id: 'x', description: '' });
+  const x1Ref = createApiRef<number>({ id: 'x1', description: '' });
+  const x1DuplicateRef = createApiRef<number>({ id: 'x1', description: '' });
+  const x2Ref = createApiRef<string>({ id: 'x2', description: '' });
 
   it('should be created', () => {
     const registry = ApiRegistry.from([]);
@@ -32,12 +33,14 @@ describe('ApiRegistry', () => {
       [x2Ref, 'y'],
     ]);
     expect(registry.get(x1Ref)).toBe(3);
+    expect(registry.get(x1DuplicateRef)).toBe(3);
     expect(registry.get(x2Ref)).toBe('y');
   });
 
   it('should be built', () => {
     const registry = ApiRegistry.builder().build();
     expect(registry.get(x1Ref)).toBe(undefined);
+    expect(registry.get(x1DuplicateRef)).toBe(undefined);
   });
 
   it('should be built with APIs', () => {
@@ -47,6 +50,7 @@ describe('ApiRegistry', () => {
 
     const registry = builder.build();
     expect(registry.get(x1Ref)).toBe(3);
+    expect(registry.get(x1DuplicateRef)).toBe(3);
     expect(registry.get(x2Ref)).toBe('y');
   });
 
@@ -55,6 +59,7 @@ describe('ApiRegistry', () => {
     const reg2 = reg1.with(x2Ref, 'y');
     const reg3 = reg2.with(x2Ref, 'z');
     const reg4 = reg3.with(x1Ref, 2);
+    const reg5 = reg3.with(x1DuplicateRef, 4);
 
     expect(reg1.get(x1Ref)).toBe(3);
     expect(reg1.get(x2Ref)).toBe(undefined);
@@ -64,5 +69,7 @@ describe('ApiRegistry', () => {
     expect(reg3.get(x2Ref)).toBe('z');
     expect(reg4.get(x1Ref)).toBe(2);
     expect(reg4.get(x2Ref)).toBe('z');
+    expect(reg5.get(x1Ref)).toBe(4);
+    expect(reg5.get(x2Ref)).toBe('z');
   });
 });

packages/core-api/src/apis/system/ApiRegistry.ts

@@ -19,10 +19,10 @@ import { ApiRef, ApiHolder } from './types';
 type ApiImpl<T = unknown> = readonly [ApiRef<T>, T];
 
 class ApiRegistryBuilder {
-  private apis: ApiImpl[] = [];
+  private apis: [string, unknown][] = [];
 
   add<T, I extends T>(api: ApiRef<T>, impl: I): I {
-    this.apis.push([api, impl]);
+    this.apis.push([api.id, impl]);
     return impl;
   }
 
@@ -38,7 +38,7 @@ export class ApiRegistry implements ApiHolder {
   }
 
   static from(apis: ApiImpl[]) {
-    return new ApiRegistry(new Map(apis));
+    return new ApiRegistry(new Map(apis.map(([api, impl]) => [api.id, impl])));
   }
 
   /**
@@ -48,10 +48,10 @@ export class ApiRegistry implements ApiHolder {
    * @param impl Implementation of the API to add
    */
   static with<T>(api: ApiRef<T>, impl: T): ApiRegistry {
-    return new ApiRegistry(new Map([[api, impl]]));
+    return new ApiRegistry(new Map([[api.id, impl]]));
   }
 
-  constructor(private readonly apis: Map<ApiRef<unknown>, unknown>) {}
+  constructor(private readonly apis: Map<string, unknown>) {}
 
   /**
    * Returns a new ApiRegistry with the provided API added to the existing ones.
@@ -60,10 +60,10 @@ export class ApiRegistry implements ApiHolder {
    * @param impl Implementation of the API to add
    */
   with<T>(api: ApiRef<T>, impl: T): ApiRegistry {
-    return new ApiRegistry(new Map([...this.apis, [api, impl]]));
+    return new ApiRegistry(new Map([...this.apis, [api.id, impl]]));
   }
 
   get<T>(api: ApiRef<T>): T | undefined {
-    return this.apis.get(api) as T | undefined;
+    return this.apis.get(api.id) as T | undefined;
   }
 }

packages/core-api/src/apis/system/ApiResolver.test.ts

@@ -19,8 +19,14 @@ import { createApiRef } from './ApiRef';
 import { ApiFactoryRegistry } from './ApiFactoryRegistry';
 
 const aRef = createApiRef<number>({ id: 'a', description: '' });
+const otherARef = createApiRef<number>({ id: 'a', description: 'other' });
 const bRef = createApiRef<string>({ id: 'b', description: '' });
+const otherBRef = createApiRef<string>({ id: 'b', description: 'other' });
 const cRef = createApiRef<{ x: string }>({ id: 'c', description: '' });
+const otherCRef = createApiRef<{ x: string }>({
+  id: 'c',
+  description: 'other',
+});
 
 function createRegistry() {
   const registry = new ApiFactoryRegistry();
@@ -36,28 +42,18 @@ function createRegistry() {
   });
   registry.register('default', {
     api: cRef,
-    deps: { b: bRef },
+    deps: { b: otherBRef },
     factory: ({ b }) => ({ x: 'x', b }),
   });
   return registry;
 }
 
-function createLongCyclicRegistry() {
+function createSelfCyclicRegistry() {
   const registry = new ApiFactoryRegistry();
   registry.register('default', {
     api: aRef,
-    deps: { b: bRef },
-    factory: () => 1,
-  });
-  registry.register('default', {
-    api: bRef,
-    deps: { c: cRef },
-    factory: () => 'b',
-  });
-  registry.register('default', {
-    api: cRef,
     deps: { a: aRef },
-    factory: () => ({ x: 'x' }),
+    factory: () => 1,
   });
   return registry;
 }
@@ -66,7 +62,37 @@ function createShortCyclicRegistry() {
   const registry = new ApiFactoryRegistry();
   registry.register('default', {
     api: aRef,
+    deps: { b: bRef },
+    factory: () => 1,
+  });
+  registry.register('default', {
+    api: bRef,
     deps: { a: aRef },
+    factory: () => 'x',
+  });
+  return registry;
+}
+
+function createShortCyclicRegistryWithOther() {
+  const registry = new ApiFactoryRegistry();
+  registry.register('default', {
+    api: aRef,
+    deps: { b: bRef },
+    factory: () => 1,
+  });
+  registry.register('default', {
+    api: otherBRef,
+    deps: { a: otherARef },
+    factory: () => 'x',
+  });
+  return registry;
+}
+
+function createLongCyclicRegistry() {
+  const registry = new ApiFactoryRegistry();
+  registry.register('default', {
+    api: aRef,
+    deps: { b: otherBRef },
     factory: () => 1,
   });
   registry.register('default', {
@@ -76,7 +102,7 @@ function createShortCyclicRegistry() {
   });
   registry.register('default', {
     api: cRef,
-    deps: { b: bRef },
+    deps: { a: aRef },
     factory: () => ({ x: 'x' }),
   });
   return registry;
@@ -87,15 +113,51 @@ describe('ApiResolver', () => {
     const resolver = new ApiResolver(new ApiFactoryRegistry());
     expect(resolver.get(aRef)).toBe(undefined);
     expect(resolver.get(bRef)).toBe(undefined);
+    expect(resolver.get(otherBRef)).toBe(undefined);
     expect(resolver.get(cRef)).toBe(undefined);
   });
 
   it('should instantiate APIs', () => {
     const resolver = new ApiResolver(createRegistry());
     expect(resolver.get(aRef)).toBe(1);
+    expect(resolver.get(otherARef)).toBe(1);
     expect(resolver.get(bRef)).toBe('b');
+    expect(resolver.get(otherBRef)).toBe('b');
     expect(resolver.get(cRef)).toEqual({ x: 'x', b: 'b' });
-    expect(resolver.get(cRef)).toBe(resolver.get(cRef));
+    expect(resolver.get(cRef)).toBe(resolver.get(otherCRef));
+  });
+
+  it('should detect self dependency cycles', () => {
+    const resolver = new ApiResolver(createSelfCyclicRegistry());
+    expect(() => resolver.get(aRef)).toThrow(
+      'Circular dependency of api factory for apiRef{a}',
+    );
+  });
+
+  it('should detect short dependency cycles', () => {
+    const resolver = new ApiResolver(createShortCyclicRegistry());
+    expect(() => resolver.get(aRef)).toThrow(
+      'Circular dependency of api factory for apiRef{a}',
+    );
+    expect(() => resolver.get(bRef)).toThrow(
+      'Circular dependency of api factory for apiRef{b}',
+    );
+  });
+
+  it('should detect short dependency cycles with other refs', () => {
+    const resolver = new ApiResolver(createShortCyclicRegistryWithOther());
+    expect(() => resolver.get(aRef)).toThrow(
+      'Circular dependency of api factory for apiRef{a}',
+    );
+    expect(() => resolver.get(bRef)).toThrow(
+      'Circular dependency of api factory for apiRef{b}',
+    );
+    expect(() => resolver.get(otherARef)).toThrow(
+      'Circular dependency of api factory for apiRef{a}',
+    );
+    expect(() => resolver.get(otherBRef)).toThrow(
+      'Circular dependency of api factory for apiRef{b}',
+    );
   });
 
   it('should detect long dependency cycles', () => {
@@ -110,17 +172,7 @@ describe('ApiResolver', () => {
     expect(() => resolver.get(bRef)).toThrow(
       'Circular dependency of api factory for apiRef{b}',
     );
-    expect(() => resolver.get(cRef)).toThrow(
-      'Circular dependency of api factory for apiRef{c}',
-    );
-  });
-
-  it('should detect short dependency cycles', () => {
-    const resolver = new ApiResolver(createShortCyclicRegistry());
-    expect(() => resolver.get(aRef)).toThrow(
-      'Circular dependency of api factory for apiRef{a}',
-    );
-    expect(() => resolver.get(bRef)).toThrow(
+    expect(() => resolver.get(otherBRef)).toThrow(
       'Circular dependency of api factory for apiRef{b}',
     );
     expect(() => resolver.get(cRef)).toThrow(
@@ -130,22 +182,54 @@ describe('ApiResolver', () => {
 
   it('should validate a factory holder', () => {
     expect(() => {
-      ApiResolver.validateFactories(createRegistry(), [aRef, bRef, cRef]);
+      ApiResolver.validateFactories(createRegistry(), [
+        aRef,
+        bRef,
+        otherBRef,
+        cRef,
+      ]);
     }).not.toThrow();
   });
 
+  it('should find self cycles with validation', () => {
+    const self = createSelfCyclicRegistry();
+    expect(() => ApiResolver.validateFactories(self, [aRef])).toThrow(
+      'Circular dependency of api factory for apiRef{a}',
+    );
+    expect(() => ApiResolver.validateFactories(self, [otherARef])).toThrow(
+      'Circular dependency of api factory for apiRef{a}',
+    );
+  });
+
   it('should find dependency cycles with validation', () => {
     const short = createShortCyclicRegistry();
-    expect(() =>
-      ApiResolver.validateFactories(short, short.getAllApis()),
-    ).toThrow('Circular dependency of api factory for apiRef{a}');
+    expect(() => ApiResolver.validateFactories(short, [aRef])).toThrow(
+      'Circular dependency of api factory for apiRef{a}',
+    );
+    expect(() => ApiResolver.validateFactories(short, [otherARef])).toThrow(
+      'Circular dependency of api factory for apiRef{a}',
+    );
     expect(() => ApiResolver.validateFactories(short, [bRef])).toThrow(
       'Circular dependency of api factory for apiRef{b}',
     );
-    expect(() => ApiResolver.validateFactories(short, [cRef])).toThrow(
-      'Circular dependency of api factory for apiRef{c}',
+    expect(() => ApiResolver.validateFactories(short, [otherBRef])).toThrow(
+      'Circular dependency of api factory for apiRef{b}',
     );
 
+    const shortOther = createShortCyclicRegistryWithOther();
+    expect(() => ApiResolver.validateFactories(shortOther, [aRef])).toThrow(
+      'Circular dependency of api factory for apiRef{a}',
+    );
+    expect(() =>
+      ApiResolver.validateFactories(shortOther, [otherARef]),
+    ).toThrow('Circular dependency of api factory for apiRef{a}');
+    expect(() => ApiResolver.validateFactories(shortOther, [bRef])).toThrow(
+      'Circular dependency of api factory for apiRef{b}',
+    );
+    expect(() =>
+      ApiResolver.validateFactories(shortOther, [otherBRef]),
+    ).toThrow('Circular dependency of api factory for apiRef{b}');
+
     const long = createLongCyclicRegistry();
     expect(() =>
       ApiResolver.validateFactories(long, long.getAllApis()),
@@ -153,6 +237,9 @@ describe('ApiResolver', () => {
     expect(() => ApiResolver.validateFactories(long, [bRef])).toThrow(
       'Circular dependency of api factory for apiRef{b}',
     );
+    expect(() => ApiResolver.validateFactories(long, [otherBRef])).toThrow(
+      'Circular dependency of api factory for apiRef{b}',
+    );
     expect(() => ApiResolver.validateFactories(long, [cRef])).toThrow(
       'Circular dependency of api factory for apiRef{c}',
     );
@@ -173,5 +260,7 @@ describe('ApiResolver', () => {
     expect(factory).toHaveBeenCalledTimes(1);
     expect(resolver.get(aRef)).toBe(2);
     expect(factory).toHaveBeenCalledTimes(1);
+    expect(resolver.get(otherARef)).toBe(2);
+    expect(factory).toHaveBeenCalledTimes(1);
   });
 });

packages/core-api/src/apis/system/ApiResolver.ts

@@ -23,8 +23,6 @@ import {
 } from './types';
 
 export class ApiResolver implements ApiHolder {
-  private readonly apis = new Map<AnyApiRef, unknown>();
-
   /**
    * Validate factories by making sure that each of the apis can be created
    * without hitting any circular dependencies.
@@ -45,7 +43,7 @@ export class ApiResolver implements ApiHolder {
         }
 
         for (const dep of Object.values(factory.deps)) {
-          if (dep === api) {
+          if (dep.id === api.id) {
             throw new Error(`Circular dependency of api factory for ${api}`);
           }
           if (!allDeps.has(dep)) {
@@ -57,6 +55,8 @@ export class ApiResolver implements ApiHolder {
     }
   }
 
+  private readonly apis = new Map<string, unknown>();
+
   constructor(private readonly factories: ApiFactoryHolder) {}
 
   get<T>(ref: ApiRef<T>): T | undefined {
@@ -64,7 +64,7 @@ export class ApiResolver implements ApiHolder {
   }
 
   private load<T>(ref: ApiRef<T>, loading: AnyApiRef[] = []): T | undefined {
-    const impl = this.apis.get(ref);
+    const impl = this.apis.get(ref.id);
     if (impl) {
       return impl as T;
     }
@@ -80,7 +80,7 @@ export class ApiResolver implements ApiHolder {
 
     const deps = this.loadDeps(ref, factory.deps, [...loading, factory.api]);
     const api = factory.factory(deps);
-    this.apis.set(ref, api);
+    this.apis.set(ref.id, api);
     return api as T;
   }