feat(context): add support for binding configuration and injection #2259

raymondfeng · 2019-01-16T21:09:54Z

Reactivation of #983 based on requirements illustrated by #2249.

Add context.configure() to configure bindings
Add context.getConfig/getConfigSync() to look up configuration for a binding
Add @config, @config.getter, and @config.view to receive injection of configuration

Checklist

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

bajtos · 2019-01-18T08:50:36Z

@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?

raymondfeng · 2019-01-18T13:00:12Z

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.

bajtos · 2019-01-29T15:32:21Z

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

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:

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? Maybe you have a different approach in mind?
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.
As an extension developer, I may want to contribute configuration for artifacts bound by a different extension. A contrived example: @loopback/rest-explorer may want to override certain configuration of @loopback/rest.
As an extension developer, I want to read configuration of bindings bound by a different extension. For example, @loopback/rest-explorer needs to find out the configuration controlling /openapi.json URL.

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.

bajtos

We really need to answer the question of how app users are going to provide the configuration before we go deeper into implementation details.

packages/context/src/resolution-session.ts

raymondfeng · 2019-02-12T18:33:11Z

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? Maybe you have a different approach in mind?

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.

As an extension developer, I may want to contribute configuration for artifacts bound by a different extension. A contrived example: @loopback/rest-explorer may want to override certain configuration of @loopback/rest.

As an extension developer, I want to read configuration of bindings bound by a different extension. For example, @loopback/rest-explorer needs to find out the configuration controlling /openapi.json URL.

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:

Define the convention to configure a bound entry in the context (the configuration should be decoupled from the target, both of them can be bound independently). This allows us to leverage our IoC/DI container for configuration resolution.
Define sugar APIs on Context to provide/consume configuration for a given binding (configure() and getConfig())
Define a @inject.config decorator to inject corresponding configuration into a class without the need to know the binding key.

bajtos · 2019-02-15T14:14:29Z

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:

Define the convention to configure a bound entry in the context (the configuration should be decoupled from the target, both of them can be bound independently). This allows us to leverage our IoC/DI container for configuration resolution.

Define sugar APIs on Context to provide/consume configuration for a given binding (configure() and getConfig())

Define a @inject.config decorator to inject corresponding configuration into a class without the need to know the binding key.

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: @loopback/rest-explorer may want to override certain configuration of @loopback/rest.

And here it becomes tricky. If we rely on the order in which the configuration was defined, then we need to ensure that @loopback/rest component is mounted first, @loopback/rest-explorer second.

Thoughts?

As an extension developer, I want to read configuration of bindings bound by a different extension. For example, @loopback/rest-explorer needs to find out the configuration controlling /openapi.json URL.)

I think this is a similar problem. If we rely on the order in which components are registered, then the default configuration provided by @loobpack/rest will be available at the time when @loobpack/rest-explorer is being mounted. However, this will not take into account configuration supplied by the application for @loopback/rest after @loopback/rest-explorer was mounted. I think that means we need to use a getter function instead of obtaining the configured value directly, similarly to how we are using @inject.getter for current authenticated user.

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, GreeterExtensionPoint is bound in SINGLETON scope. Once the binding is resolved and the singleton is created, there is no way how to update it when the configuration changes. The following code will not work:

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 @inject.config to always resolve to a getter function, we force all configuration consumers to support config updates from the start.

Let's discuss.

bajtos

Few comments on implementation details, I haven't reviewed the tests yet.

packages/context/src/inject.ts

bajtos · 2019-02-15T14:16:33Z

packages/context/src/inject.ts

@@ -331,21 +395,28 @@ export function describeInjectedArguments(
  return meta || [];
 }

-function resolveByTag(
+function resolveBindings(


Similarly here, I don't see how is this change relevant to binding configuration. Can you revert it please?

packages/context/src/inject.ts

packages/context/src/context.ts

bajtos · 2019-02-15T14:22:31Z

packages/context/src/context.ts

+    );
+  }
+
+  private getConfigResolver() {


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?

bajtos · 2019-02-15T14:24:32Z

packages/context/src/binding-config.ts

+      configPath,
+    );
+
+    const options: ResolutionOptions = Object.assign(


How about the following?

const options: ResolutionOptions = {optional: true, ...resolutionOptions};

packages/context/src/__tests__/acceptance/binding-config.feature.md

bajtos · 2019-05-14T11:09:20Z

docs/site/Context.md

+
+The following APIs are available to enforce/leverage this convention:
+
+1. ctx.configure('servers.RestServer.server1') => Binding for the configuration


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)

Good idea. I'll add Binding.configure.

Good idea. I'll add Binding.configure

Please describe the new API in the documentation too. (E.g. add it to the list here.)

bajtos · 2019-05-14T11:10:12Z

docs/site/Context.md

+the configuration value to be a deep property of the bound value. For example,
+`@config('port')` injects `RestServerConfig.port` to the target.
+
+```ts


+1

The example is missing code showing how RestServer is calling @config('port').

bajtos · 2019-05-14T11:13:10Z

packages/context/src/__tests__/acceptance/binding-config.acceptance.ts

+    // configuration
+    ctx
+      .configure('servers.rest.server1')
+      .toAlias(configBindingKeyFor('application', 'rest'));


Please note @inject does not leverage session.currentBinding.

This is very important, please describe the difference in the documentation.

packages/context/src/__tests__/acceptance/binding-config.acceptance.ts

packages/context/src/binding-key.ts

packages/context/src/context-view.ts

packages/context/src/context.ts

raymondfeng · 2019-05-14T18:33:01Z

Is it a good idea to introduce the sync version? I mean what are the use cases where await ctx.getConfig() cannot be used?

I was trying to make it symmetric with get/getSync for bindings.

bajtos

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.

bajtos · 2019-05-21T12:30:42Z

docs/site/Context.md

+
+The following APIs are available to enforce/leverage this convention:
+
+1. ctx.configure('servers.RestServer.server1') => Binding for the configuration


Good idea. I'll add Binding.configure

Please describe the new API in the documentation too. (E.g. add it to the list here.)

bajtos · 2019-05-21T12:37:49Z

docs/site/Context.md

+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


Please explain that:

All configuration accessors (ctx.getConfig, @config*) consider the configuration binding as optional, i.e. return undefined if no configuration was bound. This is different from ctx.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.

packages/context/src/keys.ts

packages/context/src/context-view.ts

bajtos

Reposting two older comments that seem to get lost.

packages/context/src/context.ts

packages/context/src/binding-config.ts

raymondfeng · 2019-05-21T15:00:03Z

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.

I'll do during squashing.

bajtos

LGTM.

Please get @hacksparrow's approval before landing.

docs/site/Context.md

packages/context/src/context.ts

b-admike

I have some questions, but overall LGTM

packages/context/src/__tests__/acceptance/binding-config.acceptance.ts

packages/context/src/__tests__/acceptance/binding-config.feature.md

packages/context/src/__tests__/acceptance/class-level-bindings.acceptance.ts

b-admike · 2019-05-22T21:20:39Z

packages/context/src/__tests__/acceptance/class-level-bindings.acceptance.ts

+    expect(store.settings).to.be.undefined();
+  });
+
+  it('injects config from config binding', () => {


How is this test different than https://github.com/strongloop/loopback-next/pull/2259/files#diff-cf5075a2ae88e5e65480d792a8a2b738R698?

It's pretty similar. In this case, only one property x is used.

Ok was wondering if we could just reduce it to one test, but ok with both.

b-admike · 2019-05-22T21:23:49Z

packages/context/src/__tests__/unit/context-view.unit.ts

+  it('supports singleValue() if only one value exist', async () => {
+    server.unbind('bar');
+    expect(await taggedAsFoo.singleValue()).to.eql('FOO');
+  });


+1 (ignore if feedback applied).

b-admike · 2019-05-22T21:24:08Z

packages/context/src/__tests__/unit/context.unit.ts

@@ -533,6 +533,21 @@ describe('Context', () => {
    });
  });

+  describe('getOwnerContext', () => {


+1 (ignore if feedback applied).

b-admike · 2019-05-22T21:24:42Z

packages/context/src/__tests__/unit/context-view.unit.ts

@@ -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(


I'm curious why we wanted to have the singleValue() method in the first place?

It's for dynamic configuration - @config.view. See acceptance tests for refs.

Okay, but we can still use values() for that too no?

Yes, we can still use values(). singleValue() is just a helper.

packages/context/src/inject-config.ts

packages/context/src/__tests__/acceptance/class-level-bindings.acceptance.ts

b-admike · 2019-05-22T21:50:26Z

packages/context/src/context.ts

+    if (isPromiseLike(valueOrPromise)) {
+      const prop = configPath ? ` property ${configPath}` : '';
+      throw new Error(
+        `Cannot get config${prop} for ${key} synchronously: the value is a promise`,


Nitpick: should we point devs to use getConfig instead?

b-admike

👏

1. Add context.configure to configure bindings 2. Add context.getConfig to look up configuration for a binding 3. Add @config.* to receive injection of configuration - @config - @config.getter - @config.view 4. Add docs for context/binding configuration

raymondfeng requested a review from bajtos as a code owner January 16, 2019 21:09

raymondfeng force-pushed the simpler-binding-options branch 4 times, most recently from e3aa58a to a2a9c1a Compare January 17, 2019 18:02

raymondfeng mentioned this pull request Jan 22, 2019

feat(core): introduce basic life cycle support for LoopBack applications #1928

Merged

7 tasks

raymondfeng force-pushed the simpler-binding-options branch 2 times, most recently from c388192 to d9664c0 Compare January 26, 2019 17:43

dhmlau added this to the February 2019 milestone milestone Jan 28, 2019

dhmlau mentioned this pull request Jan 30, 2019

Monthly Milestone - February 2019 💝⛄️ #2313

Closed

37 tasks

bajtos mentioned this pull request Feb 1, 2019

Sugar APIs for creating controllers, models, and repositories strongloop/loopback4-example-javascript#3

Closed

raymondfeng force-pushed the simpler-binding-options branch from d9664c0 to 83ab90d Compare February 5, 2019 19:20

raymondfeng mentioned this pull request Feb 7, 2019

feat(context): introduce context events and observers for bind/unbind #2291

Merged

7 tasks

bajtos mentioned this pull request Feb 12, 2019

feat: create jwt auth service loopbackio/loopback4-example-shopping#33

Merged

bajtos added feature IoC/Context @loopback/context: Dependency Injection, Inversion of Control labels Feb 12, 2019

bajtos suggested changes Feb 12, 2019

View reviewed changes

packages/context/src/resolution-session.ts Outdated Show resolved Hide resolved

bajtos requested review from jannyHou, b-admike and hacksparrow February 12, 2019 15:31

raymondfeng force-pushed the simpler-binding-options branch 2 times, most recently from ca95224 to e4ed5f1 Compare February 12, 2019 17:30

raymondfeng force-pushed the simpler-binding-options branch from 6d0f1f5 to 94f7a3d Compare February 12, 2019 21:18

bajtos reviewed Feb 15, 2019

View reviewed changes

raymondfeng mentioned this pull request Feb 15, 2019

How to pass options to a class using @inject #2407

Closed

dhmlau mentioned this pull request Apr 30, 2019

Monthly Milestone - May 2019 🌷🐝 🍄 #2818

Closed

32 tasks

bajtos modified the milestones: April 2019 milestone, May 2019 milestone May 2, 2019

bajtos reviewed May 14, 2019

View reviewed changes

raymondfeng force-pushed the simpler-binding-options branch 3 times, most recently from 7eb1add to 48fca08 Compare May 14, 2019 19:28

bajtos reviewed May 21, 2019

View reviewed changes

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

packages/context/src/binding-config.ts Show resolved Hide resolved

raymondfeng force-pushed the simpler-binding-options branch 2 times, most recently from b5019a5 to bc3a0f3 Compare May 21, 2019 14:59

bajtos approved these changes May 21, 2019

View reviewed changes

docs/site/Context.md Outdated Show resolved Hide resolved

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

raymondfeng force-pushed the simpler-binding-options branch 3 times, most recently from 7107ed0 to 28db47c Compare May 21, 2019 19:45

jannyHou approved these changes May 22, 2019

View reviewed changes

b-admike reviewed May 22, 2019

View reviewed changes

raymondfeng force-pushed the simpler-binding-options branch 2 times, most recently from 735685c to 49805dc Compare May 22, 2019 22:13

b-admike approved these changes May 22, 2019

View reviewed changes

hacksparrow approved these changes May 23, 2019

View reviewed changes

raymondfeng added 3 commits May 23, 2019 06:49

feat(context): add singleValue to ContextView

5e81363

test(context): add a unit test for context.getOwnerContext

514a2cb

raymondfeng force-pushed the simpler-binding-options branch from 49805dc to 0db93e5 Compare May 23, 2019 13:56

raymondfeng merged commit a392852 into master May 23, 2019

raymondfeng deleted the simpler-binding-options branch May 23, 2019 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(context): add support for binding configuration and injection #2259

feat(context): add support for binding configuration and injection #2259

raymondfeng commented Jan 16, 2019 •

edited

Loading

bajtos commented Jan 18, 2019

raymondfeng commented Jan 18, 2019

bajtos commented Jan 29, 2019

bajtos left a comment

raymondfeng commented Feb 12, 2019 •

edited

Loading

bajtos commented Feb 15, 2019 •

edited

Loading

bajtos left a comment

bajtos Feb 15, 2019

bajtos Feb 15, 2019

bajtos Feb 15, 2019

bajtos May 14, 2019

raymondfeng May 14, 2019

bajtos May 21, 2019

bajtos May 14, 2019

bajtos May 14, 2019

raymondfeng commented May 14, 2019

bajtos left a comment

bajtos May 21, 2019

bajtos May 21, 2019

bajtos left a comment •

edited

Loading

raymondfeng commented May 21, 2019

bajtos left a comment

b-admike left a comment

b-admike May 22, 2019

raymondfeng May 22, 2019

b-admike May 22, 2019

b-admike May 22, 2019

b-admike May 22, 2019

b-admike May 22, 2019

raymondfeng May 22, 2019

b-admike May 22, 2019

raymondfeng May 23, 2019

b-admike May 22, 2019

b-admike left a comment


		The following APIs are available to enforce/leverage this convention:

		1. ctx.configure('servers.RestServer.server1') => Binding for the configuration

feat(context): add support for binding configuration and injection #2259

feat(context): add support for binding configuration and injection #2259

Conversation

raymondfeng commented Jan 16, 2019 • edited Loading

Checklist

bajtos commented Jan 18, 2019

raymondfeng commented Jan 18, 2019

bajtos commented Jan 29, 2019

bajtos left a comment

Choose a reason for hiding this comment

raymondfeng commented Feb 12, 2019 • edited Loading

bajtos commented Feb 15, 2019 • edited Loading

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng commented May 14, 2019

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos left a comment • edited Loading

Choose a reason for hiding this comment

raymondfeng commented May 21, 2019

bajtos left a comment

Choose a reason for hiding this comment

b-admike left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-admike left a comment

Choose a reason for hiding this comment

raymondfeng commented Jan 16, 2019 •

edited

Loading

raymondfeng commented Feb 12, 2019 •

edited

Loading

bajtos commented Feb 15, 2019 •

edited

Loading

bajtos left a comment •

edited

Loading