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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(context): improve @inject.setter and add @inject.binding #2657

Merged
merged 1 commit into from Apr 12, 2019

Conversation

Projects
None yet
3 participants
@raymondfeng
Copy link
Member

commented Mar 28, 2019

Extracted from #2635

  1. Allow @inject.setter to set other forms of value providers than constant
// Set the binding to a constant value, this is the current behavior
injectedSetter('my-value'); 

// or use the returned binding to configure it
const binding = injectedSetter();
binding.toClass(MyClass);
  1. Allows bindingCreation option to control how the underlying binding is resolved/created.

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

@raymondfeng raymondfeng requested a review from bajtos as a code owner Mar 28, 2019

@raymondfeng raymondfeng changed the title feat(context): improve inject setter feat(context): improve @inject.setter Mar 28, 2019

@raymondfeng raymondfeng referenced this pull request Mar 28, 2019

Merged

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

5 of 7 tasks complete
@bajtos
Copy link
Member

left a comment

Personally, I prefer UNIX philosophy - many small tools where each tool does one job and does it well. In that light, I would prefer a new decorator for injecting the entire Binding instance. You have mentioned @inject.binding yourself in one of the earlier pull requests, what was the reasoning for your decision to put this new feature into the existing @inject.setter function?

Show resolved Hide resolved docs/site/Decorators_inject.md Outdated
Show resolved Hide resolved ...hentication/src/__tests__/unit/providers/authentication.provider.unit.ts Outdated
Show resolved Hide resolved ...ages/context/src/__tests__/acceptance/class-level-bindings.acceptance.ts Outdated
Show resolved Hide resolved packages/context/src/__tests__/unit/context.unit.ts Outdated
Show resolved Hide resolved packages/context/src/context.ts Outdated
Show resolved Hide resolved packages/context/src/inject.ts Outdated
Show resolved Hide resolved packages/context/src/inject.ts Outdated
Show resolved Hide resolved packages/context/src/inject.ts Outdated

@raymondfeng raymondfeng force-pushed the improve-inject-setter branch from f4deea3 to dcabfcd Mar 29, 2019

@raymondfeng

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2019

Personally, I prefer UNIX philosophy - many small tools where each tool does one job and does it well. In that light, I would prefer a new decorator for injecting the entire Binding instance. You have mentioned @inject.binding yourself in one of the earlier pull requests, what was the reasoning for your decision to put this new feature into the existing @inject.setter function?

I'm open here but I would like to avoid a new @inject.* flavor before it's necessary. Here is my reasoning:

@inject.setter was introduced to bind a value (getter) to the underlying key. The 1st version only allows constant value (binding.to()) by calling setter(constantValue). This PR extends the capability to have more control by exposing the Binding instance. The initial version was to allow BindingTemplate functions to be accepted by the setter function but we cannot differentiate constant function value from template function.

If we decide to introduce @inject.binding, maybe we should deprecate and finally remove @inject.setter. It's similar as preferring @inject(filterByTag('my-tag') over @inject.tag('my-tag').

@raymondfeng raymondfeng force-pushed the improve-inject-setter branch from 64b81fb to 76dc737 Mar 29, 2019

@raymondfeng

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2019

@bajtos I went ahead to add @inject.binding and leave @inject.setter to only set constant values.

@raymondfeng raymondfeng force-pushed the improve-inject-setter branch from 76dc737 to 623cf16 Mar 29, 2019

@bajtos

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

I went ahead to add @inject.binding and leave @inject.setter to only set constant values.

Awesome 馃憤

Here is my point of view:

  • I find it confusing to have a decorator called @inject.setter to inject a Binding instance.
  • Ideally, IoC related code (like Binding instances) should not leak into the actual implementation (the code receiving the dependencies via injection).

With @inject.setter, the decorated argument/property receives a generic setter function that can be passed to any 3rd-party code (e.g. a package from npm), the consumer does not need to understand anything about our Context and Binding.

In that sense, @inject.binding can be seen as a kind of an anti-pattern. Having said that, I am ok to add that functionality as an advanced tool for situations where @inject.setter is not good enough. Once we learn more about use cases where @inject.binding is needed, we can look into ways how to provide more clean API(s) to support those cases.

@bajtos
Copy link
Member

left a comment

The new version looks good at high level, let's improve implementation details now.

The documentation changes in Decorators_inject.md could use some extra attention to ensure consistency.

/**
* Set the underlying binding to a const value. Returns the `Binding` object
* so that the binding can be further configured, such as setting it to a
* class with `toClass()`.

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 1, 2019

Member

I find it weird to have two tsdoc comments for the same type. Which comment is rendered by our API docs? Can we keep only the comment that's picked up by our docs, and remove the other one please?

* be changed and returned as-is.
*/
(value?: T) => void;
```

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 1, 2019

Member

The implementation is using type Setter<T>, please update this part of the docs to do the same.

Show resolved Hide resolved docs/site/Decorators_inject.md Outdated
Show resolved Hide resolved docs/site/Decorators_inject.md Outdated
u => (currentUser = u),
u => {
currentUser = u;
return new Binding('authentication.currentUser').to(currentUser);

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 1, 2019

Member

I don't see why is this change needed as there don't seem to be any tests depending on this new behavior. What's your intention here? Can we revert changes in this file?

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Apr 1, 2019

Author Member

It's a mock-up return a binding object.

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 4, 2019

Member

Sure, I understand that, but I don't see any test in this file that would use that binding object. What's the point of creating & returning it then?

Show resolved Hide resolved packages/context/src/__tests__/unit/context.unit.ts Outdated
Show resolved Hide resolved packages/context/src/__tests__/unit/context.unit.ts Outdated
Show resolved Hide resolved packages/context/src/__tests__/unit/context.unit.ts Outdated
Show resolved Hide resolved packages/context/src/__tests__/unit/context.unit.ts Outdated
Show resolved Hide resolved packages/context/src/inject.ts

@raymondfeng raymondfeng force-pushed the improve-inject-setter branch from 623cf16 to 9da57f1 Apr 1, 2019

@raymondfeng raymondfeng requested a review from bajtos Apr 1, 2019

@raymondfeng raymondfeng changed the title feat(context): improve @inject.setter feat(context): improve @inject.setter and add @inject.binding Apr 1, 2019

@bajtos

This comment has been minimized.

Copy link
Member

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.

u => (currentUser = u),
u => {
currentUser = u;
return new Binding('authentication.currentUser').to(currentUser);

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 4, 2019

Member

Sure, I understand that, but I don't see any test in this file that would use that binding object. What's the point of creating & returning it then?

Show resolved Hide resolved packages/context/src/inject.ts Outdated

@raymondfeng raymondfeng force-pushed the improve-inject-setter branch from 2e40798 to 9be64e0 Apr 4, 2019

@raymondfeng raymondfeng requested a review from bajtos Apr 4, 2019

@raymondfeng raymondfeng force-pushed the improve-inject-setter branch from 9be64e0 to c7173e2 Apr 4, 2019

@raymondfeng

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

@bajtos PTAL

@raymondfeng

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2019

@bajtos Ping.

@dhmlau dhmlau referenced this pull request Apr 10, 2019

Closed

Monthly Milestone - April 2019 馃尶馃惏鈽旓笍 #2669

20 of 46 tasks complete
@bajtos

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

What about the bindingCreation option?

This option is configured at DI/IoC level in @inject.setter arguments, it makes sense to keep it available. As I see it, it allows the developer that's configuring DI wiring to tweak the behavior of the setter that will be injected.

@bajtos

bajtos approved these changes Apr 12, 2019

Copy link
Member

left a comment

馃憦

Show resolved Hide resolved packages/context/src/inject.ts
feat(context): add `@inject.binding` and improve `@inject.setter`
1. The decorators now allow binding creation policy control
2. `@inject.binding` can be used to resolve/configure a binding

@raymondfeng raymondfeng force-pushed the improve-inject-setter branch from c7173e2 to 8c29301 Apr 12, 2019

@raymondfeng raymondfeng merged commit a396274 into master Apr 12, 2019

4 checks passed

clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 91.257%
Details

@raymondfeng raymondfeng deleted the improve-inject-setter branch Apr 12, 2019

@dhmlau dhmlau added this to the April 2019 milestone milestone Apr 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.