feat(context): forbid bind().to() a Promise instance (#854)

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.

Promises are NOT a tool for storing values that may become available
in the future, depending on the success or a failure of a background
async task.

Values stored in bindings are typically accessed only later,
in a different turn of the event loop or the Promise micro-queue.
As a result, when a promise is stored via `.to()` and is rejected
later, then more likely than not, there will be no error (catch)
handler registered yet, and Node.js will print
"Unhandled Rejection Warning".

BREAKING CHANGE: It is no longer possible to pass a promise instance
to `.to()` method of a Binding. Use `.toDynamicValue()` instead.
Consider deferring the async computation (that produced the promise
instance you are binding) into the dynamic value getter function,
i.e. start the async computation only from the getter function.

An example diff showing how to upgrade your existing code:

-    ctx.bind('bar').to(Promise.resolve('BAR'));
+    ctx.bind('bar').toDynamicValue(() => Promise.resolve('BAR'));

Author: bajtos

packages/context/src/binding.ts

@@ -267,7 +267,8 @@ export class Binding {
   }
 
   /**
-   * Bind the key to a constant value.
+   * Bind the key to a constant value. The value must be already available
+   * at binding time, it is not allowed to pass a Promise instance.
    *
    * @param value The bound value.
    *
@@ -278,6 +279,27 @@ export class Binding {
    * ```
    */
   to(value: BoundValue): this {
+    if (isPromise(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.
+      //
+      // Promises are NOT a tool for storing values that may become available
+      // in the future, depending on the success or a failure of a background
+      // async task.
+      //
+      // Values stored in bindings are typically accessed only later,
+      // in a different turn of the event loop or the Promise micro-queue.
+      // As a result, when a promise is stored via `.to()` and is rejected
+      // later, then more likely than not, there will be no error (catch)
+      // handler registered yet, and Node.js will print
+      // "Unhandled Rejection Warning".
+      throw new Error(
+        'Promise instances are not allowed for constant values ' +
+          'bound via ".to()". Register an async getter function ' +
+          'via ".toDynamicValue()" instead.',
+      );
+    }
     this.type = BindingType.CONSTANT;
     this._getValue = () => value;
     return this;

packages/context/test/unit/binding.ts

@@ -103,6 +103,18 @@ describe('Binding', () => {
       binding.to('value');
       expect(binding.type).to.equal(BindingType.CONSTANT);
     });
+
+    it('rejects promise values', () => {
+      expect(() => binding.to(Promise.resolve('value'))).to.throw(
+        /Promise instances are not allowed.*toDynamicValue/,
+      );
+    });
+
+    it('rejects rejected promise values', () => {
+      expect(() => binding.to(Promise.reject('error'))).to.throw(
+        /Promise instances are not allowed.*toDynamicValue/,
+      );
+    });
   });
 
   describe('toDynamicValue(dynamicValueFn)', () => {

packages/context/test/unit/context.ts

@@ -253,15 +253,15 @@ describe('Context', () => {
 
   describe('get', () => {
     it('returns a promise when the binding is async', async () => {
-      ctx.bind('foo').to(Promise.resolve('bar'));
+      ctx.bind('foo').toDynamicValue(() => Promise.resolve('bar'));
       const result = await ctx.get('foo');
       expect(result).to.equal('bar');
     });
 
     it('returns the value with property separator', async () => {
       const SEP = Binding.PROPERTY_SEPARATOR;
       const val = {x: {y: 'Y'}};
-      ctx.bind('foo').to(Promise.resolve(val));
+      ctx.bind('foo').toDynamicValue(() => Promise.resolve(val));
       const value = await ctx.get(`foo${SEP}x`);
       expect(value).to.eql({y: 'Y'});
     });
@@ -351,7 +351,9 @@ describe('Context', () => {
     });
 
     it('returns nested property (asynchronously)', async () => {
-      ctx.bind('key').to(Promise.resolve({test: 'test-value'}));
+      ctx
+        .bind('key')
+        .toDynamicValue(() => Promise.resolve({test: 'test-value'}));
       const value = await ctx.getValueOrPromise('key#test');
       expect(value).to.equal('test-value');
     });

packages/context/test/unit/resolver.test.ts

@@ -95,8 +95,8 @@ describe('async constructor injection', () => {
 
   before(function() {
     ctx = new Context();
-    ctx.bind('foo').to(Promise.resolve('FOO'));
-    ctx.bind('bar').to(Promise.resolve('BAR'));
+    ctx.bind('foo').toDynamicValue(() => Promise.resolve('FOO'));
+    ctx.bind('bar').toDynamicValue(() => Promise.resolve('BAR'));
   });
 
   it('resolves constructor arguments', async () => {
@@ -196,8 +196,8 @@ describe('async property injection', () => {
 
   before(function() {
     ctx = new Context();
-    ctx.bind('foo').to(Promise.resolve('FOO'));
-    ctx.bind('bar').to(Promise.resolve('BAR'));
+    ctx.bind('foo').toDynamicValue(() => Promise.resolve('FOO'));
+    ctx.bind('bar').toDynamicValue(() => Promise.resolve('BAR'));
   });
 
   it('resolves injected properties', async () => {
@@ -246,8 +246,8 @@ describe('async dependency injection', () => {
 
   before(function() {
     ctx = new Context();
-    ctx.bind('foo').to(Promise.resolve('FOO'));
-    ctx.bind('bar').to(Promise.resolve('BAR'));
+    ctx.bind('foo').toDynamicValue(() => Promise.resolve('FOO'));
+    ctx.bind('bar').toDynamicValue(() => Promise.resolve('BAR'));
   });
 
   it('resolves properties and constructor arguments', async () => {
@@ -268,7 +268,7 @@ describe('async constructor & sync property injection', () => {
 
   before(function() {
     ctx = new Context();
-    ctx.bind('foo').to(Promise.resolve('FOO'));
+    ctx.bind('foo').toDynamicValue(() => Promise.resolve('FOO'));
     ctx.bind('bar').to('BAR');
   });
 
@@ -291,7 +291,7 @@ describe('sync constructor & async property injection', () => {
   before(function() {
     ctx = new Context();
     ctx.bind('foo').to('FOO');
-    ctx.bind('bar').to(Promise.resolve('BAR'));
+    ctx.bind('bar').toDynamicValue(() => Promise.resolve('BAR'));
   });
 
   it('resolves properties and constructor arguments', async () => {
@@ -396,8 +396,8 @@ describe('async method injection', () => {
 
   before(function() {
     ctx = new Context();
-    ctx.bind('foo').to(Promise.resolve('FOO'));
-    ctx.bind('bar').to(Promise.resolve('BAR'));
+    ctx.bind('foo').toDynamicValue(() => Promise.resolve('FOO'));
+    ctx.bind('bar').toDynamicValue(() => Promise.resolve('BAR'));
   });
 
   it('resolves arguments for a prototype method', async () => {