feat(context): add optional typing for Binding

[raymondfeng]

This PR adds optional typing for Binding and getDeepProperty.

Related to #1169

Checklist

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

[bajtos]

IIUC, this pull request is introducing a subset of changes I proposed in my #1169. That's cool! Would you mind to list me as a coauthor of these changes? See https://github.com/blog/2496-commit-together-with-co-authors

I have few comments to consider/address, see below.

[bajtos]

Can you use a different generic parameter name than T please? I find it confusing to have Binding<T>#getValueOrPromise<T>, where each T is a different generic parameter.

[raymondfeng]

Well, did you notice that getValueOrPromise() is a method of Context, not Binding?

[bajtos]

While you are changing the signature of this function, would it make sense to give value argument a generic type too?

export function getDeepProperty<V, T = BoundValue>(
  value: V,
  path: string
): T {
  // impl
}

Ideally, I'd like to get rid of as many usages of any type as we can, eventually removing BoundValue alias too.

Feel free to leave such change out of this pull request if you disagree or if it's not trivial to make.

[raymondfeng]

I have struggled between option A and B:

A:

export function getDeepProperty<V = BoundValue, T = BoundValue>(
  value: V,
  path: string,
): T | undefined {// ...}

B:

export function getDeepProperty<T = BoundValue, V = BoundValue>(
  value: V,
  path: string,
): T | undefined {// ...}

For A, type V and T follows the convention as parameter and return types. But T is more important than V. With A, we cannot use getDeepProperty<string> anymore if the result is expected to string. Instead, we have to use getDeepProperty<V, string>. It's a bit awkward to me.

For B, T comes before V and it's not so natural either.

Anyhow, getDeepProperty is mostly used internally. So I go with A for now.

[bajtos]

I think the changes in this files should not be strictly required, because getDeepProperty defaults to any type, which is fine as long as expect is concerned.

Are you adding <number> specialization to verify that getDeepProperty accepts a generic parameter? We should write a new test to do so, one that will also verify at compile time that the result of getDeepProperty is indeed the type we specified. (Most of) the existing tests should remain unchanged, that way we can verify that getDeepProperty can be used without the generic parameter too.

See #1169 for an example:

https://github.com/strongloop/loopback-next/blob/7ba261850603fecd4d6c16329b47dee63b7daa9d/packages/context/test/acceptance/creating-and-resolving-bindings.ts#L48-L55

To make the intent easier to understand without the need for excessive comments, I would write a helper along the following lines:

// usage
      it('allows access to a deep property', async () => {
        const key = new BindingKey<object>('foo');
        ctx.bind(key).to({rest: {port: 80}});
        const value = await ctx.get(key.deepProperty<number>('rest.port'));
        expectNumberAtCompileTime(value, 80);
      });

// impl
function expectNumberAtCompileTime(actual: number, expected: number) {
  expect(actual).to.be.a.Number()
  expect(actual).to.equal(expected);
}

[raymondfeng]

Fixed.

[raymondfeng]

@bajtos PTAL

[bajtos]

For A, type V and T follows the convention as parameter and return types. But T is more important than V. With A, we cannot use getDeepProperty anymore if the result is expected to string. Instead, we have to use getDeepProperty<V, string>. It's a bit awkward to me.

For B, T comes before V and it's not so natural either.

Anyhow, getDeepProperty is mostly used internally. So I go with A for now.

I am not sure that I understand what you are saying, the generic parameter names T and V make the reasoning difficult for me now in the evening.

In my experience, it's ok to put the return parameter as the first generic argument, and then input parameter 1,2,3, etc. as following generic arguments. I find it pretty natural, because the type of the input arguments is know and can be inferred by the compiler, while the return type must be usually explicitly provided by the user. I have definitely seen this convention in code in the past.

Another point to consider - the compiler should be able to infer the type of the input value for us, users should not need to provide it.

For me, this would be the ideal signature - although I don't have bandwidth now to try it out:

export function getDeepProperty<Result, Input>(
  value: Input,
  path: string,
): Result | undefined {// ...}

// intended usage:
getDeepProperty<number>({port: 80}, 'port');

Anyhow, I agree getDeepProperty is internal and thus can be easily reworked any time later.

[bajtos]

LGTM, but please consider renaming V and T in getDeepProperty and switching their order per my comment above.

[shimks]

If I understand correctly, this PR will be replacing #1169 right?

[raymondfeng]

If I understand correctly, this PR will be replacing #1169 right?

No, it's complementary to #1169, which will introduce BindingKey as a symbol for pre-defined binding keys with type check.

[raymondfeng]

LGTM, but please consider renaming V and T in getDeepProperty and switching their order per my comment above.

Fixed.

[virkt25]

I may have asked this before but what is the purpose of the trailing !?

[raymondfeng]

The ! tells TS compiler that the value is not undefined. In this case, we already test the key exists in the cache 1st.