-
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
feat(context): add support for binding configuration and injection #2259
Conversation
e3aa58a
to
a2a9c1a
Compare
@raymondfeng In #983, we had a long discussion about different aspects and user requirements for configuration system. How is this pull request different from #983? Can you also explain which comments you have addressed (and how), and which parts you decided to leave unaddressed? |
I removed the magic to resolve config values following the namespace hierarchy. The implementation now only uses a naming convention to find the bound config for a given binding key. |
c388192
to
d9664c0
Compare
Cool, that should make the review easier. How do you imagine the bigger picture and the end-to-end usage of this new feature? Few stories to consider:
I think it's important to start from the outside (work top-to-bottom) and think about the intended end-to-end user experience first. It may be best to start with a spike that will allow you to iterate faster, make it easier for all of us to keep the discussion at high level & focused on the intended user experience and internal design. |
d9664c0
to
83ab90d
Compare
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.
We really need to answer the question of how app users are going to provide the configuration before we go deeper into implementation details.
ca95224
to
e4ed5f1
Compare
I agree with the use cases but they are out of the scope of this PR. The purpose of this PR should be very simple (precondition: there are other tiers that load/populate the configuration into the context chain) based on requirements from #2249:
|
6d0f1f5
to
94f7a3d
Compare
Sure, I agree it's good to work in small increments and thus keep the scope of this pull request small. My concern is that you are defining foundational building blocks for the app/extension configuration, but we don't know if they will be able to support our requirements. Once this new feature is published as part of our public API, it will become difficult to make breaking changes. I am not asking you to create a full implementation for the use cases I have described above, a high-level description of the indented solution is good enough. For example: As an LB4 app developer, I'd like to have a single JSON file to define all configuration entries. How are we going to map from a JSON format to the API proposed in this pull request? We can define a convention where the binding key is used as the key in the JSON file too. E.g. {
"rest.port": 3000,
"rest.host": "localhost",
} Is it just me who see a similarity with Java property files? Maybe we should use nesting instead of dot-separated property names? Anyhow, that's just an implementation detail we can figure out later. As an extension developer, I may want my extension to provide default configuration options that can be later overridden by app-level config provided by the user. Can we rely on the order in which the configuration bindings are created? class MyApp {
constructor() {
app.component(RestComponent); // defines the default configuration
// custom configuration - overrides the config values bound by components
}
} Would this become a pain point and we will need to look for different ways how to allow extensions to provide default values? As an extension developer, I may want to contribute configuration for artifacts bound by a different extension. A contrived example: And here it becomes tricky. If we rely on the order in which the configuration was defined, then we need to ensure that Thoughts? As an extension developer, I want to read configuration of bindings bound by a different extension. For example, I think this is a similar problem. If we rely on the order in which components are registered, then the default configuration provided by Now we have a question very relevant to your pull request: do we want to support both flavors of config injection (value vs getter)? I am arguing that we should always inject a getter function to support configuration changes made after the config was resolved. For example, in your greeter extension example shown in #2249, app.configure('greeter-extension-point').to({color: false});
(await app.get('greeter-extension-point')).greet('en', 'monochrome');
app.configure('greeter-extension-point').to({color: true});
(await app.get('greeter-extension-point')).greet('en', 'rainbow');
// ^^ this does not pick up the new setting!!! If we change Let's discuss. |
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.
Few comments on implementation details, I haven't reviewed the tests yet.
packages/context/src/inject.ts
Outdated
@@ -331,21 +395,28 @@ export function describeInjectedArguments( | |||
return meta || []; | |||
} | |||
|
|||
function resolveByTag( | |||
function resolveBindings( |
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.
Similarly here, I don't see how is this change relevant to binding configuration. Can you revert it please?
packages/context/src/context.ts
Outdated
); | ||
} | ||
|
||
private getConfigResolver() { |
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 find it ugly to have both configResolver
as a property and getConfigResolver
as a getter function. Have you considered initializing this.configResolver
right in the constructor?
configPath, | ||
); | ||
|
||
const options: ResolutionOptions = Object.assign( |
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.
How about the following?
const options: ResolutionOptions = {optional: true, ...resolutionOptions};
packages/context/src/__tests__/acceptance/binding-config.feature.md
Outdated
Show resolved
Hide resolved
docs/site/Context.md
Outdated
|
||
The following APIs are available to enforce/leverage this convention: | ||
|
||
1. ctx.configure('servers.RestServer.server1') => Binding for the configuration |
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 concerned that in order to contribute configuration, the code needs a reference to the target Context instance. It creates a coupling that makes it difficult for extensions to contribute configuration bindings.
Can you add API for creating standalone configuration bindings (no context instance required)? Perhaps a short-cut leveraging configBindingKeyFor
helper?
For example, we can add a static method Binding.configure
that will create a new Binding
instance with the correct configuration key:
// in a component, export the following binding:
class MyComponent {
bindings: [
// some other bindings
Binding.configure('servers.RestServer.server1').to({port: 0})
],
});
// in mountComponent - already in place
if (component.bindings) {
for (const binding of component.bindings) {
app.add(binding);
}
}
I am not sure if Binding.configure
is the right name, feel free to pick a better one!
I am also fine to leave out such new helper out of scope of this pull request, as long as the documentation explains how to create standalone Binding instances for configuration (e.g. using configBindingKeyFor
)
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.
Good idea. I'll add Binding.configure
.
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.
Good idea. I'll add
Binding.configure
Please describe the new API in the documentation too. (E.g. add it to the list here.)
the configuration value to be a deep property of the bound value. For example, | ||
`@config('port')` injects `RestServerConfig.port` to the target. | ||
|
||
```ts |
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.
+1
The example is missing code showing how RestServer
is calling @config('port')
.
// configuration | ||
ctx | ||
.configure('servers.rest.server1') | ||
.toAlias(configBindingKeyFor('application', 'rest')); |
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.
Please note
@inject
does not leveragesession.currentBinding
.
This is very important, please describe the difference in the documentation.
packages/context/src/__tests__/acceptance/binding-config.acceptance.ts
Outdated
Show resolved
Hide resolved
I was trying to make it symmetric with |
7eb1add
to
48fca08
Compare
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.
Almost there!
Besides the few remaining comments below, please structure the commit history in such way that there are two standalone commits for the addition of ContextView.prototype.singleValue()
and Context.prototype.getOwnerContext()
APIs. I'd like to see dedicated CHANGELOG entries.
docs/site/Context.md
Outdated
|
||
The following APIs are available to enforce/leverage this convention: | ||
|
||
1. ctx.configure('servers.RestServer.server1') => Binding for the configuration |
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.
Good idea. I'll add
Binding.configure
Please describe the new API in the documentation too. (E.g. add it to the list here.)
docs/site/Context.md
Outdated
2. ctx.getConfig('servers.RestServer.server1') => Get configuration | ||
3. `@config` to inject corresponding configuration | ||
4. `@config.getter` to inject a getter function for corresponding configuration | ||
5. `@config.view` to inject a `ContextView` for corresponding configuration |
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.
Please explain that:
All configuration accessors (
ctx.getConfig
,@config*
) consider the configuration binding as optional, i.e. returnundefined
if no configuration was bound. This is different fromctx.get
and@inject
APIs, which treat bindings as required and throw an error when the requested binding is not found.
I see that you have documented this binding in API docs. Can you please describe it in docs/site/Context.md
too? In my experience, most people are not going to read our API docs.
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.
Reposting two older comments that seem to get lost.
b5019a5
to
bc3a0f3
Compare
I'll do during squashing. |
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.
LGTM.
Please get @hacksparrow's approval before landing.
7107ed0
to
28db47c
Compare
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 have some questions, but overall LGTM
packages/context/src/__tests__/acceptance/class-level-bindings.acceptance.ts
Show resolved
Hide resolved
packages/context/src/__tests__/acceptance/class-level-bindings.acceptance.ts
Show resolved
Hide resolved
expect(store.settings).to.be.undefined(); | ||
}); | ||
|
||
it('injects config from config 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.
How is this test different than https://github.com/strongloop/loopback-next/pull/2259/files#diff-cf5075a2ae88e5e65480d792a8a2b738R698?
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.
It's pretty similar. In this case, only one property x
is used.
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.
Ok was wondering if we could just reduce it to one test, but ok with both.
it('supports singleValue() if only one value exist', async () => { | ||
server.unbind('bar'); | ||
expect(await taggedAsFoo.singleValue()).to.eql('FOO'); | ||
}); |
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.
+1 (ignore if feedback applied).
@@ -533,6 +533,21 @@ describe('Context', () => { | |||
}); | |||
}); | |||
|
|||
describe('getOwnerContext', () => { |
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.
+1 (ignore if feedback applied).
@@ -45,6 +45,17 @@ describe('ContextView', () => { | |||
expect(await taggedAsFoo.asGetter()()).to.eql(['BAR', 'FOO']); | |||
}); | |||
|
|||
it('reports error on singleValue() if multiple values exist', async () => { | |||
return expect(taggedAsFoo.singleValue()).to.be.rejectedWith( |
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'm curious why we wanted to have the singleValue()
method in the first place?
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.
It's for dynamic configuration - @config.view
. See acceptance tests for refs.
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.
Okay, but we can still use values()
for that too no?
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.
Yes, we can still use values()
. singleValue()
is just a helper.
packages/context/src/__tests__/acceptance/class-level-bindings.acceptance.ts
Show resolved
Hide resolved
if (isPromiseLike(valueOrPromise)) { | ||
const prop = configPath ? ` property ${configPath}` : ''; | ||
throw new Error( | ||
`Cannot get config${prop} for ${key} synchronously: the value is a promise`, |
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.
Nitpick: should we point devs to use getConfig
instead?
735685c
to
49805dc
Compare
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.
👏
49805dc
to
0db93e5
Compare
Reactivation of #983 based on requirements illustrated by #2249.
@config
,@config.getter
, and@config.view
to receive injection of configurationChecklist
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated