From 7eb1add9c8c91df6677c664983844c4ff83e4e1f Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Tue, 14 May 2019 11:33:50 -0700 Subject: [PATCH] chore(context): address review comments (05/14/2019) --- docs/site/Context.md | 6 ++ .../acceptance/binding-config.acceptance.ts | 13 +++- .../class-level-bindings.acceptance.ts | 10 +-- .../src/__tests__/unit/context-config.unit.ts | 25 +++++--- packages/context/src/binding-config.ts | 2 +- packages/context/src/binding-key.ts | 8 ++- packages/context/src/binding.ts | 27 +++++++- packages/context/src/context-view.ts | 7 +-- packages/context/src/context.ts | 10 ++- packages/context/src/inject-config.ts | 63 ++++++++++--------- packages/context/src/keys.ts | 5 ++ 11 files changed, 114 insertions(+), 62 deletions(-) diff --git a/docs/site/Context.md b/docs/site/Context.md index 1025b178d299..38fe0773b31b 100644 --- a/docs/site/Context.md +++ b/docs/site/Context.md @@ -610,6 +610,12 @@ appCtx.bind('servers.RestServer.server2').toClass(RestServer); appCtx.configure('servers.RestServer.server2').to({protocol: 'http', port: 80}); ``` +Please note that `@config.*` is different from `@inject.*` as `@config.*` +injects configuration based on the current binding where `@config.*` is applied. +No hard-coded binding key is needed. The `@config.*` also allows the same class +such as `RestServer` to be bound to different keys with different configurations +as illustrated in the code snippet above. + ### Allow configuration to be changed dynamically Some configurations are designed to be changeable dynamically, for example, the diff --git a/packages/context/src/__tests__/acceptance/binding-config.acceptance.ts b/packages/context/src/__tests__/acceptance/binding-config.acceptance.ts index 6a1b98409028..4960a88291c4 100644 --- a/packages/context/src/__tests__/acceptance/binding-config.acceptance.ts +++ b/packages/context/src/__tests__/acceptance/binding-config.acceptance.ts @@ -4,7 +4,7 @@ // License text available at https://opensource.org/licenses/MIT import {expect} from '@loopback/testlab'; -import {config, configBindingKeyFor, Context, ContextView} from '../..'; +import {config, configBindingKeyFor, Context, ContextView, Getter} from '../..'; describe('Context bindings - injecting configuration for bound artifacts', () => { let ctx: Context; @@ -83,7 +83,7 @@ describe('Context bindings - injecting configuration for bound artifacts', () => class Logger { constructor( @config.getter() - public configGetter: () => Promise, + public configGetter: Getter, ) {} } @@ -98,10 +98,17 @@ describe('Context bindings - injecting configuration for bound artifacts', () => expect(configObj).to.eql({level: 'INFO'}); // Update logger configuration - ctx.configure(LOGGER_KEY).to({level: 'DEBUG'}); + const configBinding = ctx.configure(LOGGER_KEY).to({level: 'DEBUG'}); configObj = await logger.configGetter(); expect(configObj).to.eql({level: 'DEBUG'}); + + // Now remove the logger configuration + ctx.unbind(configBinding.key); + + // configGetter returns undefined as config is optional by default + configObj = await logger.configGetter(); + expect(configObj).to.be.undefined(); }); it('injects a view to access config', async () => { diff --git a/packages/context/src/__tests__/acceptance/class-level-bindings.acceptance.ts b/packages/context/src/__tests__/acceptance/class-level-bindings.acceptance.ts index c2c87654b5e5..32ccacc4722d 100644 --- a/packages/context/src/__tests__/acceptance/class-level-bindings.acceptance.ts +++ b/packages/context/src/__tests__/acceptance/class-level-bindings.acceptance.ts @@ -769,7 +769,7 @@ describe('Context bindings - Injecting dependencies of classes', () => { expect(store.optionXY).to.eql('y'); }); - it('injects config if the binding key is not present', () => { + it('injects config if the configPath is not present', () => { class Store { constructor(@config() public configObj: object) {} } @@ -780,7 +780,7 @@ describe('Context bindings - Injecting dependencies of classes', () => { expect(store.configObj).to.eql({x: 1, y: 'a'}); }); - it("injects config if the binding key is ''", () => { + it("injects config if the configPath is ''", () => { class Store { constructor(@config('') public configObj: object) {} } @@ -791,7 +791,7 @@ describe('Context bindings - Injecting dependencies of classes', () => { expect(store.configObj).to.eql({x: 1, y: 'a'}); }); - it('injects config if the binding key is a path', () => { + it('injects config with configPath', () => { class Store { constructor(@config('x') public optionX: number) {} } @@ -802,7 +802,7 @@ describe('Context bindings - Injecting dependencies of classes', () => { expect(store.optionX).to.eql(1); }); - it('injects undefined option if key not found', () => { + it('injects undefined option if configPath not found', () => { class Store { constructor(@config('not-exist') public option: string | undefined) {} } @@ -813,7 +813,7 @@ describe('Context bindings - Injecting dependencies of classes', () => { expect(store.option).to.be.undefined(); }); - it('injects a config property based on the parent binding', async () => { + it('injects a config property for different bindings with the same class', async () => { class Store { constructor( @config('x') public optionX: number, diff --git a/packages/context/src/__tests__/unit/context-config.unit.ts b/packages/context/src/__tests__/unit/context-config.unit.ts index 0f453e8dcafa..a601f202108c 100644 --- a/packages/context/src/__tests__/unit/context-config.unit.ts +++ b/packages/context/src/__tests__/unit/context-config.unit.ts @@ -9,6 +9,7 @@ import { BindingKey, ConfigurationResolver, Context, + ContextTags, DefaultConfigurationResolver, ResolutionOptions, ValueOrPromise, @@ -32,7 +33,9 @@ describe('Context binding configuration', () => { expect(bindingForConfig.key).to.equal( BindingKey.buildKeyForConfig('foo'), ); - expect(bindingForConfig.tagMap).to.eql({config: 'foo'}); + expect(bindingForConfig.tagMap).to.eql({ + [ContextTags.CONFIGURATION_OF]: 'foo', + }); }); it('configures options for a binding after it is bound', () => { @@ -41,7 +44,9 @@ describe('Context binding configuration', () => { expect(bindingForConfig.key).to.equal( BindingKey.buildKeyForConfig('foo'), ); - expect(bindingForConfig.tagMap).to.eql({config: 'foo'}); + expect(bindingForConfig.tagMap).to.eql({ + [ContextTags.CONFIGURATION_OF]: 'foo', + }); }); }); @@ -51,7 +56,7 @@ describe('Context binding configuration', () => { expect(await ctx.getConfig('foo')).to.eql({x: 1}); }); - it('gets local config for a binding', async () => { + it('gets config for a binding with configPath', async () => { ctx .configure('foo') .toDynamicValue(() => Promise.resolve({a: {x: 0, y: 0}})); @@ -86,10 +91,16 @@ describe('Context binding configuration', () => { expect(ctx.getConfigSync('foo')).to.eql({x: 1}); }); + it('gets config for a binding with configPath', () => { + ctx.configure('foo').to({x: 1}); + expect(ctx.getConfigSync('foo', 'x')).to.eql(1); + expect(ctx.getConfigSync('foo', 'y')).to.be.undefined(); + }); + it('throws a helpful error when the config is async', () => { ctx.configure('foo').toDynamicValue(() => Promise.resolve('bar')); expect(() => ctx.getConfigSync('foo')).to.throw( - /Cannot get config\[\] for foo synchronously: the value is a promise/, + /Cannot get config for foo synchronously: the value is a promise/, ); }); }); @@ -101,7 +112,7 @@ describe('Context binding configuration', () => { configPath?: string, resolutionOptions?: ResolutionOptions, ): ValueOrPromise { - return ((key + '.config') as unknown) as ConfigValueType; + return (`Dummy config for ${key}` as unknown) as ConfigValueType; } } it('gets default resolver', () => { @@ -112,7 +123,7 @@ describe('Context binding configuration', () => { it('allows custom resolver', () => { ctx.configResolver = new MyConfigResolver(); const config = ctx.getConfigSync('xyz'); - expect(config).to.equal('xyz.config'); + expect(config).to.equal('Dummy config for xyz'); }); it('allows custom resolver bound to the context', () => { @@ -120,7 +131,7 @@ describe('Context binding configuration', () => { .bind(`${BindingKey.CONFIG_NAMESPACE}.resolver`) .toClass(MyConfigResolver); const config = ctx.getConfigSync('xyz'); - expect(config).to.equal('xyz.config'); + expect(config).to.equal('Dummy config for xyz'); expect(ctx.configResolver).to.be.instanceOf(MyConfigResolver); }); }); diff --git a/packages/context/src/binding-config.ts b/packages/context/src/binding-config.ts index 03eeaea92215..eb3d94c8f87d 100644 --- a/packages/context/src/binding-config.ts +++ b/packages/context/src/binding-config.ts @@ -62,7 +62,7 @@ export function configBindingKeyFor( configPath?: string, ) { return BindingKey.create( - BindingKey.buildKeyForConfig(key), + BindingKey.buildKeyForConfig(key).toString(), configPath, ); } diff --git a/packages/context/src/binding-key.ts b/packages/context/src/binding-key.ts index f15458045a7d..85eabbbb2022 100644 --- a/packages/context/src/binding-key.ts +++ b/packages/context/src/binding-key.ts @@ -104,14 +104,18 @@ export class BindingKey { ); } + /** + * Name space for configuration binding keys + */ static CONFIG_NAMESPACE = '$config'; + /** * Build a binding key for the configuration of the given binding. * The format is `:$config` * - * @param key The binding key that accepts the configuration + * @param key Key of the target binding to be configured */ - static buildKeyForConfig(key: BindingAddress = '') { + static buildKeyForConfig(key: BindingAddress = ''): BindingAddress { const suffix = BindingKey.CONFIG_NAMESPACE; const bindingKey = key ? `${key}:${suffix}` : suffix; return bindingKey; diff --git a/packages/context/src/binding.ts b/packages/context/src/binding.ts index f761a5715a8d..42f9bd3ca9e3 100644 --- a/packages/context/src/binding.ts +++ b/packages/context/src/binding.ts @@ -7,6 +7,7 @@ import * as debugFactory from 'debug'; import {BindingAddress, BindingKey} from './binding-key'; import {Context} from './context'; import {createProxyWithInterceptors} from './interception-proxy'; +import {ContextTags} from './keys'; import {Provider} from './provider'; import { asResolutionOptions, @@ -187,9 +188,9 @@ export class Binding { return this._valueConstructor; } - constructor(key: string, public isLocked: boolean = false) { + constructor(key: BindingAddress, public isLocked: boolean = false) { BindingKey.validate(key); - this.key = key; + this.key = key.toString(); } /** @@ -596,7 +597,27 @@ export class Binding { * @param key Binding key */ static bind(key: BindingAddress): Binding { - return new Binding(key.toString()); + return new Binding(key); + } + + /** + * Create a configuration binding for the given key + * + * @example + * ```ts + * const configBinding = Binding.configure('servers.RestServer.server1') + * .to({port: 3000}); + * ``` + * + * @typeparam T Generic type for the configuration value (not the binding to + * be configured) + * + * @param key Key for the binding to be configured + */ + static configure(key: BindingAddress): Binding { + return new Binding(BindingKey.buildKeyForConfig(key)).tag({ + [ContextTags.CONFIGURATION_OF]: key.toString(), + }); } } diff --git a/packages/context/src/context-view.ts b/packages/context/src/context-view.ts index 24352e1d7958..d4e10a93f09c 100644 --- a/packages/context/src/context-view.ts +++ b/packages/context/src/context-view.ts @@ -165,10 +165,9 @@ export class ContextView extends EventEmitter const values = await this.values(session); if (values.length === 0) return undefined; if (values.length === 1) return values[0]; - else - throw new Error( - 'The ContextView has more than one values. Use values() to access them.', - ); + throw new Error( + 'The ContextView has more than one values. Use values() to access them.', + ); } } diff --git a/packages/context/src/context.ts b/packages/context/src/context.ts index 16ef4bfb2a2b..006ea24437ad 100644 --- a/packages/context/src/context.ts +++ b/packages/context/src/context.ts @@ -374,10 +374,8 @@ export class Context extends EventEmitter { configure( key: BindingAddress = '', ): Binding { - const keyForConfig = BindingKey.buildKeyForConfig(key); - const bindingForConfig = this.bind(keyForConfig).tag({ - config: key, - }); + const bindingForConfig = Binding.configure(key); + this.add(bindingForConfig); return bindingForConfig; } @@ -468,9 +466,9 @@ export class Context extends EventEmitter { resolutionOptions, ); if (isPromiseLike(valueOrPromise)) { + const prop = configPath ? ` property ${configPath}` : ''; throw new Error( - `Cannot get config[${configPath || - ''}] for ${key} synchronously: the value is a promise`, + `Cannot get config${prop} for ${key} synchronously: the value is a promise`, ); } return valueOrPromise; diff --git a/packages/context/src/inject-config.ts b/packages/context/src/inject-config.ts index 41c5b4801d0f..aa1057d9d71b 100644 --- a/packages/context/src/inject-config.ts +++ b/packages/context/src/inject-config.ts @@ -12,37 +12,38 @@ import {ResolutionSession} from './resolution-session'; import {getDeepProperty, ValueOrPromise} from './value-promise'; /** - * Inject a property from `config` of the current binding. If no corresponding - * config value is present, `undefined` will be injected. - * - * @example - * ```ts - * class Store { - * constructor( - * @config('x') public optionX: number, - * @config('y') public optionY: string, - * ) { } - * } - * - * ctx.configure('store1', { x: 1, y: 'a' }); - * ctx.configure('store2', { x: 2, y: 'b' }); - * - * ctx.bind('store1').toClass(Store); - * ctx.bind('store2').toClass(Store); - * - * const store1 = ctx.getSync('store1'); - * expect(store1.optionX).to.eql(1); - * expect(store1.optionY).to.eql('a'); - - * const store2 = ctx.getSync('store2'); - * expect(store2.optionX).to.eql(2); - * expect(store2.optionY).to.eql('b'); - * ``` - * - * @param configPath Optional property path of the config. If is `''` or not - * present, the `config` object will be returned. - * @param metadata Optional metadata to help the injection - */ + * Inject a property from `config` of the current binding. If no corresponding + * config value is present, `undefined` will be injected as the configuration + * binding is resolved with `optional: true` by default. + * + * @example + * ```ts + * class Store { + * constructor( + * @config('x') public optionX: number, + * @config('y') public optionY: string, + * ) { } + * } + * + * ctx.configure('store1', { x: 1, y: 'a' }); + * ctx.configure('store2', { x: 2, y: 'b' }); + * + * ctx.bind('store1').toClass(Store); + * ctx.bind('store2').toClass(Store); + * + * const store1 = ctx.getSync('store1'); + * expect(store1.optionX).to.eql(1); + * expect(store1.optionY).to.eql('a'); + * + * const store2 = ctx.getSync('store2'); + * expect(store2.optionX).to.eql(2); + * expect(store2.optionY).to.eql('b'); + * ``` + * + * @param configPath Optional property path of the config. If is `''` or not + * present, the `config` object will be returned. + * @param metadata Optional metadata to help the injection + */ export function config(configPath?: string, metadata?: InjectionMetadata) { configPath = configPath || ''; metadata = Object.assign( diff --git a/packages/context/src/keys.ts b/packages/context/src/keys.ts index b8c653a0399c..36b0a57d0bbe 100644 --- a/packages/context/src/keys.ts +++ b/packages/context/src/keys.ts @@ -29,6 +29,11 @@ export namespace ContextTags { */ export const KEY = 'key'; + /** + * Binding tag to associate a configuration binding with the target binding key + */ + export const CONFIGURATION_OF = 'configurationOf'; + /** * Binding tag for global interceptors */