chore(context): address review comments (05/14/2019)

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

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<LoggerConfig | undefined>,
+        public configGetter: Getter<LoggerConfig | undefined>,
       ) {}
     }
 
@@ -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 () => {

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,

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<ConfigValueType | undefined> {
-        return ((key + '.config') as unknown) as ConfigValueType;
+        return (`Dummy config for ${key}` as unknown) as ConfigValueType;
       }
     }
     it('gets default resolver', () => {
@@ -112,15 +123,15 @@ 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', () => {
       ctx
         .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);
     });
   });

packages/context/src/binding-config.ts

@@ -62,7 +62,7 @@ export function configBindingKeyFor<ConfigValueType = unknown>(
   configPath?: string,
 ) {
   return BindingKey.create<ConfigValueType>(
-    BindingKey.buildKeyForConfig(key),
+    BindingKey.buildKeyForConfig<ConfigValueType>(key).toString(),
     configPath,
   );
 }

packages/context/src/binding-key.ts

@@ -104,14 +104,18 @@ export class BindingKey<ValueType> {
     );
   }
 
+  /**
+   * Name space for configuration binding keys
+   */
   static CONFIG_NAMESPACE = '$config';
+
   /**
    * Build a binding key for the configuration of the given binding.
    * The format is `<key>:$config`
    *
-   * @param key The binding key that accepts the configuration
+   * @param key Key of the target binding to be configured
    */
-  static buildKeyForConfig(key: BindingAddress<unknown> = '') {
+  static buildKeyForConfig<T>(key: BindingAddress = ''): BindingAddress<T> {
     const suffix = BindingKey.CONFIG_NAMESPACE;
     const bindingKey = key ? `${key}:${suffix}` : suffix;
     return bindingKey;

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<T = BoundValue> {
     return this._valueConstructor;
   }
 
-  constructor(key: string, public isLocked: boolean = false) {
+  constructor(key: BindingAddress<T>, public isLocked: boolean = false) {
     BindingKey.validate(key);
-    this.key = key;
+    this.key = key.toString();
   }
 
   /**
@@ -596,7 +597,27 @@ export class Binding<T = BoundValue> {
    * @param key Binding key
    */
   static bind<T = unknown>(key: BindingAddress<T>): Binding<T> {
-    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<T = unknown>(key: BindingAddress): Binding<T> {
+    return new Binding(BindingKey.buildKeyForConfig<T>(key)).tag({
+      [ContextTags.CONFIGURATION_OF]: key.toString(),
+    });
   }
 }
 

packages/context/src/context-view.ts

@@ -165,10 +165,9 @@ export class ContextView<T = unknown> 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.',
+    );
   }
 }
 

packages/context/src/context.ts

@@ -374,10 +374,8 @@ export class Context extends EventEmitter {
   configure<ConfigValueType = BoundValue>(
     key: BindingAddress = '',
   ): Binding<ConfigValueType> {
-    const keyForConfig = BindingKey.buildKeyForConfig(key);
-    const bindingForConfig = this.bind<ConfigValueType>(keyForConfig).tag({
-      config: key,
-    });
+    const bindingForConfig = Binding.configure<ConfigValueType>(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;

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(

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
    */