Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

provide singletons #46

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 72 additions & 9 deletions packages/core/src/index.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,17 @@ 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);
});

it('#getSingleton should instantiate a singleton factory once', () => {
let app = new App();
let factoryFunc = () => app.getSingleton(Database);
let i = 0;
let factoryFunc = (_app: App) => ({ id: ++i });
app.provideSingleton(factoryFunc);
let db1 = app.getSingleton(factoryFunc);
let db2 = app.getSingleton(factoryFunc);
expect(db1.id).toEqual(db2.id);
Expand All @@ -42,21 +45,21 @@ 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;
}
},
);
class MyDB extends Database {
constructor(app: App) {
super(app);
dbDidInit = true;
}
}
app.load(MyDB);
expect(dbDidInit).toBe(true);
});
});

describe('Overrides', () => {
it('#getSingleton should use and respect singleton overrides', () => {
let app = new App();
app.provideSingleton(Database);
app.overrideSingleton(
Database,
class MockDb extends Database {
Expand Down Expand Up @@ -98,6 +101,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 {
Expand All @@ -112,13 +116,15 @@ 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);
});

it('#getSingleton should instantiate non-existing singletons in the child app, not parent', () => {
let parentApp = new App();
parentApp.provideSingleton(Database);
let childApp = parentApp.createChildApp();
let childDbId = childApp.getSingleton(Database).id;
expect(parentApp.getSingleton(Database).id !== childDbId);
Expand Down Expand Up @@ -174,3 +180,60 @@ describe('Context disposal', () => {
);
});
});

describe('providing dependencies', () => {
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
});

it('should throw when you require unprovided singletons', () => {
let app = new App();
class MySingleton extends AppSingleton {}
expect(() => app.requireSingleton(MySingleton)).toThrow();
});
});

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();
});
});
63 changes: 61 additions & 2 deletions packages/core/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,18 @@ export type PublicInterface<T> = { [K in keyof T]: T[K] };
*/
export class App {
private singletonLocator: Locator<this>;
private providedSingletons: WeakMap<ConstructorOrFactory<App, any>, 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);

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.
}
}

/**
Expand Down Expand Up @@ -102,6 +109,42 @@ 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<T>(Klass: ConstructorOrFactory<App, T>): void {
if (this.isSingletonProvided(Klass)) {
throw new Error(`The singleton ${Klass.name} is already provided.`);
}
this.providedSingletons.set(Klass, true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we refactor so the same map is used for provides and overrides?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on second thought maybe not, you might want to override before providing.

}

private isSingletonProvided<T>(Klass: ConstructorOrFactory<App, T>): boolean {
if (this.providedSingletons.has(Klass)) {
return true;
} else if (this.parentApp) {
return this.parentApp.isSingletonProvided(Klass);
} else {
return false;
}
}

/**
* Ensures that the singleton is provided; throws if it's not.
*
* Use this to detect unprovided but used singletons early.
*/
requireSingleton<T>(Klass: ConstructorOrFactory<App, T>): void {
if (!this.isSingletonProvided(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.
Expand All @@ -115,6 +158,12 @@ export class App {
if (this.hasSingleton(Klass)) {
return this.getExistingSingleton(Klass);
}
if (!this.isSingletonProvided(Klass)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe at locator level.

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

Expand Down Expand Up @@ -170,10 +219,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<T>(Klass: ConstructorOrFactory<App, T>): void {
this.getSingleton(Klass); // force initialization;
return;
if (this.isSingletonProvided(Klass)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case it should be just getSingleton (multiple loads should be ok)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to re-do this with a bit of thinking.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redo this PR or the loading-once-or-twice mechanics? Fine with both really.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to think whether we want to allow loading plugins within plugins, because thats what implies multiple load calls are OK.

throw new Error(`Singleton ${Klass.name} is already initialized.`);
} else {
this.provideSingleton(Klass);
this.getSingleton(Klass); // force initialization
}
}

/**
Expand Down Expand Up @@ -298,6 +353,10 @@ export class ServiceContext {
getSingleton<T>(SingletonClass: ConstructorOrFactory<App, T>): T {
return this.app.getSingleton(SingletonClass);
}

requireSingleton<T>(SingletonClass: ConstructorOrFactory<App, T>) {
return this.app.requireSingleton(SingletonClass);
}
}

type ContextListener = (serviceCtx: ServiceContext, error: Error | null) => PromiseLike<void>;
Expand Down