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

[bajtos]

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'));

This is a follow-up for our discussion in #671, in particular #671 (comment)

Checklist

• npm test passes on your machine
• New tests added or existing tests modified to cover all changes
• Code conforms with the style guide
• Related API Documentation was updated
• (n/a) Affected artifact templates in packages/cli were updated
• (n/a) Affected example projects in packages/example-* were updated

[kjdelisle]

I think this is an important point, and a good change.

Promises are only ever meant to resolve once, so binding them on .to is the same sort of anti-pattern as creating .on(...) event handlers that resolve promises.

[virkt25]

Does this have any impact on components which return a value() function that resolves a promise? (I think log-example-extension might have an example of that or even authentication.

[raymondfeng]

This can be further simplified as ctx.bind('foo').toDynamicValue(async () => 'bar');

[bajtos]

Agreed.

I personally prefer Promise.resolve in this particular case, because async functions incur overhead in terms of the generated code (either explicitly by tsc for Node.js 6.x or implicitly by V8).

[raymondfeng]

We can use async functions to further simplify toDynamicValue, for example,

• ctx.bind('foo').toDynamicValue(async () => 'bar');
• ctx.bind('foo').toDynamicValue(async () => throw new Error('bar error'));

Please also add a test to reject ctx.bind('foo').to(Promise.reject(new Error('err'))

[raymondfeng]

Please rebase it against latest master to get CI passing.

[jannyHou]

LGTM a great practice to learn Promise and event loop.
I agree that the value should be resolved when we retrieve it by a ctx key, not when we bind the ctx key to it.

Hmm..a question(maybe related) when looking at the context functions:
Why we don't allow people to get a Promise from the context but always a value that is resolved? I find getValueOrContext but it looks like only for internal use.

[bajtos]

Hmm..a question(maybe related) when looking at the context functions:
Why we don't allow people to get a Promise from the context but always a value that is resolved? I find getValueOrContext but it looks like only for internal use.

@jannyHou Are you referring to ctx.getValueOrPromise? The API of this function is a bit clunky to use, because the consumers have to check whether a promise or a value was returned.

ctx.get is the API most people should use if they need to access context directly (which is something they should not do in most cases, we provide @inject for Inversion of Control & Depedency Injection design patterns). ctx.get does return a Promise of the bound value.

When the users are sure the value can be obtained synchronously, they can use ctx.getSync.

I think you raised a valid question, would you mind updating our documentation (e.g. Key Concepts >> Context) so that the next person wondering about this topic can find the answer already written down?

[bajtos]

The Travis build has failed on Commit linting:

⧗   input: feat(context): forbid bind().to() a Promise instance
✖   footer must not be longer than 100 characters [footer-max-length]
✖   found 1 problems, 0 warnings

I am going to ignore this error and send a new pull request to tweak commit linter config.

[jannyHou]

Thanks @bajtos I will create a PR for the document.