Skip to content

feature(core): App-level configuration method #607

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.

Sign up for GitHub

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

Closed
kjdelisle wants to merge 1 commit into from
Closed

feature(core): App-level configuration method #607

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
32 changes: 32 additions & 0 deletions packages/core/src/application.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ export class Application extends Context {
// Make options available to other modules as well.
this.bind(CoreBindings.APPLICATION_CONFIG).to(options);

this.config(options);

if (options.components) {
for (const component of options.components) {
this.component(component);
Expand Down Expand Up @@ -161,6 +163,36 @@ export class Application extends Context {
);
}

// tslint:disable-next-line:no-any
private config(cfg: {[key: string]: any}) {
for (const key in cfg) {
const item = cfg[key];
// Iterate through the keys one level down (not recursively!)
if (item instanceof Object) {
for (const subkey in item) {
this.bindItemOrClass(`${key}.${subkey}`, item[subkey]);
}
}
this.bindItemOrClass(key, item);
Copy link
Contributor

@raymondfeng raymondfeng Oct 2, 2017

Choose a reason for hiding this comment

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

Do we want to add a namespace to avoid possible conflicts with other binding providers? For example, app.config.<key>?

Copy link
Contributor Author

@kjdelisle kjdelisle Oct 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

No, in fact, one of the awesome parts about this is that it can be used as your one-stop shop for wiring up almost everything* if you want; the unit test actually demonstrates using this to wire up a named server instance.

Copy link
Contributor Author

@kjdelisle kjdelisle Oct 2, 2017

Choose a reason for hiding this comment

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

(Which will also require no code changes at this end when we refactor for multiple server support!)

Copy link
Contributor Author

@kjdelisle kjdelisle Oct 2, 2017

Choose a reason for hiding this comment

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

The test should probably change when we do that, though, but it'll end up being a part of that PR, if this lands first.

}
}
/**
* Helper function for either binding an item or a class, depending.
* @private
* @param key The key of the configuration element.
* @param itemOrClass The item or class to bind.
*/
// tslint:disable-next-line:no-any
private bindItemOrClass(key: string, itemOrClass: any) {
if (itemOrClass instanceof Function) {
this.bind(key)
.toClass(itemOrClass)
.inScope(BindingScope.SINGLETON);
Copy link
Member

@bajtos bajtos Oct 3, 2017

Choose a reason for hiding this comment

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

I am concerned this convention is very limiting. What if the configuration is expecting a regular function that should be bound via this.bind.to(fn)?

const config = {
  LoggerComponent: {
    writer: (msg: string) => saveLogToBackendViaHttpCall(msg)
  }
}

Copy link
Contributor Author

@kjdelisle kjdelisle Oct 3, 2017

Choose a reason for hiding this comment

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

What would be the best way to distinguish between a constructor and a regular function here? Additional checks for properties like .constructor?

Copy link
Member

@bajtos bajtos Oct 5, 2017

Choose a reason for hiding this comment

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

To my best knowledge, there is no way how to distinguish between a class constructor and a regular function, especially if the class constructor is using ES5 style.

Actually, if we are ok to enforce only ES6 class constructors, then it may be possible to detect classes by checking that fn.toString() starts with class keyword. Seee http://www.ecma-international.org/ecma-262/6.0/#sec-function.prototype.tostring.

$ node
> (class Foo {}).toString()
'class Foo {}'

However, are we sure that we are never going to want a configuration option that's expecting a class constructor instead of a class instance? I can imagine that for example an authentication component may want to receive custom User and AccessToken models (class constructors) in the configuration.

My point is that at framework level, it's not possible to reliable detect whether the user wants to bind function/class constructor via .to() or via .toClass() (or even .toDynamicValue()), we need the user to explicitly tell us their intent.

} else {
this.bind(key).to(itemOrClass);
}
}

/**
* Add a component to this application.
*
Expand Down
27 changes: 27 additions & 0 deletions packages/core/test/unit/application.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,33 @@ describe('Application', () => {
});

describe('configuration', () => {
it('allows bindings to be set via config method', async () => {
// tslint:disable-next-line:no-any
const samoflange: any = {
vertices: 'many',
usefulness: 0,
};
const app = new Application({
magicCode: 'foobar',
servers: {
abc123: FakeServer,
},
CustomComponentCo: {
samoflange: samoflange,
},
});
const code = await app.get('magicCode');
const server = (await app.getServer('abc123')) as FakeServer;
expect(code).to.equal('foobar');
expect(server.constructor.name).to.equal(FakeServer.name);
const samo = await app.get('CustomComponentCo.samoflange');
for (const key in samo) {
expect(samo[key]).to.equal(samoflange[key]);
}
const vertices = await app.get('CustomComponentCo.samoflange#vertices');
Copy link
Contributor

@virkt25 virkt25 Oct 2, 2017

Choose a reason for hiding this comment

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

I think CustomComponentCo.samoflange.vertices would be better :P

expect(vertices).to.equal('many');
});

it('allows servers to be provided via config', async () => {
const name = 'abc123';
const app = new Application({
Expand Down