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

bajtos · 2018-02-26T08:34:12Z

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:

- 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.

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
Affected example projects in packages/example-* were updated

bajtos · 2018-02-26T08:38:25Z

Changes related to Promise vs. PromiseLike were triggered by a compilation error I received:

Property 'catch' does not exist on type 'PromiseLike'

While I was researching the problem, I found the following content as related. After a week's break, I no longer remember details, but I am still posting the links for posterity.

bajtos · 2018-02-26T08:41:44Z

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

-    const booterInst2 = await ctx.get(booterKey2);
-    expect(booterInst.configureCalled).to.be.False();
-    expect(booterInst.loadCalled).to.be.False();
-    expect(booterInst2.configureCalled).to.be.True();


These assertions are rather unhelpful when they fail:

1) boot-strapper unit tests only runs booters passed in via BootOptions.filter.booters: AssertionError: expected false to be true at Assertion.fail (packages/testlab/node_modules/should/as-function.js:275:17) at Assertion.value (packages/testlab/node_modules/should/as-function.js:356:19) at Context.it (packages/boot/test/unit/bootstrapper.unit.ts:4:65) at <anonymous>

While I was fixing the cause of the test failure, I rewrote all tests to be more helpful when failing:

AssertionError: expected Array [ ] to equal Array [ 'TestBooter:configure' ] + expected - actual [ + "TestBooter:configure" ]

raymondfeng · 2018-02-26T16:40:04Z

packages/context/src/value-promise.ts

@@ -32,8 +32,8 @@ export type MapObject<T> = {[name: string]: T};
 *
 * @param value The value to check.
 */
-export function isPromise<T>(
-  value: T | PromiseLike<T>,
+export function isPromiseLike<T>(


What's the expectation that we want to set for developers, PromiseLike or Promise? IIUC, PromiseLike does not have catch function. Is it going to be a surprise to developers that want to use compliant promises? Maybe we should switch from PromiseLike to Promise.

For the record, Promise.prototype.catch() is available since ES2015.

+1 for switching to Promise instead of us having to do https://github.com/strongloop/loopback-next/pull/1050/files#diff-f726e786caf6fb7f3616d8a58c63b3bfR58 (but that's the only instance I found). Not sure of other reasons.

You are right that Promise.prototype.catch is available since ES2015. It's the other API, Promise.prototype.finally that is coming in ES2018 IIRC - see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/finally.

What's the expectation that we want to set for developers, PromiseLike or Promise?

My personal take based on Robustness principle (Be conservative in what you do, be liberal in what you accept from others.):

When expecting a Promise created by the user/caller, we should allow any PromiseLike value. This simplifies integration with existing 3rd party libraries that do not use native promises yet, our own juggler is a prime example.

When returning a Promise to the user/caller, always return a native Promise.

Please note that this is exactly how async/await works according to the spec - you can await any promise-like value, async function always return a native promise.

The changes proposed in this pull request are going in this direction.

Is it going to be a surprise to developers that want to use compliant promises?

IMO, as long as we always return native promises, developers should not face any surprise.

Having said that, I would be surprised if LoopBack did not accept non-native Promises (e.g. as returned by Bluebird) and forced me to convert non-native promises returned by 3rd party modules to native promises.

raymondfeng · 2018-02-26T16:45:24Z

packages/context/src/context.ts

      );
+      return Promise.resolve<T | undefined>(valueOrPromiseLike);


The implementation can be simplified using async await. For example:

// Implementation async get<T>( keyWithPath: string, optionsOrSession?: ResolutionOptions | ResolutionSession, ): Promise<T | undefined> { /* istanbul ignore if */ if (debug.enabled) { debug('Resolving binding: %s', keyWithPath); } return await this.getValueOrPromise<T>( keyWithPath, optionsOrSession, ); }

I think we are/were intentionally avoiding async functions in performance-critical code paths. This pull request is also focusing on fixing type definitions, not necessarily rewriting code from promise-like to async/await style.

Having said that, if there is a consensus that Context functions should be rewritten to async/await style as part of this pull request, then I don't mind too much.

On the second thought, I think the actual reason for using Promise.resolve instead of await was that I though it's not allowed to await a non-promise value.

I looked up docs and it looks it's perfectly fine to await a value that's not a promise, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/await:

If the value of the expression following the await operator is not a Promise, it's converted to a resolved Promise.

In that light, I agree ctx.get should be rewritten to an async function and I'll make the changes ASAP.

b-admike

Nice work! There are some minor fixups that you included in this PR, and it'd be great if you can do them in another PR, but I'm ok with including them here.

b-admike · 2018-02-27T05:49:06Z

packages/context/src/context.ts

+   * ```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});


Shall we modify the code snippet in this example as well? config = await ctx.get<RestComponentConfig>('config#rest', {optional: true}); (see https://github.com/strongloop/loopback-next/blob/master/packages/rest/src/rest-component.ts#L48)

Good catch! I'll edit the snippet.

b-admike · 2018-02-27T05:51:30Z

packages/context/src/context.ts

+   * ```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});


b-admike · 2018-02-27T05:53:50Z

packages/context/src/value-promise.ts

@@ -32,8 +32,8 @@ export type MapObject<T> = {[name: string]: T};
 *
 * @param value The value to check.
 */
-export function isPromise<T>(
-  value: T | PromiseLike<T>,
+export function isPromiseLike<T>(


+1 for switching to Promise instead of us having to do https://github.com/strongloop/loopback-next/pull/1050/files#diff-f726e786caf6fb7f3616d8a58c63b3bfR58 (but that's the only instance I found). Not sure of other reasons.

bajtos · 2018-02-27T09:20:56Z

@raymondfeng @b-admike thank you for the review. I have addressed all your comments and responded to questions about PromiseLike vs. Promise.

LGTY now?

b-admike

🚢🇮🇹

virkt25 · 2018-02-27T15:10:18Z

Wow. Very cool PR and feature but I do have a question regarding the breaking change ...

Is it possible to make the type information optional? I don't think it's good UX to require the user to always import the Class / Function / Value type being retrieved when using .get. If I'm already importing the Class / Function then in some cases I might as well just use that class as is (but yea DI has other advantages).

What was the use case for this because it's possible to cast the get into the type you want as done in Bootstrapper.

const bootstrapper: Bootstrapper = await this.get('boot.bootstrapper');
const applause = bootstrapper.danceSamba('hey', 'hou'); // Compiler complains "Property 'danceSamba' does not exist on type 'Bootstrapper"

raymondfeng · 2018-02-27T16:33:29Z

packages/context/src/context.ts

    keyWithPath: string,
    optionsOrSession?: ResolutionOptions | ResolutionSession,
-  ): ValueOrPromise<BoundValue> {
+  ): T | PromiseLike<T> | undefined {


Should it be ValueOrPromise<T | undefined>? If the optional is true, can a value provider return undefined?

This is a good question. Right now, the code in getValueOrPromise returns undefined only when the binding was not found. A provider cannot return undefined unless it's declared as implements Provider</*...*/ | undefined>, which is actually a valid usage we need to consider!

Another source of undefined is getDeepProperty, which can return undefined when the deep property does not exist.

I need to take another look at both issues pointed above.

I think that ctx.get should throw when a deep property does not exist, for consistency with the case when the binding itself was not found. What do you think?

Would you mind if I restrict the changes in this patch to changing the return value of getValueOrPromise to ValueOrPromise<T | undefined>, and then look how to further improve type checks (and possibly throw when a deep property was not found) in a follow-up pull request?

bajtos · 2018-02-27T18:34:27Z

@virkt25

Is it possible to make the type information optional? I don't think it's good UX to require the user to always import the Class / Function / Value type being retrieved when using .get. If I'm already importing the Class / Function then in some cases I might as well just use that class as is (but yea DI has other advantages).

Yes, you can always specify the type as any:

const fooBar = ctx.get<any>('foo.bar');

I just want you to be explicit about the fact that you are introducing any.

Also note that you can omit the generic parameter altogether, in which case TypeScript will assume {} (an empty object).

I am intentionally NOT defaulting to any, because that will effectively turn off compiler checks. (I have tried that early on while working on this pull request.) I want the compiler to tell me when I forgot to specify the type and introduce any as a result.

What was the use case for this because it's possible to cast the get into the type you want as done in Bootstrapper.

Consider the following snippet, which was the first version you wrote IIRC:

const bootstrapper = await this.get('boot.bootstrapper');
const applause = bootstrapper.danceSamba('hey', 'hou');

As a reader, it's difficult to tell that bootstrapper is any in this case, because we are used to the fact that TypeScript can infer variable types using the right hand side of the initialization assignment. I think you, as a person writing this code, did not realize introduction of any either, did you?

Compare the code above with the following:

const productRepository = new DefaultCrudRepository<Product>(db);

const items = await productRepository.find({where: {price: {$lt: 10}}});

There is no type annotation for items, and yet they have a proper type inferred by TypeScript (Product[]).

So I am looking for a solution that will allow us to distinguish between those two cases, accept the second case because we have type information there, reject the first case because it introduces any.

Before my change, it was too easy to miss the fact that ctx.get is introducing any. With my change in place, the compiler will complain if you forget to specify the result type of ctx.get. If you don't want to specify the type, then you can use any, but you must be explicit about such choice.

Sorry if my comment is repetitive too much, it's a late evening and I need to get this out of my system, so that I can stop thinking about work :)

raymondfeng · 2018-02-28T16:51:42Z

For non-optional injections, it's probably better to only throw an error if the binding key cannot be found in the context hierarchy. There are situations where undefined is a valid value and it's hard to justify whether a undefined does not fulfill the dependency.

…ends 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`.

bajtos · 2018-03-01T11:10:43Z

For non-optional injections, it's probably better to only throw an error if the binding key cannot be found in the context hierarchy. There are situations where undefined is a valid value and it's hard to justify whether a undefined does not fulfill the dependency.

I'd like to explore this problem more, but let's keep that out of scope of this pull request. For now, I am going to keep the current implementation that throws only when the binding key cannot be found.

bajtos · 2018-03-01T11:14:37Z

New changes:

Moved spelling fixes to a new pull request fix(boot): fix spelling typos #1066 to speed up the review of that part.
Reworked ctx.getValueOrPromise to use ValueOrPromise under the hood. Reworked ValueOrPromise to use PromiseLike instead of Promise for the async variant. See feat(context): add type as a generic parameter to ctx.get() and friends #1050 (comment) for an explanation why I think this is the right way to go.

@virkt25 @raymondfeng LGTY now?

came across this while reviewing the code / readme. Making separate PRs for easier review. This usage was introduced in #1050 connected to #988

bajtos added developer-experience Issues affecting ease of use and overall experience of LB users breaking-change labels Feb 26, 2018

bajtos added this to the February 2018 milestone Feb 26, 2018

bajtos self-assigned this Feb 26, 2018

bajtos requested review from raymondfeng, kjdelisle and virkt25 February 26, 2018 08:34

bajtos added the needs-priority label Feb 26, 2018

bajtos commented Feb 26, 2018

View reviewed changes

raymondfeng reviewed Feb 26, 2018

View reviewed changes

b-admike reviewed Feb 27, 2018

View reviewed changes

bajtos force-pushed the feat/typed-context-get branch from a0a3397 to 78a85de Compare February 27, 2018 09:26

bajtos changed the title ~~Feat/typed context get~~ feat(context): add type as a generic parameter to ctx.get() and friends Feb 27, 2018

b-admike approved these changes Feb 27, 2018

View reviewed changes

raymondfeng reviewed Feb 27, 2018

View reviewed changes

This was referenced Mar 1, 2018

Fix tsc setup to report project-relative paths in error messages #1010

Closed

fix(boot): fix spelling typos #1066

Merged

bajtos force-pushed the feat/typed-context-get branch from 78a85de to 6cd7d7c Compare March 1, 2018 11:02

fixup! change ctx.getValueOrPromise to return ValueOrPromise

ce0d79f

bajtos modified the milestones: February 2018, March 2018 Mar 1, 2018

raymondfeng approved these changes Mar 1, 2018

View reviewed changes

raymondfeng merged commit 1d9979f into master Mar 1, 2018

raymondfeng removed the needs-priority label Mar 1, 2018

raymondfeng deleted the feat/typed-context-get branch March 1, 2018 16:26

bajtos mentioned this pull request Mar 5, 2018

lb4: fix usage of Context#get to specify type loopbackio/loopback.io#637

Merged

virkt25 added a commit that referenced this pull request Mar 12, 2018

refactor(core): use generics for type casting

5663a37

came across this while reviewing the code / readme. Making separate PRs for easier review. This usage was introduced in #1050 connected to #988

virkt25 mentioned this pull request Mar 12, 2018

refactor(core): use generics for type casting #1123

Merged

6 tasks

virkt25 added a commit that referenced this pull request Mar 13, 2018

refactor(core): use generics for type casting

6adf5d6

came across this while reviewing the code / readme. Making separate PRs for easier review. This usage was introduced in #1050 connected to #988

virkt25 added a commit that referenced this pull request Mar 13, 2018

refactor(core): use generics for type casting

a626da2

came across this while reviewing the code / readme. Making separate PRs for easier review. This usage was introduced in #1050 connected to #988

virkt25 added a commit that referenced this pull request Mar 13, 2018

refactor(core): use generics for type casting

5512308

came across this while reviewing the code / readme. Making separate PRs for easier review. This usage was introduced in #1050 connected to #988

virkt25 added a commit that referenced this pull request Mar 13, 2018

refactor(core): use generics for type casting

302caa5

came across this while reviewing the code / readme. Making separate PRs for easier review. This usage was introduced in #1050 connected to #988

virkt25 added a commit that referenced this pull request Mar 14, 2018

refactor(core): use generics for type casting

efa9252

came across this while reviewing the code / readme. Making separate PRs for easier review. This usage was introduced in #1050 connected to #988

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bajtos commented Feb 26, 2018 •

edited

Loading

bajtos commented Feb 26, 2018

bajtos Feb 26, 2018

raymondfeng Feb 26, 2018

raymondfeng Feb 27, 2018

b-admike Feb 27, 2018

bajtos Feb 27, 2018

raymondfeng Feb 26, 2018

bajtos Feb 27, 2018

bajtos Feb 27, 2018

b-admike left a comment

b-admike Feb 27, 2018

bajtos Feb 27, 2018

b-admike Feb 27, 2018

b-admike Feb 27, 2018

bajtos commented Feb 27, 2018

b-admike left a comment

virkt25 commented Feb 27, 2018

raymondfeng Feb 27, 2018

bajtos Feb 27, 2018

bajtos commented Feb 27, 2018

raymondfeng commented Feb 28, 2018

bajtos commented Mar 1, 2018

bajtos commented Mar 1, 2018

		);
		return Promise.resolve<T \| undefined>(valueOrPromiseLike);

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

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

Conversation

bajtos commented Feb 26, 2018 • edited Loading

Checklist

bajtos commented Feb 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-admike left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Feb 27, 2018

b-admike left a comment

Choose a reason for hiding this comment

virkt25 commented Feb 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Feb 27, 2018

raymondfeng commented Feb 28, 2018

bajtos commented Mar 1, 2018

bajtos commented Mar 1, 2018

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

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

bajtos commented Feb 26, 2018 •

edited

Loading