Add support for mixed sync/async bindings

[bajtos]

This is required to allow injection of asynchronously-computed values into Controller constructors.

• Allow dynamic bindings to be created using an async function returning
  a promise.

• Modify ctx.get to always return a promise, regardless of whether
  the binding is sync or async.

• Modify dependency injection code to detect sync vs. async dependencies and optimise the code building the dependencies to prefer sync flow if allowed by dependencies.

• Add new API ctx.getAsync that returns the bound value synchronously and throws if the factory function was async.

• Add new API ctx.getBinding to look up the Binding object by a key

• Add new API isPromise that can be used by consumers of ctx.getBinding().getValue() to detect whether the value was resolved sync or async.

As we discussed elsewhere, most code should use @inject to resolve dependencies, therefore it should not be affected by sync/async nuances.

When getting values from the context directly, people should default to asynchronous ctx.get(key), because it's difficult to tell in advance which bindings may eventually become asynchronous due to 3rd party extensions.

const name = await ctx.get('application.name');

Having wrote that, the internal API for accessing either sync value or async Promise is public and available for advanced users.

const binding = ctx.getBinding('application.name');
const valueOrPromise = binding.getValue();
if (isPromise(valueOrPromise)) {
  valueOrPromise.then(doSomething);
} else {
  doSomething(valueOrPromise);
}

---

This is a pre-requisite for #186 and #188 which I am implementing as part of the time allocated in strongloop-internal/scrum-asteroid#117.

cc @bajtos @raymondfeng @ritch @superkhau

[raymondfeng]

Making everything async seems to complicate the context so much. What about introducing ctx.await(...) to always treat the bound value as a Promise (which can be created from a sync value/function internally)? For example:

class Context {
  await: async function(key) {
    return await this.get(key); // Return a promise regardless of the return value from get
  }
  ...

[bajtos]

I agree async is adding extra complexity.

However, consider this example in our codebase:

https://github.com/strongloop/loopback-next/blob/7cedcf549c915ec4215f3e9df7b76789319dfc07/packages/loopback/lib/server.ts#L43-L47

    this.find('applications.*').forEach(appBinding => {
      debug('Registering app controllers for %j', appBinding.key);
      const app = appBinding.getValue() as Application;
      app.mountControllers(router);
    });

Now let's say an application developer needs to run some async logic to build an app instance (it may be a bit silly, but let's use it for the sake of this example):

this.bind('applications.async').toDynamicValue(() => {
  // fetch some data, e.g. WSDL or Swagger
  // or perhaps check with Consul to fetch updated config
  const config = await appConfig();
  return new AsyncApp(config);
});

Now the code in LoopBack's server.ts file will crash, because it will treat Promise<Application> as an Application instance.

Again, this example may be not probable, but I feel there will be other, more probable places where we will expect context bindings to be synchronous and thus severely limiting extensibility.

A real-world example: Socket.io session adapters (pre 1.x) - they are synchronous, therefore the only reliable implementation is the one storing data in memory (in-process), anything else is suffering from race conditions. See socketio/socket.io#952 and our own strongloop/strong-cluster-socket.io-store#8.

Aren't we using async/await exactly to allow us to use Promises/async flow control everywhere, because the code to consume async APIs is so easy now?

[bajtos]

Few more ideas:

• Changing a sync function to async function has rippling effect - all callers of the modified functions must become async, then the callers of the callers, etc.
• An example from LoopBack 3.x: strong-remoting parses and validates all arguments synchronously, therefore customization of the injected options object is clumsy. See https://loopback.io/doc/en/lb3/Using-current-context.html#customize-the-value-provided-to-options I think this is a prime example why it's difficult to estimate upfront which places can stay sync and which must be async in order to allow extensibility.

[bajtos]

I proposed a feature for tslint that would make this comment unnecessary - see palantir/tslint#2637

[bajtos]

On the second thought, let's remove promise-function-async completely, it no longer serves the original purpose: #221

[ritch]

We had an offline discussion about this and we've agreed to implement the following:

// Context is a container of async functions that resolve useful/parsed values from the request

const binding = ctx.findById('b')
binding.isSync()   // true when bound to a value synchronously
binding.getSync()  // returns value direct
ctx.getSync()      // throws unless sync
ctx.get()          // get the promise

We should optimize inject to use getSync for you.

[bajtos]

@ritch I ended up with a simpler and more powerful design. There is no isSync flag, the sync/async detection uses the value returned by binding.getValue(). Benefits:

• It is easier to detect sync/async mode of transitive dependencies, like when Class1 depends on Class2 that depends on computed ValueX that may be sync or async. In my proposed implementation, if ValueX is bound in sync way, then both Class2 and Class1 can be retrieved synchronously. If somebody rebinds ValueX to async, then the next fetch of Class2 will become async too.
• We can keep a single API .toDynamicValue() to support both sync and async factories. This creates less space for confusion and programmer errors, because the sync/async decision is made from the actual return value of a function, not from what the programmer told us in metadata.

@ritch @raymondfeng @superkhau please review

[bajtos]

A question to consider: should we rename binding.getValue to binding.getValueOrPromise to make it more explicit that this function may return either a sync value or an async promise?

[ritch]

LGTM

Since you have the reference to the binding anyways its better to just resolve the value with that. 👍

[raymondfeng]

The name can be instantiate.

[raymondfeng]

LGTM

[superkhau]

LGTM