From d7415e8be09e09dc7e3a999e07330538a6df803a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Thu, 11 Jan 2018 13:36:27 +0100 Subject: [PATCH] feat(context): forbid bind().to() a Promise instance 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')); --- packages/context/src/binding.ts | 24 ++++++++++++++++++++- packages/context/test/unit/binding.ts | 12 +++++++++++ packages/context/test/unit/context.ts | 8 ++++--- packages/context/test/unit/resolver.test.ts | 20 ++++++++--------- 4 files changed, 50 insertions(+), 14 deletions(-) diff --git a/packages/context/src/binding.ts b/packages/context/src/binding.ts index 74cf9f6c2166..e4ce3d66ec23 100644 --- a/packages/context/src/binding.ts +++ b/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; diff --git a/packages/context/test/unit/binding.ts b/packages/context/test/unit/binding.ts index b3ff1af21c16..ef7786ab09a0 100644 --- a/packages/context/test/unit/binding.ts +++ b/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)', () => { diff --git a/packages/context/test/unit/context.ts b/packages/context/test/unit/context.ts index 55fc8a033440..2471805712b1 100644 --- a/packages/context/test/unit/context.ts +++ b/packages/context/test/unit/context.ts @@ -253,7 +253,7 @@ 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'); }); @@ -261,7 +261,7 @@ describe('Context', () => { 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'); }); diff --git a/packages/context/test/unit/resolver.test.ts b/packages/context/test/unit/resolver.test.ts index 286cddcc0aec..bdad238ff2af 100644 --- a/packages/context/test/unit/resolver.test.ts +++ b/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 () => {