Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(context): provide resolution context metadata for factory functions with toDynamicValue() #5370

Merged
merged 2 commits into from
May 15, 2020

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented May 8, 2020

A provider class can use dependency injection to receive resolution-related
metadata such as context and binding. But the overhead to wrap a factory
function is not desired for some use cases. This PR introduces a lightweight
alternative using toDynamicValue as follows:

  1. Add a parameter to the factory function to receive resolution context
  2. Support a specialized class with a static value method that allows
    parameter injection.

Some benchmark tests are added to benchmark to measure performances for
various flavors of the dynamic value resolution for bindings.

See https://github.com/strongloop/loopback-next/tree/pass-ctx-to-dynamic-value/benchmark/src/context-binding for examples.

This change is backward compatible. Existing factory functions continue to work.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

  • 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 examples/* were updated

👉 Check out how to submit a PR 👈

@raymondfeng raymondfeng force-pushed the pass-ctx-to-dynamic-value branch from 7c4e33e to 8defa1d Compare May 9, 2020 18:07
@raymondfeng raymondfeng requested a review from bajtos as a code owner May 9, 2020 18:07
@raymondfeng raymondfeng force-pushed the pass-ctx-to-dynamic-value branch from 8defa1d to 3f11e90 Compare May 9, 2020 18:08
@raymondfeng raymondfeng self-assigned this May 9, 2020
@raymondfeng raymondfeng added the IoC/Context @loopback/context: Dependency Injection, Inversion of Control label May 9, 2020
@raymondfeng raymondfeng force-pushed the pass-ctx-to-dynamic-value branch from 3f11e90 to 3895b22 Compare May 9, 2020 21:14
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea 👏 Let's discuss the proposed design, I have also few comments on the implementation details of the benchmarks.

I'd also like to hear from @deepakrkris @emonddr @jannyHo as the CODEOWNERS of the context package.

benchmark/src/context-binding/README.md Outdated Show resolved Hide resolved
benchmark/src/context-binding/context-binding.ts Outdated Show resolved Hide resolved
benchmark/tsconfig.json Show resolved Hide resolved
docs/site/Binding.md Show resolved Hide resolved
@bajtos
Copy link
Member

bajtos commented May 11, 2020

In the past, I was against allowing dynamic value factories to access the context object, because it makes it too easy to slip into using Service Locator design pattern (ctx.get('user')) instead of the preferred Dependency Injection pattern.

Let me add more context to explain the ideal I'd like us to eventually reach in the future.

Let's take a slightly modified example from the PR description:

ctx.bind('user').to('John');
ctx.bind('greeting').toDynamicValue(getUserGreeting);

function getUserGreeting(_ctx) {
  const user = _ctx.getSync('user');
  return `Hello, ${user}`;
}

Using Service Locator has several flaws:

  1. From outside, it's difficult to say what are the dependencies of the function getUserGreeting. All we see is a function accepting a Context instance.

  2. It's difficult to test getUserGreeting in isolation. We have to create a full context object to be able to call the function.

    describe('getUserGreeting', () => {
      it('greets user with name', () => {
        const ctx = new Context();
        // Code smell: magic constant 'user'! 
        // Where is the binding key coming from?
        ctx.bind('user').to('John');
      
        const greeting = getUserGreeting(ctx);
        expect(greeting).to.equal('Hello, John');
      });
    });
  3. When a new (required) dependency is added to getUserGreeting, the compiler cannot point us to places that need to be updated, we will discover the missing dependency only at runtime.

Let's see how Dependency Injection makes everything better:

  1. The function signature clearly describes the dependencies, the implementation is much simpler because it does not have to resolve dependencies.

    function getUserGreeting(user) {
      return `Hello, ${user}`;
    }
    
    ctx.bind('user').to('John');
    ctx.bind('greeting').toDynamicValue<string>('user', getUserGreeting);
  2. The function is easy to test in isolation, no context instance is needed.

    describe('getUserGreeting', () => {
      it('greets user with name', () => {
        const greeting = getUserGreeting('John');
        expect(greeting).to.equal('Hello, John');
      });
    });
  3. When a new (required) dependency is added to getUserGreeting, the compiler will print errors for all places calling getUserGreeting with insufficient number of arguments.

Here is a mock-up of a possible implementation that I was playing with few years ago:

export class Binding<T = BoundValue> extends EventEmitter {
  // ...

   // no dependencies
  toDynamicValue(factoryFn: () => ValueOrPromise<T>): this;
  // 1 dependency
  toDynamicValue<D1>(key1: BindingKey<D1>, factoryFn: (d1: D1) => ValueOrPromise<T>): this;
  // 2 dependencies
  toDynamicValue<D1, D2>(key1: BindingKey<D1>, key2: BindingKey<D2>, factoryFn: (d1: D1, d2: D2) => ValueOrPromise<T>): this;
  // rinse & repeat for 3,4,5,... dependencies

  // the implementation
  toDynamicValue(...args: unknown) {
    const factoryFn = args.pop() as (...deps: unknown[]) => ValueOrPromise<T>;
    const depKeys = args as BindingKey<unknown>[];
    this._setValueGetter(ctx => {
      const depValues = resolveList(depKeys, k => ctx.getPromiseOrValue(k));
      return transformValueOrPromise(depValues, values => factoryFn(values));
    });
  }
}

Now as I wrote before, I am not against the Service Locator approach proposed in this pull request, I am fine to implement it as a short-to-medium-term solution for dynamic value factories. I am just asking to design the APIs in such way that will make it feasible to add Dependency Injection to dynamic value factories later in the future, in a way that won't be disruptive to existing applications using dynamic values based on the Service Locator pattern.

@raymondfeng raymondfeng force-pushed the pass-ctx-to-dynamic-value branch from 3895b22 to 48d437c Compare May 11, 2020 16:28
@raymondfeng
Copy link
Contributor Author

@bajtos I think I have a better/lighter DI solution without reinventing the wheels.

class StaticGreetingProvider {
  static value(@inject('user') user: string) {
    return `Hello, ${user}`;
  }
}

ctx.bind('greeting').toDynamicValue(StaticGreetingProvider);

The idea is to have a class with static value method that supports method parameter injection. The latest PR has implemented the idea.

@raymondfeng raymondfeng force-pushed the pass-ctx-to-dynamic-value branch 2 times, most recently from 7deabac to 02e2921 Compare May 11, 2020 19:40
@raymondfeng raymondfeng requested a review from bajtos May 11, 2020 19:43
@raymondfeng raymondfeng force-pushed the pass-ctx-to-dynamic-value branch 2 times, most recently from 56b877e to 32744a5 Compare May 13, 2020 15:48
@raymondfeng raymondfeng changed the title feat(context): pass more information to the factory function for toDynamicValue feat(context): provide resolution context metadata for factory functions with toDynamicValue() May 13, 2020
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better 👏

I like the approach using a static value method to support @inject based decoration 👍

There is the downside of missing type checks of injected parameters vs. binding key value type, but that's a problem of all existing patterns we have for @inject, so this PR is not making the situation much worse.

I have few comments on how to improve implementation details, see below.

benchmark/src/context-binding/context-binding.ts Outdated Show resolved Hide resolved
benchmark/src/context-binding/context-binding.ts Outdated Show resolved Hide resolved
benchmark/src/context-binding/context-binding.ts Outdated Show resolved Hide resolved
benchmark/src/context-binding/README.md Outdated Show resolved Hide resolved
docs/site/Binding.md Show resolved Hide resolved
packages/context/src/__tests__/unit/binding.unit.ts Outdated Show resolved Hide resolved
packages/context/src/binding.ts Outdated Show resolved Hide resolved
packages/context/src/binding.ts Outdated Show resolved Hide resolved
packages/context/src/resolution-session.ts Outdated Show resolved Hide resolved
@raymondfeng raymondfeng force-pushed the pass-ctx-to-dynamic-value branch from 32744a5 to c21f214 Compare May 14, 2020 15:57
@raymondfeng raymondfeng requested a review from bajtos May 14, 2020 16:02
@raymondfeng
Copy link
Contributor Author

@bajtos Comments addressed. PTAL.

…DynamicValue()

A provider class can use dependency injection to receive resolution-related
metadata such as `context` and `binding`. But the overhead to wrap a factory
function is not desired for some use cases. This PR introduces a lightweight
alternative using `toDynamicValue` as follows:

1. Add a parameter to the factory function to receive resolution context
2. Support a specialized class with a static `value` method that allows
parameter injection.

Some benchmark tests are added to `benchmark` to measure performances for
various flavors of the dynamic value resolution for bindings.
@raymondfeng raymondfeng force-pushed the pass-ctx-to-dynamic-value branch from c21f214 to 6674925 Compare May 14, 2020 16:06
@raymondfeng raymondfeng merged commit bdad689 into master May 15, 2020
@raymondfeng raymondfeng deleted the pass-ctx-to-dynamic-value branch May 15, 2020 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IoC/Context @loopback/context: Dependency Injection, Inversion of Control
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants