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): add binding.toAlias() #2635

Merged
merged 1 commit into from
Apr 4, 2019
Merged

feat(context): add binding.toAlias() #2635

merged 1 commit into from
Apr 4, 2019

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Mar 22, 2019

A few improvements for @loopback/context related to #2259

  • Add binding.toAlias()
    Use case:
    ctx.bind('parent.options').to({child: {disabled: true}});
    ctx.bind('child.options').toAlias('parent.options#child');
    const childOptions = ctx.getSync('child.options');
    expect(childOptions).to.eql({disabled: true});

Related to #2657
Depends on #2656

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

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.

Nice feature!

What happens when somebody attempts to change the value of an alias? Are going to rebind they key or updated the alias target instead? How about @inject.setter, should it be allowed on aliases?

@bajtos bajtos added feature IoC/Context @loopback/context: Dependency Injection, Inversion of Control labels Mar 25, 2019
@raymondfeng
Copy link
Contributor Author

raymondfeng commented Mar 25, 2019

What happens when somebody attempts to change the value of an alias? Are going to rebind they key or updated the alias target instead? How about @inject.setter, should it be allowed on aliases?

These questions apply to existing binding types too. IMO, It should rebind the same key to whatever we allows, such as:

ctx.bind('existing-key').to<...>();

For @injecter.setter, we implement it as follows:

return function setter(value: unknown) {
    ctx.bind(bindingSelector).to(value);
  };

The binding is rebound as a constant value. Maybe @inject.setter (or a new @inject.binding) should inject an instance of Binding instead. This way, we can do:

binding.to<...>();

This is even better aligned with the idea to have placeholder binding for inject.setter.

The other option is to allow the setter to accept a BindingTemplate function in addition to a const value.

setter((binding) => {
  binding.toClass(MyClass);
});

@raymondfeng
Copy link
Contributor Author

raymondfeng commented Mar 25, 2019

@bajtos Based on the discussion, I added two more commits:

  1. Allow injected setter function from @inject.setter to take binding template functions
  2. Make sure binding cache is cleared if its scope and/or _getValue changes.

@raymondfeng raymondfeng force-pushed the binding-alias branch 4 times, most recently from b28a40d to 1bfc5ef Compare March 25, 2019 19:33
@raymondfeng raymondfeng requested a review from bajtos March 26, 2019 16:01
or

```ts
const binding = this.greetingSetter(b => b.toDynamicValue(() => 'Greetings!'));
Copy link
Contributor Author

@raymondfeng raymondfeng Mar 26, 2019

Choose a reason for hiding this comment

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

An alternative approach is:

this.greetingSetter().toDynamicValue(() => 'Greetings!');

Please note that the setter function now returns Binding.

@raymondfeng raymondfeng changed the title feat(context): add binding.toAlias() to resolve values from another binding feat(context): improve @inject.setter and add binding.toAlias() Mar 26, 2019
@raymondfeng raymondfeng force-pushed the binding-alias branch 3 times, most recently from 431227a to 700231a Compare March 26, 2019 21:45
@raymondfeng raymondfeng force-pushed the binding-alias branch 2 times, most recently from 445d750 to f36e233 Compare March 27, 2019 17:09
Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

LGTM 👍 pls make sure appveyor passes.

@bajtos
Copy link
Member

bajtos commented Mar 28, 2019

Uh oh, I am not a big fan of pull requests adding multiple features. I would have preferred to see @inject.setter changes in a standalone patch.

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.

Allow injected setter function from @inject.setter to take binding template functions

I find this problematic. At the moment, it's possible to bind values to functions too. For example, all sequence actions hold a function as the bound value.

Consider the following example that works well in the current implementation:

// A custom function/action accepting a single string argument 
// and performing some work with side effects
export type MyAction = (arg: string) => Promise<void>;

class MyStrategy {
  constructor(
    @inject.setter('my.action')
    setAction: Setter<MyAction>
  ){}

  void run() {
    setAction(arg => console.log('run my action with', arg));
  }
}

How are you envisioning the detect whether the function argument passed to setAction is a value (e.g. a function implementing an action) or a binding template?

@bajtos
Copy link
Member

bajtos commented Mar 28, 2019

In general, I agree we should improve @inject.setter to allow consumers to use different value types (toDynamicValue, toAlias, etc.) Let's find a more robust solution now.

Personally, I think I prefer @inject.binding most. It fully preserves simplicity and current behavior of @inject.setter, it also communicates well what is the intent of the new injection and what value will be injected.

@raymondfeng raymondfeng changed the title feat(context): improve @inject.setter and add binding.toAlias() feat(context): add binding.toAlias() Mar 28, 2019
@raymondfeng
Copy link
Contributor Author

I have refactored other related commits to:

This PR will be only responsible for adding binding.toAlias(). Please note it depends #2656

@raymondfeng raymondfeng force-pushed the binding-alias branch 3 times, most recently from acf2868 to 6cb1fac Compare April 1, 2019 16:21
@bajtos
Copy link
Member

bajtos commented Apr 2, 2019

@raymondfeng I don't have bandwidth to review this patch today, but I do want to check your updates before this patch is merged. Please give me few more days.

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.

I like how is the pull request small & easy to understand now 👏

The changes look mostly good, PTAL at my two comments below. No further review is needed from my side.

packages/context/src/__tests__/unit/binding.unit.ts Outdated Show resolved Hide resolved
packages/context/src/binding.ts Show resolved Hide resolved
@bajtos bajtos requested a review from a team April 4, 2019 06:15
@raymondfeng raymondfeng merged commit 15dcd16 into master Apr 4, 2019
@raymondfeng raymondfeng deleted the binding-alias branch April 4, 2019 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature IoC/Context @loopback/context: Dependency Injection, Inversion of Control
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants