feat(context): add type as a generic parameter to ctx.get() and friends

Modify `ctx.get()` and friends to request callers to explicitly specify
what type they are expecting to receive from the context. This prevents
unintentional introduction of `any` type into our codebase.

Consider the following example:

    const bootstrapper = ctx.get('core.Bootstrapper');
    bootstrapper.run(options);

The variable `bootstrapper` has type `any`, which is super easy to miss
when reading/reviewing such code.

Once we introduce `any`, the compiler does not check anything - we can
invoke arbitrary methods with arbitrary parameters, the result of such
call is `any` again. As far as the compiler is concerned, the following
code is perfectly valid too:

    const bootstrapper = ctx.get('core.Bootstrapper');
    const applause = bootstrapper.danceSamba('hey', 'hou');
    applause.play();

With my commit in place, neither of the examples above compiles and the
first example needs to be fixed as follows:

    const bootstrapper = ctx.get<Bootstrapper>('core.Bootstrapper');
    bootstrapper.run(options);

While working on this pull request, I discovered and fixed a problem
related to how we treat Promises. In TypeScript, there are two entities
describing Promises:

 - PromiseLike interface describes any object conforming to Promise/A+
   specification (i.e. has `p.then()` method).
 - Promise class describes the native Promise class. In ES2018, this
   class has additional methods compared to PromiseLike, e.g.
  `p.catch()`.

In our code, there were places where the type definitions were assuming
the native Promise, but the API allowed consumers to provide any
PromiseLike value. For example, loopback-datasource-juggler always
returns Bluebird promises.

This commit fixes type annotations to use PromiseLike in all places
that a user-provided promise value can enter.

BREAKING CHANGE: `ctx.get()` and `ctx.getSync()` require a type now.
See the example below for upgrade instructions:

```diff
- const c: MyController = await ctx.get('MyController');
+ const c = await ctx.get<MyController>('MyController');
```

`isPromise` was renamed to `isPromiseLike` and acts as a type guard
for `PromiseLike`, not `Promise`.  When upgrading affected code, you
need to determine whether the code was accepting any Promise
implementation (i.e. `PromiseLike`) or only native Promises. In the
former case, you should use `isPromiseLike` and potentially convert the
userland Promise instance to a native Promise via
`Promise.resolve(promiseLike)`. In the latter case, you can replace
`isPromise(p)` with `p instanceof Promise`.

Author: bajtos

packages/authentication/test/unit/authenticate-action.test.ts

@@ -60,10 +60,10 @@ describe('AuthenticationProvider', () => {
           .bind(AuthenticationBindings.AUTH_ACTION)
           .toProvider(AuthenticationProvider);
         const request = <ParsedRequest>{};
-        const authenticate = await context.get(
+        const authenticate = await context.get<AuthenticateFn>(
           AuthenticationBindings.AUTH_ACTION,
         );
-        const user: UserProfile = await authenticate(request);
+        const user: UserProfile | undefined = await authenticate(request);
         expect(user).to.be.equal(mockUser);
       });
 
@@ -73,7 +73,7 @@ describe('AuthenticationProvider', () => {
         context
           .bind(AuthenticationBindings.AUTH_ACTION)
           .toProvider(AuthenticationProvider);
-        const authenticate = await context.get(
+        const authenticate = await context.get<AuthenticateFn>(
           AuthenticationBindings.AUTH_ACTION,
         );
         const request = <ParsedRequest>{};
@@ -92,7 +92,7 @@ describe('AuthenticationProvider', () => {
         context
           .bind(AuthenticationBindings.AUTH_ACTION)
           .toProvider(AuthenticationProvider);
-        const authenticate = await context.get(
+        const authenticate = await context.get<AuthenticateFn>(
           AuthenticationBindings.AUTH_ACTION,
         );
         const request = <ParsedRequest>{};

packages/boot/src/bootstrapper.ts

@@ -109,7 +109,10 @@ export class Bootstrapper {
 
     // Resolve Booter Instances
     const booterInsts = await resolveList(filteredBindings, binding =>
-      bootCtx.get(binding.key),
+      // We cannot use Booter interface here because "filter.booters"
+      // allows arbitrary string values, not only the phases defined
+      // by Booter interface
+      bootCtx.get<{[phase: string]: () => Promise<void>}>(binding.key),
     );
 
     // Run phases of booters

packages/boot/test/unit/bootstrapper.unit.ts

@@ -13,49 +13,50 @@ describe('boot-strapper unit tests', () => {
   let app: BootApp;
   let bootstrapper: Bootstrapper;
   const booterKey = `${BootBindings.BOOTER_PREFIX}.TestBooter`;
-  const booterKey2 = `${BootBindings.BOOTER_PREFIX}.TestBooter2`;
+  const anotherBooterKey = `${BootBindings.BOOTER_PREFIX}.AnotherBooter`;
 
   beforeEach(getApplication);
   beforeEach(getBootStrapper);
 
   it('finds and runs registered booters', async () => {
     const ctx = await bootstrapper.boot();
-    const booterInst = await ctx.get(booterKey);
-    expect(booterInst.configureCalled).to.be.True();
-    expect(booterInst.loadCalled).to.be.True();
+    const booterInst = await ctx.get<TestBooter>(booterKey);
+    expect(booterInst.phasesCalled).to.eql([
+      'TestBooter:configure',
+      'TestBooter:load',
+    ]);
   });
 
   it('binds booters passed in BootExecutionOptions', async () => {
-    const ctx = await bootstrapper.boot({booters: [TestBooter2]});
-    const booterInst2 = await ctx.get(booterKey2);
-    expect(booterInst2).to.be.instanceof(TestBooter2);
-    expect(booterInst2.configureCalled).to.be.True();
+    const ctx = await bootstrapper.boot({booters: [AnotherBooter]});
+    const anotherBooterInst = await ctx.get<AnotherBooter>(anotherBooterKey);
+    expect(anotherBooterInst).to.be.instanceof(AnotherBooter);
+    expect(anotherBooterInst.phasesCalled).to.eql(['AnotherBooter:configure']);
   });
 
   it('no booters run when BootOptions.filter.booters is []', async () => {
     const ctx = await bootstrapper.boot({filter: {booters: []}});
-    const booterInst = await ctx.get(booterKey);
-    expect(booterInst.configureCalled).to.be.False();
-    expect(booterInst.loadCalled).to.be.False();
+    const booterInst = await ctx.get<TestBooter>(booterKey);
+    expect(booterInst.phasesCalled).to.eql([]);
   });
 
   it('only runs booters passed in via BootOptions.filter.booters', async () => {
     const ctx = await bootstrapper.boot({
-      booters: [TestBooter2],
-      filter: {booters: ['TestBooter2']},
+      booters: [AnotherBooter],
+      filter: {booters: ['AnotherBooter']},
     });
-    const booterInst = await ctx.get(booterKey);
-    const booterInst2 = await ctx.get(booterKey2);
-    expect(booterInst.configureCalled).to.be.False();
-    expect(booterInst.loadCalled).to.be.False();
-    expect(booterInst2.configureCalled).to.be.True();
+    const testBooterInst = await ctx.get<TestBooter>(booterKey);
+    const anotherBooterInst = await ctx.get<AnotherBooter>(anotherBooterKey);
+    const phasesCalled = testBooterInst.phasesCalled.concat(
+      anotherBooterInst.phasesCalled,
+    );
+    expect(phasesCalled).to.eql(['AnotherBooter:configure']);
   });
 
   it('only runs phases passed in via BootOptions.filter.phases', async () => {
     const ctx = await bootstrapper.boot({filter: {phases: ['configure']}});
-    const booterInst = await ctx.get(booterKey);
-    expect(booterInst.configureCalled).to.be.True();
-    expect(booterInst.loadCalled).to.be.False();
+    const booterInst = await ctx.get<TestBooter>(booterKey);
+    expect(booterInst.phasesCalled).to.eql(['TestBooter:configure']);
   });
 
   /**
@@ -77,24 +78,37 @@ describe('boot-strapper unit tests', () => {
    * A TestBooter for testing purposes. Implements configure and load.
    */
   class TestBooter implements Booter {
-    configureCalled = false;
-    loadCalled = false;
+    private configureCalled = false;
+    private loadCalled = false;
     async configure() {
       this.configureCalled = true;
     }
 
     async load() {
       this.loadCalled = true;
     }
+
+    get phasesCalled() {
+      const result = [];
+      if (this.configureCalled) result.push('TestBooter:configure');
+      if (this.loadCalled) result.push('TestBooter:load');
+      return result;
+    }
   }
 
   /**
    * A TestBooter for testing purposes. Implements configure.
    */
-  class TestBooter2 implements Booter {
-    configureCalled = false;
+  class AnotherBooter implements Booter {
+    private configureCalled = false;
     async configure() {
       this.configureCalled = true;
     }
+
+    get phasesCalled() {
+      const result = [];
+      if (this.configureCalled) result.push('AnotherBooter:configure');
+      return result;
+    }
   }
 });

packages/build/config/tslint.build.json

@@ -12,7 +12,10 @@
     // These rules catch common errors in JS programming or otherwise
     // confusing constructs that are prone to producing bugs.
 
-    "await-promise": true,
+    // User-land promises like Bluebird implement "PromiseLike" (not "Promise")
+    // interface only. The string "PromiseLike" bellow is needed to
+    // tell tslint that it's ok to `await` such promises.
+    "await-promise": [true, "PromiseLike"],
     "no-floating-promises": true,
     "no-unused-variable": true,
     "no-void-expression": [true, "ignore-arrow-function-shorthand"]

packages/context/src/binding.ts

@@ -8,7 +8,7 @@ import {ResolutionSession} from './resolution-session';
 import {instantiateClass} from './resolver';
 import {
   Constructor,
-  isPromise,
+  isPromiseLike,
   BoundValue,
   ValueOrPromise,
 } from './value-promise';
@@ -176,7 +176,7 @@ export class Binding {
   ): ValueOrPromise<BoundValue> {
     // Initialize the cache as a weakmap keyed by context
     if (!this._cache) this._cache = new WeakMap<Context, BoundValue>();
-    if (isPromise(result)) {
+    if (isPromiseLike(result)) {
       if (this.scope === BindingScope.SINGLETON) {
         // Cache the value at owning context level
         result = result.then(val => {
@@ -294,7 +294,7 @@ export class Binding {
    * ```
    */
   to(value: BoundValue): this {
-    if (isPromise(value)) {
+    if (isPromiseLike(value)) {
       // Promises are a construct primarily intended for flow control:
       // In an algorithm with steps 1 and 2, we want to wait for the outcome
       // of step 1 before starting step 2.
@@ -380,7 +380,7 @@ export class Binding {
         ctx!,
         session,
       );
-      if (isPromise(providerOrPromise)) {
+      if (isPromiseLike(providerOrPromise)) {
         return providerOrPromise.then(p => p.value());
       } else {
         return providerOrPromise.value();

packages/context/src/context.ts

@@ -4,12 +4,7 @@
 // License text available at https://opensource.org/licenses/MIT
 
 import {Binding} from './binding';
-import {
-  isPromise,
-  BoundValue,
-  ValueOrPromise,
-  getDeepProperty,
-} from './value-promise';
+import {isPromiseLike, getDeepProperty} from './value-promise';
 import {ResolutionOptions, ResolutionSession} from './resolution-session';
 
 import {v1 as uuidv1} from 'uuid';
@@ -178,8 +173,8 @@ export class Context {
   }
 
   /**
-   * Get the value bound to the given key, optionally return a (deep) property
-   * of the bound value.
+   * Get the value bound to the given key, throw an error when no value was
+   * bound for the given key.
    *
    * @example
    *
@@ -197,22 +192,49 @@ export class Context {
    *
    * @param keyWithPath The binding key, optionally suffixed with a path to the
    *   (deeply) nested property to retrieve.
+   * @returns A promise of the bound value.
+   */
+  get<T>(keyWithPath: string): Promise<T>;
+
+  /**
+   * Get the value bound to the given key, optionally return a (deep) property
+   * of the bound value.
+   *
+   * @example
+   *
+   * ```ts
+   * // get "rest" property from the value bound to "config"
+   * // use "undefined" when not config was provided
+   * const config = await ctx.get('config#rest', {optional: true});
+   * ```
+   *
+   * @param keyWithPath The binding key, optionally suffixed with a path to the
+   *   (deeply) nested property to retrieve.
    * @param optionsOrSession Options or session for resolution. An instance of
    * `ResolutionSession` is accepted for backward compatibility.
-   * @returns A promise of the bound value.
+   * @returns A promise of the bound value, or a promise of undefined when
+   * the optional binding was not found.
    */
-  get(
+  get<T>(
     keyWithPath: string,
     optionsOrSession?: ResolutionOptions | ResolutionSession,
-  ): Promise<BoundValue> {
+  ): Promise<T | undefined>;
+
+  // Implementation
+  get<T>(
+    keyWithPath: string,
+    optionsOrSession?: ResolutionOptions | ResolutionSession,
+  ): Promise<T | undefined> {
     /* istanbul ignore if */
     if (debug.enabled) {
       debug('Resolving binding: %s', keyWithPath);
     }
     try {
-      return Promise.resolve(
-        this.getValueOrPromise(keyWithPath, optionsOrSession),
+      const valueOrPromiseLike = this.getValueOrPromise<T>(
+        keyWithPath,
+        optionsOrSession,
       );
+      return Promise.resolve<T | undefined>(valueOrPromiseLike);
     } catch (err) {
       return Promise.reject(err);
     }
@@ -242,20 +264,50 @@ export class Context {
    * `ResolutionSession` is accepted for backward compatibility.
    * @returns A promise of the bound value.
    */
-  getSync(
+  getSync<T>(keyWithPath: string): T;
+
+  /**
+   * Get the synchronous value bound to the given key, optionally
+   * return a (deep) property of the bound value.
+   *
+   * This method throws an error if the bound value requires async computation
+   * (returns a promise). You should never rely on sync bindings in production
+   * code.
+   *
+   * @example
+   *
+   * ```ts
+   * // get "rest" property from the value bound to "config"
+   * // use "undefined" when not config was provided
+   * const config = await ctx.getSync('config#rest', {optional: true});
+   * ```
+   *
+   * @param keyWithPath The binding key, optionally suffixed with a path to the
+   *   (deeply) nested property to retrieve.
+   * * @param optionsOrSession Options or session for resolution. An instance of
+   * `ResolutionSession` is accepted for backward compatibility.
+   * @returns The bound value, or undefined when an optional binding was not found.
+   */
+  getSync<T>(
     keyWithPath: string,
     optionsOrSession?: ResolutionOptions | ResolutionSession,
-  ): BoundValue {
+  ): T | undefined;
+
+  // Implementation
+  getSync<T>(
+    keyWithPath: string,
+    optionsOrSession?: ResolutionOptions | ResolutionSession,
+  ): T | undefined {
     /* istanbul ignore if */
     if (debug.enabled) {
       debug('Resolving binding synchronously: %s', keyWithPath);
     }
-    const valueOrPromise = this.getValueOrPromise(
+    const valueOrPromise = this.getValueOrPromise<T>(
       keyWithPath,
       optionsOrSession,
     );
 
-    if (isPromise(valueOrPromise)) {
+    if (isPromiseLike(valueOrPromise)) {
       throw new Error(
         `Cannot get ${keyWithPath} synchronously: the value is a promise`,
       );
@@ -326,16 +378,25 @@ export class Context {
    *   on how the binding was configured.
    * @internal
    */
-  getValueOrPromise(
+  getValueOrPromise<T>(
     keyWithPath: string,
     optionsOrSession?: ResolutionOptions | ResolutionSession,
-  ): ValueOrPromise<BoundValue> {
+  ): T | PromiseLike<T> | undefined {
+    // ^^^ Note that boundValue can be any Promise-like implementation,
+    // not necessarily the native Promise instance. As such, it may be
+    // missing newer APIs like Promise#catch() and cannot be casted
+    // directly to a Promise. Callers of this method should use
+    // Promise.resolve() to convert the result into a native Promise.
     const {key, path} = Binding.parseKeyWithPath(keyWithPath);
+
+    // backwards compatibility
     if (optionsOrSession instanceof ResolutionSession) {
       optionsOrSession = {session: optionsOrSession};
     }
+
     const binding = this.getBinding(key, optionsOrSession);
     if (binding == null) return undefined;
+
     const boundValue = binding.getValue(
       this,
       optionsOrSession && optionsOrSession.session,
@@ -344,11 +405,11 @@ export class Context {
       return boundValue;
     }
 
-    if (isPromise(boundValue)) {
-      return boundValue.then(v => getDeepProperty(v, path));
+    if (isPromiseLike(boundValue)) {
+      return boundValue.then(v => getDeepProperty(v, path) as T);
     }
 
-    return getDeepProperty(boundValue, path);
+    return getDeepProperty(boundValue, path) as T;
   }
 
   /**

packages/context/src/index.ts

@@ -6,7 +6,7 @@
 export * from '@loopback/metadata';
 
 export {
-  isPromise,
+  isPromiseLike,
   BoundValue,
   Constructor,
   ValueOrPromise,