Introduce typed binding key #1169

bajtos · 2018-03-22T08:32:48Z

Modify Context, Binding, and related methods to allow developers to add a type information to individual binding keys. These binding keys will allow the compiler to verify that a correct value is passed to binding setters (.to(), .toClass(), etc.) and automatically infer the type of the value returned by ctx.get() and ctx.getSync().

~~This is an early prototype to discuss whether we want to follow the proposed direction.~~

@raymondfeng @strongloop/lb-next-dev @strongloop/loopback-next thoughts?

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
Affected artifact templates in packages/cli were updated
Affected example projects in packages/example-* were updated
Documentation updates

bajtos · 2018-03-22T08:35:30Z

packages/context/src/context.ts

-  bind(key: string): Binding {
+  bind<T = BoundValue>(key: string | BindingKey<T>): Binding<T> {
+    if (typeof key !== 'string') {
+      key = key.value;


Discussion point:

I am thinking about two alternatives, I am not sure which one is better:

keep the current solution where we require the key object to have a .value property

use a different approach - convert the key object to a string key via .toString()

The difference is for people using Context in JavaScript codebase, it allows them to use anything as a key, as long as the value can be converted to a string.

Thoughts?

I think the current approach is sufficient requiring a .value property to the key object. However, I see the UX value in us handling it internally with your alternative approach.

One problem I see with our current approach is that two separate objects with key value mapping to the same string would not behave as users may expect it to.

As an aside, did you mean JSON.stringify as opposed to .toString()?

One problem I see with our current approach is that two separate objects with key value mapping to the same string would not behave as users may expect it to.

@shimks I am afraid I don't understand, could you please add more details? What would users expect and what would they get instead?

As an aside, did you mean JSON.stringify as opposed to .toString()?

I meant .toString(). To be more precise, I would use the following conversion for its performance:

key = '' + key;

b-admike

Nice! I'm glad that we don't have to explicitly type out the expected type in .get, and .getSync calls.

b-admike · 2018-03-22T14:11:07Z

packages/context/src/context.ts

-  bind(key: string): Binding {
+  bind<T = BoundValue>(key: string | BindingKey<T>): Binding<T> {
+    if (typeof key !== 'string') {
+      key = key.value;


I think the current approach is sufficient requiring a .value property to the key object. However, I see the UX value in us handling it internally with your alternative approach.

shimks · 2018-03-22T14:07:09Z

packages/context/test/acceptance/creating-and-resolving-bindings.ts

+        const value = await ctx.get(key);
+        // The following line is accessing a String property as a way
+        // of verifying the value type at compile time
+        expect(value.length).to.equal(5);


Isn't it also fair to just assert expect(value).to.be.a.String()?

Ignore this comment and the next if the approach above has no way of verifying the type at compile time. If so, I'm curious as to why it works like that

Unfortunately expect(value) accepts any type at compile time, it cannot be used to make any compile-time assertions.

shimks · 2018-03-22T14:07:38Z

packages/context/test/acceptance/creating-and-resolving-bindings.ts

+        const value = await ctx.get(key.deepProperty<number>('rest.port'));
+        // The following line is accessing a Number property as a way
+        // of verifying the value type at compile time
+        expect(value.toFixed()).to.equal('80');


Same thing as before here but assert for number

shimks · 2018-03-22T14:19:32Z

packages/context/src/context.ts

-  bind(key: string): Binding {
+  bind<T = BoundValue>(key: string | BindingKey<T>): Binding<T> {
+    if (typeof key !== 'string') {
+      key = key.value;


One problem I see with our current approach is that two separate objects with key value mapping to the same string would not behave as users may expect it to.

As an aside, did you mean JSON.stringify as opposed to .toString()?

shimks · 2018-03-22T14:23:36Z

packages/context/test/acceptance/creating-and-resolving-bindings.ts

+        // of verifying the value type at compile time
+        expect(value.length).to.equal(5);
+      });
+


Could we also get a test case for a custom type?

What kind of a custom type would you like to test?

I was thinking just any random classes, an empty one would be fine.

shimks · 2018-03-22T14:48:12Z

packages/context/src/context.ts

   * @returns A promise of the bound value.
   */
-  get<T>(keyWithPath: string): Promise<T>;
+  get<T>(keyWithPath: string | BindingKey<T>): Promise<T>;


Can we get rid of using generics for these functions? Instead of this get<string>('some.key'), can we force users to do this: get(new BindingKey<string>('some.key'))

We could, but I don't want to. Think about people using JavaScript - I prefer ctx.get('some.key') over ctx.get(new BindingKey('some.key')).

People using JavaScript wouldn't get type coersion anyway, so they're free to choose either ways of retrieving their dependencies (I'd think that most would go for the ctx.get('some.key') option). What I don't like about the hard type casting at the function level is that it's something that users could get wrong and not get any errors from. But the same could be said about the change you're introducing, so maybe it's ok?

Well, if you force users to do get(new BindingKey<string>('some.key')) then JavaScript users won't be able to write ctx.get('some.key'). Unless I misunderstood you?

What I don't like about the hard type casting at the function level is that it's something that users could get wrong and not get any errors from.

Could you please post a code snippet demonstrating such invalid usage? I like making APIs difficult to misuse, but I am having difficulties imagining the example you have in mind.

My bad, I meant that we should force users to do this: get(new BindingKey<string>('some.key') OR get('some.key') (notice the lack of generics here). So if they want type coercion, we have them use the BindingKey constructor.

Take a look at this example:

const key = 'key'; const value = 'value'; const b = new Context(); b.bind(value).to(key); const retrievedValue = b.getSync<number>(key);

There is going to be no compilation error here since there is no way of knowing if retrievedValue is actually a number.
Since we're going to be exporting the keys in the form of BindingKey in most of our modules, if users were to use get() to retrieve whatever's bound to the BindingKey they're importing, they'd immediately know what was the type of the value gotten.

raymondfeng · 2018-03-22T15:44:07Z

-1 to introduce runtime overhead (new BindingKey) for typing. Typing enforcement should only incur compile time cost.

I also would like to keep the binding key simple as string. The type should NOT be associated with the key, which is the address/uri of the bound value.

If we really want to add typing for bind, I would rather make Binding to be typed, such as:

export class Binding<T = BoundValue> {
 // ...
 to(val: T): this {
 }
 
 toClass(cls: Constructor<T>): this {
 }

 toDynamicValue(fn: () => T): this {
 }

 toProvider(provider: Provider<T>): this {
 }
}

export class Context {
  bind<T>(key: string): Binding<T> {
    // ...
  }
}

bajtos · 2018-03-22T16:07:13Z

@raymondfeng I have a different opinion here.

-1 to introduce runtime overhead (new BindingKey) for typing. Typing enforcement should only incur compile time cost.

Please note that each BindingKey is created only once, when we define a well-known binding. See e.g. RestBinding namespace: https://github.com/strongloop/loopback-next/blob/7ba261850603fecd4d6c16329b47dee63b7daa9d/packages/rest/src/keys.ts#L12-L13

Assuming each package has ~10 binding keys, the overhead we are talking about is creation of ~10 objects (instead of ~10 string) at startup. That seems rather insignificant to me, I am sure we have much bigger inefficiencies to worry about.

Do you see any other possible source of runtime overhead?

I also would like to keep the binding key simple as string. The type should NOT be associated with the key, which is the address/uri of the bound value.
If we really want to add typing for bind, I would rather make Binding to be typed, such as.

My problem with your approach:

Users have to know the exact name of the type that's associated with each binding key and also where to import it from.
Users must explicitly provide this type to the compiler when calling ctx.get in order to benefit from compile-type checks.
It's easy to accidentally provide a wrong type when retrieving the value and get a false sense of security.

The third point is important because the bugs can be subtle and difficult to spot.

Consider the following REST binding:

export const HOST = new BindingKey<string | undefined>('rest.host');

Before my change, there was no indication that undefined is a valid value for the HOST binding. Without that knowledge, one could write following code:

const resolve = promisify(dns.resolve);

const host = await ctx.get<string>(RestBindings.HOST);
const records = await resolve(host);
// etc.

The compiler will happily accept such code and only at runtime we will learn that HOST may be also undefined in which case our code needs to find the server's host name using a different way.

Now consider the same snippet with my proposal in place - the compiler immediately tells the developer that "undefined" is an edge case they must handle:

const host = await ctx.get(RestBindings.HOST);
const records = await resolve(host);
// Compiler complains:
// - cannot convert string | undefined to string
//  - cannot convert undefined to string

Ideally, I'd like @inject to enforce the value type too, i.e. somehow reject the situation where the decorated property/argument has a different type than the injected value. I am not sure if that's possible though, so I am starting with an incremental step of fortifying ctx.get for starter.

raymondfeng · 2018-03-22T16:41:14Z

If the purpose is for pre-defined binding keys as constants, I'm fine with the idea to introduce a typed binding key symbol.

The implementation can be simplified as follows:

export class BindingKey<T = BoundValue> {
  public readonly key: string;
  public readonly path?: string;

  private constructor(keyWithOptionalPath: string) {
    Object.assign(this, Binding.parseKeyWithPath(keyWithOptionalPath)); 
  }

  toString() {
    return Binding.buildKeyWithPath(this.key, this.path);
  }

  static create<T = BoundValue>(keyWithOptionalPath: string): BindingKey<T> {
    return new BindingKey<T>(keyWithOptionalPath: string);
  }
}

Then add the following static method to Binding:

  static key<T = BoundValue>(keyWithOptionalPath: string) {
    return BindingKey.create<T>(keyWithOptionalPath: string);
  }
}

The usage will be:

const HOST = Binding.key<string | undefined>('rest.host');

In addition, I created #1174 to add optional typing for Binding.

raymondfeng · 2018-03-22T21:21:14Z

packages/context/src/context.ts

   */
  get<T>(
-    keyWithPath: string,
+    keyWithPath: string | BindingKey<T>,


Maybe it's better to define a type alias as:

export type BindingKeyType<T> = string | BindingKey<T>;

I created the type alias as BindingAddress.

bajtos · 2018-03-26T14:19:22Z

@raymondfeng

const HOST = Binding.key<string | undefined>('rest.host');

This unfortunately triggers a problem/limitation of the current TypeScript and TSLint implementations.

In order to make such code compile, one has to import both Binding and BindingKey.
Because BindingKey is not explicitly referenced, TSLint complains about unused variable.

I'll stick to new BindingKey<string | undefined)('rest.host') for the time being.

raymondfeng · 2018-03-26T15:02:46Z

What about const HOST = BindingKey.create<string | undefined>('rest.host');? Using a factory method give us more flexibility.

bajtos · 2018-03-26T15:21:32Z

What about const HOST = BindingKey.create<string | undefined>('rest.host');? Using a factory method give us more flexibility.

Fair enough, I can do that.

bajtos · 2018-03-26T15:36:10Z

@raymondfeng @b-admike @shimks I have addressed (hopefully all of) your feedback. PTAL again, do the changes look good to you now?

raymondfeng · 2018-03-26T15:49:02Z

packages/context/src/BindingKey.ts

+    return {
+      key: keyWithPath.substr(0, index).trim(),
+      path: keyWithPath.substr(index + 1),
+    };


I think we should turn BindingKey into {key, path?}.

raymondfeng · 2018-03-26T15:50:28Z

packages/context/src/BindingKey.ts

+    return new BindingKey<ValueType>(key);
+  }
+
+  private constructor(public key: string) {}


Let's allow an optional path:

private constructor(public readonly key: string, public readonly path?: string ) {}

raymondfeng · 2018-03-26T15:51:41Z

packages/context/src/BindingKey.ts

+   * @param key The binding key.
+   */
+  public static create<ValueType>(key: string): BindingKey<ValueType> {
+    return new BindingKey<ValueType>(key);


Let's change key to keyWithPath and parse it into {key, path?}.

I'll allow the following flavors:

BindingKey.create('rest'); BindingKey.create('rest#port'); BindingKey.create('rest', 'port');

raymondfeng · 2018-03-26T15:53:23Z

packages/context/src/binding.ts

    this.type = BindingType.PROVIDER;
    this._getValue = (ctx, session) => {
-      const providerOrPromise = instantiateClass<Provider<T>>(
+      const providerOrPromise = instantiateClass<Provider<BoundValue>>(


Why BoundValue instead of T?

This looks like an unintended change, I'll revert it.

raymondfeng · 2018-03-26T15:54:03Z

packages/context/src/context.ts

   */
-  bind<T = BoundValue>(key: string): Binding<T> {
+  bind<ValueType = BoundValue>(
+    key: string | BindingKey<ValueType>,


Can we use BindingAddress<ValueType> here?

raymondfeng · 2018-03-26T16:02:34Z

packages/context/src/BindingKey.ts

+  }
+
+  /**
+   * Get a bnding address for retrieving a deep property of the object


Typo: bnding -> binding

raymondfeng · 2018-03-26T16:07:29Z

packages/context/src/BindingKey.ts

+    return this.key;
+  }
+
+  static asString<T>(address: BindingAddress<T>): string {


This helper method is unnecessary as address.toString() will work for both string and BindingKey.

bajtos · 2018-03-27T13:12:09Z

@raymondfeng comments addressed, PTAL again.

shimks

I'm sure you already have this in mind, but once @raymondfeng is happy with the changes, please make sure to check the docs/cli templates/README so that the code samples utilize the the feature of this PR. LGTM otherwise

virkt25 · 2018-03-27T15:29:35Z

packages/authentication/src/auth-component.ts

    this.providers = {
-      [AuthenticationBindings.AUTH_ACTION]: AuthenticationProvider,
-      [AuthenticationBindings.METADATA]: AuthMetadataProvider,
+      [AuthenticationBindings.AUTH_ACTION.toString()]: AuthenticationProvider,


I find .toString to be messy. Any reason we can't just access the key property of the BindingKey?

AuthenticationBindings.AUTH_ACTION.key

+1 to use AuthenticationBindings.AUTH_ACTION.key.

virkt25 · 2018-03-27T15:39:10Z

+1 for no longer needing to add types when using ctx.get from this PR.

Can someone please explain why we must use a factory method for BindingKey instead of doing what's stated in some of the earlier comments:
new BindingKey<string | undefined>('rest.host');

.create creates extra work imo. Anything preventing a user just instantiating the class?

raymondfeng · 2018-03-27T17:06:44Z

Can someone please explain why we must use a factory method for BindingKey instead of doing what's stated in some of the earlier comments:
new BindingKey<string | undefined>('rest.host');
.create creates extra work imo. Anything preventing a user just instantiating the class?

A static factory method hides the implementation details and give us more flexibility. For example, we might want to cache instances of BindingKey(s).

raymondfeng · 2018-03-27T17:09:00Z

The PR mostly LGTM now.

It would be nice to use AuthenticationBindings.AUTH_ACTION.key instead of AuthenticationBindings.AUTH_ACTION.toString().

Please fix the commit log:

⧗   input: wip: typed binding keys
✖   type must be one of [build, chore, ci, docs, feat, fix, perf, refactor, revert, style, test] [type-enum]
✖   found 1 problems, 0 warnings

virkt25 · 2018-03-27T17:11:33Z

A static factory method hides the implementation details and give us more flexibility. For example, we might want to cache instances of BindingKey(s).

Can't this be done in the constructor?

raymondfeng · 2018-03-27T17:25:17Z

Can't this be done in the constructor?

No. A constructor will always create a new instance.

bajtos · 2018-03-29T12:34:49Z

@shimks

I'm sure you already have this in mind, but once @raymondfeng is happy with the changes, please make sure to check the docs/cli templates/README so that the code samples utilize the the feature of this PR

Thank you for reminding me of this part!

The documentation update turned out to be more involved than I anticipated, I discovered quite few outdated pages. I updated them as part of my work here. I am not entirely happy with squashing possibly unrelated doc updates with the changes adding typed binding keys, but then I don't know how many of those doc updates are possible before the typed binding keys are available.

The commit ccffe17 contains all changes I made since the last round of review.

Please fix the commit log

I'll fix it once this PR gets approved and the history is cleaned up.

@raymondfeng @b-admike @shimks @virkt25 LGTY now?

shimks

👍

shimks · 2018-03-30T16:14:09Z

docs/site/Creating-components.md

 ```

+We recommend to component authors to use
+[Typed binding keys](./Context.html#encoding-value-types-in-binding-keys)


.html -> .md

shimks · 2018-03-30T16:19:53Z

packages/authentication/README.md

 // src/application.ts
 import {BootMixin, Binding, Booter} from '@loopback/boot';
-import {RestApplication, RestServer} from '@loopback/rest';
+import {RestApplication, RestServeri, RestBindings} from '@loopback/rest';


RestServer

Modify Context, Binding, and related methods to allow developers to add a type information to individual binding keys. These binding keys will allow the compiler to verify that a correct value is passed to binding setters (`.to()`, `.toClass()`, etc.) and automatically infer the type of the value returned by `ctx.get()` and `ctx.getSync()`. Bring the documentation in sync with the actual implementation in code. Rename `AuthenticationProvider` to `AuthenticateActionProvider`

bajtos added developer-experience Issues affecting ease of use and overall experience of LB users feature needs discussion labels Mar 22, 2018

bajtos self-assigned this Mar 22, 2018

bajtos requested review from b-admike, jannyHou, raymondfeng, shimks and virkt25 March 22, 2018 08:32

bajtos added needs-priority and removed needs-priority labels Mar 22, 2018

bajtos commented Mar 22, 2018

View reviewed changes

b-admike reviewed Mar 22, 2018

View reviewed changes

shimks reviewed Mar 22, 2018

View reviewed changes

raymondfeng mentioned this pull request Mar 22, 2018

feat(context): add optional typing for Binding #1174

Merged

7 tasks

raymondfeng reviewed Mar 22, 2018

View reviewed changes

bajtos force-pushed the feat/typed-binding-key branch from 7ba2618 to 6408ff5 Compare March 26, 2018 15:10

bajtos changed the title ~~RFC: add value type to the binding key~~ Introduce typed binding key Mar 26, 2018

bajtos force-pushed the feat/typed-binding-key branch 2 times, most recently from 4739bc1 to df583c5 Compare March 26, 2018 15:30

raymondfeng reviewed Mar 26, 2018

View reviewed changes

shimks reviewed Mar 27, 2018

View reviewed changes

virkt25 reviewed Mar 27, 2018

View reviewed changes

dhmlau mentioned this pull request Mar 27, 2018

Context bindings - Type Safety / IntelliSense #67

Closed

bajtos force-pushed the feat/typed-binding-key branch 2 times, most recently from ccffe17 to eba76e6 Compare March 29, 2018 12:33

raymondfeng approved these changes Mar 29, 2018

View reviewed changes

shimks approved these changes Mar 30, 2018

View reviewed changes

shimks mentioned this pull request Apr 2, 2018

feat(repository): add getRepository function to mixin #1215

Merged

7 tasks

bajtos force-pushed the feat/typed-binding-key branch from eba76e6 to af8d3d5 Compare April 9, 2018 15:38

bajtos merged commit 685195c into master Apr 9, 2018

bajtos deleted the feat/typed-binding-key branch April 9, 2018 16:04

bajtos removed the review label Apr 9, 2018

raymondfeng mentioned this pull request Apr 9, 2018

Introduce strongly-typed metadata key #1240

Merged

7 tasks

bajtos mentioned this pull request Mar 29, 2019

fix(context): clear binding cache upon scope/getValue changes #2656

Merged

7 tasks

Introduce typed binding key #1169

Introduce typed binding key #1169

Uh oh!

Conversation

bajtos commented Mar 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

b-admike left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shimks Mar 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raymondfeng commented Mar 22, 2018

Uh oh!

bajtos commented Mar 22, 2018

Uh oh!

raymondfeng commented Mar 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Mar 26, 2018

Uh oh!

raymondfeng commented Mar 26, 2018

Uh oh!

bajtos commented Mar 26, 2018

Uh oh!

bajtos commented Mar 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Mar 22, 2018 •

edited

Loading

shimks Mar 22, 2018 •

edited

Loading

raymondfeng commented Mar 22, 2018 •

edited

Loading

virkt25 commented Mar 27, 2018 •

edited

Loading