-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[WIP] feat(context): Allow components to expose a list of bindings #929
Conversation
@raymondfeng what is the expected behavior when const ctx1 = new Context();
const ctx2 = new Context();
const b = ctx1.bind('foo').to('bar');
ctx2.bind(b2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@raymondfeng I am concerned about the downsides of allowing users to add Binding
instances directly to the context.
What high-level problem (end-to-end scenario) are you trying to address by this patch?
What other solutions have you considered and rejected?
packages/context/src/context.ts
Outdated
*/ | ||
bind(key: string): Binding { | ||
bind(keyOrBinding: string | Binding): Binding { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not very happy about this method overload. So far, bind
is always followed with .to{something}()
and possibly other modifiers like inScope
, etc. Do you expect people will call those modifiers when binding a Binding directly? I don't.
Instead of overloading the existing bind
method, I am proposing to add a new method called add
. That way we have to distinctive usage patterns:
ctx.bind('foo').to('bar').inScope(BindingScope.SINGLETON);
const b = new Binding('foo').to('bar').inScope(BindingScope.SINGLETON);
ctx.add(b);
@@ -17,6 +17,13 @@ describe('Context', () => { | |||
expect(result).to.be.true(); | |||
}); | |||
|
|||
it('accepts a binding', () => { | |||
const binding = new Binding('foo').to('bar'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code looks rather silly to me, as it reads new binding 'foo' to 'bar'.
IMO, the API of Binding
class was not designed to support usage outside of Context
and we need to fix this problem before allowing components to contribute new bindings via Binding
instance.
0e8b945
to
543c305
Compare
@bajtos The main use case is to contribute bindings from a component (without accessing the context). The bindings need to be set up (such as scope or value) before they are bound to the app later. See https://github.com/strongloop/loopback-next/blob/543c30509f3a28d523c7724c2fd422fee99dc40d/packages/core/src/component.ts#L82. Alternative solutions are:
|
We're addressing it with 62fda01. |
I agree this is an important and valid requirement that we need to address, and I also agree that the current status (only Providers can be provided by components for DI registration) is too limited. At the same time, these improvements is out of scope of our MVP release, therefore we should postpone them until the MVP is done. You pull request is a reasonable incremental improvement. However, I am concerned that the current design of extensions is sort of a dead end and the changes proposed here are only digging a deeper hole we will have to get out of. Let's talk about that in #953, so that the discussion in this pull request can stay focused on your proposed code changes. |
543c305
to
c45990a
Compare
c45990a
to
f10ecb5
Compare
f10ecb5
to
a39cc92
Compare
10b703c
to
b3fcd6d
Compare
b3fcd6d
to
e6eba3a
Compare
There have been no updates in this pull request for many months, I am closing it for now. @raymondfeng feel free to reopen if you get time to continue this work. (Considering how outdated these changes are, it may be better to start from scratch.) |
The PR adds a few APIs:
Checklist
npm test
passes on your machinepackages/cli
were updatedpackages/example-*
were updated