From b0d6c5fbce31fdd7d19310beb17f0b9521368a64 Mon Sep 17 00:00:00 2001 From: Andrej T Date: Sat, 22 Jun 2019 13:37:47 +0200 Subject: [PATCH 1/6] provide singletons --- packages/core/src/index.spec.ts | 35 ++++++++++++++++++++------- packages/core/src/index.ts | 43 +++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 9 deletions(-) diff --git a/packages/core/src/index.spec.ts b/packages/core/src/index.spec.ts index 81ce71e..36c3030 100644 --- a/packages/core/src/index.spec.ts +++ b/packages/core/src/index.spec.ts @@ -26,6 +26,7 @@ class UserProvider extends BaseService { describe('Instantiation', () => { it('#getSingleton should instantiate a singleton class once', () => { let app = new App(); + app.provideSingleton(Database); let db1 = app.getSingleton(Database); let db2 = app.getSingleton(Database); expect(db1.id).toEqual(db2.id); @@ -34,6 +35,8 @@ describe('Instantiation', () => { it('#getSingleton should instantiate a singleton factory once', () => { let app = new App(); let factoryFunc = () => app.getSingleton(Database); + app.provideSingleton(Database); + app.provideSingleton(factoryFunc); let db1 = app.getSingleton(factoryFunc); let db2 = app.getSingleton(factoryFunc); expect(db1.id).toEqual(db2.id); @@ -42,14 +45,15 @@ describe('Instantiation', () => { it('#load should force a singleton to instantitate', () => { let app = new App(); let dbDidInit: boolean = false; - app.load( - class MyDB extends Database { - constructor(app: App) { - super(app); - dbDidInit = true; - } - }, - ); + app.provideSingleton(Database); + class MyDB extends Database { + constructor(app: App) { + super(app); + dbDidInit = true; + } + } + app.provideSingleton(MyDB); + app.load(MyDB); expect(dbDidInit).toBe(true); }); }); @@ -57,6 +61,7 @@ describe('Instantiation', () => { describe('Overrides', () => { it('#getSingleton should use and respect singleton overrides', () => { let app = new App(); + app.provideSingleton(Database); app.overrideSingleton( Database, class MockDb extends Database { @@ -98,6 +103,7 @@ describe('Overrides', () => { it('#clearSingletonOverrides should cause original singletons to instantiate', () => { let app = new App(); + app.provideSingleton(Database); app.overrideSingleton( Database, class MockDb extends Database { @@ -112,6 +118,7 @@ describe('Overrides', () => { describe('App nesting', () => { it('#getSingleton should find instantiated singletons in a parent app', () => { let app = new App(); + app.provideSingleton(Database); let dbId = app.getSingleton(Database).id; let childApp = app.createChildApp(); expect(childApp.getSingleton(Database).id).toEqual(dbId); @@ -119,8 +126,10 @@ describe('App nesting', () => { it('#getSingleton should instantiate non-existing singletons in the child app, not parent', () => { let parentApp = new App(); + // childApp.provideSingleton(Database); // works if provided in parent app but... see below. let childApp = parentApp.createChildApp(); - let childDbId = childApp.getSingleton(Database).id; + childApp.provideSingleton(Database); + let childDbId = childApp.getSingleton(Database).id; // why does this throw??! expect(parentApp.getSingleton(Database).id !== childDbId); }); }); @@ -174,3 +183,11 @@ describe('Context disposal', () => { ); }); }); + +describe('providing dependencies', () => { + it('should throw when loading', () => { + let app = new App(); + class MySingleton extends AppSingleton {} + expect(() => app.getSingleton(MySingleton)).toThrowError(); + }); +}); diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index e86183f..735e1e8 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -46,11 +46,13 @@ export type PublicInterface = { [K in keyof T]: T[K] }; */ export class App { private singletonLocator: Locator; + private providedSingletons: WeakMap, boolean> = new WeakMap(); parentApp: App | null; constructor(opts: { parentApp?: App } = {}) { this.parentApp = opts.parentApp ? opts.parentApp : null; this.singletonLocator = new Locator(this, s => '__appSingleton' in s); + this.provideSingleton(ServiceContextEvents); // otherwise, withServiceContext fails. } /** @@ -102,6 +104,37 @@ export class App { } } + /** + * Registers a singleton as "provided". It's informing the application that a plugin + * agreed to expose that singleton. + * + * Calls to app.getSingleton(UnprovidedService) will fail with an error. + * + * Also see: `App#requireSingleton` + */ + provideSingleton(Klass: ConstructorOrFactory): void { + if (this.providedSingletons.has(Klass)) { + throw new Error(`The singleton ${Klass.name} is already provided.`); + } + this.providedSingletons.set(Klass, true); + } + + private isSingletonProvided(Klass: ConstructorOrFactory): boolean { + if (this.providedSingletons.has(Klass)) { + return true; + } else if (this.parentApp) { + return this.parentApp.isSingletonProvided(Klass); + } else { + return false; + } + } + + requireSingleton(Klass: ConstructorOrFactory): void { + if (!this.providedSingletons.has(Klass)) { + throw new Error(`The singleton ${Klass} is required, but wasnt provided.`); + } + } + /** * Returns an instance of the singleton, if it exists somewhere here or * in some of the parent apps. If it doesn't it's created in this app. @@ -115,6 +148,12 @@ export class App { if (this.hasSingleton(Klass)) { return this.getExistingSingleton(Klass); } + if (!this.isSingletonProvided(Klass)) { + throw new Error(`Singleton ${Klass.name} wasnt provided`); + // console.warn(`The singleton ${Klass} was constructed, but wasn't provided beforehand.`); + // console.warn(`Please provide it explicitly using "App#provideSingleton(${Klass})".`); + // console.warn('In the future, this will be an error.'); + } return this.singletonLocator.get(Klass); } @@ -298,6 +337,10 @@ export class ServiceContext { getSingleton(SingletonClass: ConstructorOrFactory): T { return this.app.getSingleton(SingletonClass); } + + requireSingleton(SingletonClass: ConstructorOrFactory) { + return this.app.requireSingleton(SingletonClass); + } } type ContextListener = (serviceCtx: ServiceContext, error: Error | null) => PromiseLike; From b87ccf9d6bd0c3c34fd0c9de62a2cf3b30eeb8e0 Mon Sep 17 00:00:00 2001 From: Andrej T Date: Sat, 22 Jun 2019 13:40:01 +0200 Subject: [PATCH 2/6] parent, not child --- packages/core/src/index.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/index.spec.ts b/packages/core/src/index.spec.ts index 36c3030..c67f8f0 100644 --- a/packages/core/src/index.spec.ts +++ b/packages/core/src/index.spec.ts @@ -126,7 +126,7 @@ describe('App nesting', () => { it('#getSingleton should instantiate non-existing singletons in the child app, not parent', () => { let parentApp = new App(); - // childApp.provideSingleton(Database); // works if provided in parent app but... see below. + // parentApp.provideSingleton(Database); // works if provided in parent app but... see below. let childApp = parentApp.createChildApp(); childApp.provideSingleton(Database); let childDbId = childApp.getSingleton(Database).id; // why does this throw??! From 9441f1024ab02da41aeb310981ae2af826c23376 Mon Sep 17 00:00:00 2001 From: Andrej T Date: Sat, 22 Jun 2019 14:03:16 +0200 Subject: [PATCH 3/6] all tests pass --- packages/core/src/index.spec.ts | 5 ++--- packages/core/src/index.ts | 9 +++++++-- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/packages/core/src/index.spec.ts b/packages/core/src/index.spec.ts index c67f8f0..dd2d27b 100644 --- a/packages/core/src/index.spec.ts +++ b/packages/core/src/index.spec.ts @@ -126,10 +126,9 @@ describe('App nesting', () => { it('#getSingleton should instantiate non-existing singletons in the child app, not parent', () => { let parentApp = new App(); - // parentApp.provideSingleton(Database); // works if provided in parent app but... see below. + parentApp.provideSingleton(Database); let childApp = parentApp.createChildApp(); - childApp.provideSingleton(Database); - let childDbId = childApp.getSingleton(Database).id; // why does this throw??! + let childDbId = childApp.getSingleton(Database).id; expect(parentApp.getSingleton(Database).id !== childDbId); }); }); diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 735e1e8..9c6d5ec 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -52,7 +52,12 @@ export class App { constructor(opts: { parentApp?: App } = {}) { this.parentApp = opts.parentApp ? opts.parentApp : null; this.singletonLocator = new Locator(this, s => '__appSingleton' in s); - this.provideSingleton(ServiceContextEvents); // otherwise, withServiceContext fails. + + if (!opts.parentApp) { + // we must provide this, otherwise withServiceContext will fail every time. + // we do it once, on the parent app, because child app construction will fail otherwise. + this.provideSingleton(ServiceContextEvents); // otherwise, withServiceContext fails. + } } /** @@ -113,7 +118,7 @@ export class App { * Also see: `App#requireSingleton` */ provideSingleton(Klass: ConstructorOrFactory): void { - if (this.providedSingletons.has(Klass)) { + if (this.isSingletonProvided(Klass)) { throw new Error(`The singleton ${Klass.name} is already provided.`); } this.providedSingletons.set(Klass, true); From 471c9eabeeceeea4665a1e307f70c277fa81aa85 Mon Sep 17 00:00:00 2001 From: Andrej T Date: Sat, 22 Jun 2019 14:38:28 +0200 Subject: [PATCH 4/6] App#load now auto-provides the plugin fn; added and adapted tests --- packages/core/src/index.spec.ts | 51 +++++++++++++++++++++++++++++---- packages/core/src/index.ts | 10 +++++-- 2 files changed, 54 insertions(+), 7 deletions(-) diff --git a/packages/core/src/index.spec.ts b/packages/core/src/index.spec.ts index dd2d27b..c18018b 100644 --- a/packages/core/src/index.spec.ts +++ b/packages/core/src/index.spec.ts @@ -34,8 +34,8 @@ describe('Instantiation', () => { it('#getSingleton should instantiate a singleton factory once', () => { let app = new App(); - let factoryFunc = () => app.getSingleton(Database); - app.provideSingleton(Database); + let i = 0; + let factoryFunc = (_app: App) => ({ id: ++i }); app.provideSingleton(factoryFunc); let db1 = app.getSingleton(factoryFunc); let db2 = app.getSingleton(factoryFunc); @@ -45,14 +45,12 @@ describe('Instantiation', () => { it('#load should force a singleton to instantitate', () => { let app = new App(); let dbDidInit: boolean = false; - app.provideSingleton(Database); class MyDB extends Database { constructor(app: App) { super(app); dbDidInit = true; } } - app.provideSingleton(MyDB); app.load(MyDB); expect(dbDidInit).toBe(true); }); @@ -184,9 +182,52 @@ describe('Context disposal', () => { }); describe('providing dependencies', () => { - it('should throw when loading', () => { + it('should throw when getting singletons that arent provided', () => { let app = new App(); class MySingleton extends AppSingleton {} expect(() => app.getSingleton(MySingleton)).toThrowError(); }); + + it('should not throw when getting provided singletons', () => { + let app = new App(); + class MySingleton extends AppSingleton {} + let mySingletonModule = (app: App) => app.provideSingleton(MySingleton); + app.load(mySingletonModule); + app.getSingleton(MySingleton); // shouldn't throw + }); + + it('should throw when you provide singletons twice', () => { + let app = new App(); + class MySingleton extends AppSingleton {} + app.provideSingleton(MySingleton); + expect(() => app.provideSingleton(MySingleton)).toThrow(); + }); + + it('dependencies provided in child apps shouldnt affect parent apps', () => { + let app = new App(); + let childApp = app.createChildApp(); + class MySingleton extends AppSingleton {} + let myPlugin = (app: App) => { + app.provideSingleton(MySingleton); + }; + childApp.load(myPlugin); + expect(() => app.getSingleton(MySingleton)).toThrow(); + }); + + it('dependencies provided in the parent should be present in child apps', () => { + let app = new App(); + class MySingleton extends AppSingleton {} + app.provideSingleton(MySingleton); + let childApp = app.createChildApp(); + childApp.getSingleton(MySingleton); // should not throw + }); +}); + +describe('loading plugins', () => { + it('should throw when loading plugins twice', () => { + let app = new App(); + let myPlugin = (app: App) => app.provideSingleton(class MySingleton extends AppSingleton {}); + app.load(myPlugin); + expect(() => app.load(myPlugin)).toThrow(); + }); }); diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 9c6d5ec..d5ad2ae 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -214,10 +214,16 @@ export class App { * While singleton classes are typically side effect free and can be instantiated lazily when * first requested, plugins have side-effects, such as adding router routes, adding RPC endpoints * or setting up event listeners. The load method is therefore used to load those plugins. + * + * You can load a plugin only once; load throws an error the second time. */ load(Klass: ConstructorOrFactory): void { - this.getSingleton(Klass); // force initialization; - return; + if (this.isSingletonProvided(Klass)) { + throw new Error(`Singleton ${Klass.name} is already initialized.`); + } else { + this.provideSingleton(Klass); + this.getSingleton(Klass); // force initialization + } } /** From a4ae359a660784839f00a27520c88f0f2a949f3d Mon Sep 17 00:00:00 2001 From: Andrej T Date: Sat, 22 Jun 2019 15:22:14 +0200 Subject: [PATCH 5/6] requireSingleton now checks recursively for provided presence --- packages/core/src/index.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index d5ad2ae..24d09a5 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -134,8 +134,13 @@ export class App { } } + /** + * Ensures that the singleton is provided; throws if it's not. + * + * Use this to detect unprovided but used singletons early. + */ requireSingleton(Klass: ConstructorOrFactory): void { - if (!this.providedSingletons.has(Klass)) { + if (!this.isSingletonProvided(Klass)) { throw new Error(`The singleton ${Klass} is required, but wasnt provided.`); } } From 71d67b23567ba06ad83992d6a482d3d93aefec54 Mon Sep 17 00:00:00 2001 From: Andrej T Date: Sat, 22 Jun 2019 15:22:25 +0200 Subject: [PATCH 6/6] test for requireSingleton --- packages/core/src/index.spec.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/core/src/index.spec.ts b/packages/core/src/index.spec.ts index c18018b..9397480 100644 --- a/packages/core/src/index.spec.ts +++ b/packages/core/src/index.spec.ts @@ -221,6 +221,12 @@ describe('providing dependencies', () => { let childApp = app.createChildApp(); childApp.getSingleton(MySingleton); // should not throw }); + + it('should throw when you require unprovided singletons', () => { + let app = new App(); + class MySingleton extends AppSingleton {} + expect(() => app.requireSingleton(MySingleton)).toThrow(); + }); }); describe('loading plugins', () => {