feat(core): improve component bindings

[raymondfeng]

Reactivation of #929

• Improve the component design to allow contribution of bindings and classes.

A component class can be declared as follows:

    class MyComponent implements Component {
      bindings = [Binding.bind('x').to.(1)];
      classes = {'binding-key-for-my-class': MyClass};
      constructor() {}
    }

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]

Improve the component design to allow contribution of various bindings

+1, this part makes sense to me. Can we have a dedicated pull request for this feature please? We should discuss documentation updates, descriptive API naming, etc. as part of the review.

Introduce a @component decorator to mark a component class

I don't understand the benefits of this change. I believe the example shown above can be easily rewritten without @component decorator as follows:

    class MyComponent implements Component {
      controllers: [MyController],
      bindings = [binding];
      classes: {'my-class': MyClass},
      providers: {'my-provider': MyProvider},

      constructor() {
        // Dynamically add more bindings
        this.bindings.push(...);
      }
    }

What benefits do you @raymondfeng see in the @component() based solution?

I don't want to introduce two ways how to accomplish the same thing, because that usually confuse users. Let's pick one way that we think is the best, make it easy to use and well documented.

BTW I think the current approach for building components is too cumbersome and we should look into ways how to leverage @loopback/boot to load component artifacts based on conventions. For example:

// provided by LB
class ComponentBase {
  // implements Application-like APIs for registering artifacts
  controller(ctor) {
    this.controllers.push(ctor);
  }
}

// in an extension
import {boot} from '@loopback/boot';

class MyExtension extends BootMixin(RepositoryMixin(ServiceMixin(ComponentBase)) {
  boot() {
    boot(this, {/* boot options */});
  }
}

As long as both Application and ComponentBase provide the same API for registering artifacts, we should be able to use the same Booter implementations for both applications and components. How awesome would that be for extension developers!

Let me open a new issue to discuss that idea: #1968

[bajtos]

Let's reduce the scope of this pull request please and start with the following feature: Improve the component design to allow contribution of various bindings.

[bajtos]

As I was thinking about this some more and in the light of #1968, I think it would be best if the code example shown in the PR description was instead written this way:

    class MyComponent extends Component {
      constructor() {
        this.controller(MyController);
        this.bind('foo').to('bar');
        this.bind('my-class').toClass(MyClass);
        this.bind('my-provider').toProvider(MyProvider);
     }
  }

I see few important benefits in such approach:

• Extension developers can use the APIs they are already familiar with from building Applications. Less APIs to learn make LB users more productive.
• I find the originally proposed extension properties like classes and providers difficult to understand. In my proposal, we reuse the existing Binding APIs - no need to invent new terminology.
• By leveraging existing Application and Binding APIs, we have less new APIs to document, test and maintain for the future.
• Later on, we can leverage @loopback/boot to load and register artifacts from source files automatically, based on project layout conventions.

[raymondfeng]

@bajtos I think the different options boil down to declarative vs imperative. I like the new style of using Component as a base class. I assume the Component itself is Context.

IIRC, I proposed before that each component becomes a context to group its contributions. If that happens, the application will becomes:

• app (Application extends Component)
• rest component
• explorer component

We also need to find a way to aggregate all contexts, either by merging or delegating.

[raymondfeng]

BTW, we can do something similar today:

export class MyComponent {
      constructor(@inject(CoreBinding.APPLICATION_INSTANCE) app: Application) {
        app.controller(MyController);
        app.bind('foo').to('bar');
        app.bind('my-class').toClass(MyClass);
        app.bind('my-provider').toProvider(MyProvider);
     }
  }

[bajtos]

I think the different options boil down to declarative vs imperative. I like the new style of using Component as a base class. I assume the Component itself is Context.

IIRC, I proposed before that each component becomes a context to group its contributions. If that happens, the application will becomes:

• app (Application extends Component)
• rest component
• explorer component

We also need to find a way to aggregate all contexts, either by merging or delegating.

I don't think it's a good idea for Component to be also a Context:

• IMO, merging/aggregation of multiple context hierarchies is a complex problem that's difficult to implement correctly (think about multiple inheritance and the diamond problem). We have discussed this in the PR where you proposed an implementation merging contexts.
• By giving Components access to APIs for binding resolution (e.g. ctx.get()), we make it easy for developers to shoot into their own foot. Typically, the Component context need to inherit bindings like CoreBinding.APPLICATION_INSTANCE from the Application context. Until the Component context is attached to the Application, these bindings cannot be resolved though. I expect there may be more similar edge cases.

Here is what I am proposing:

(1) Let's keep the contract between Application and Component simple. The component exports an array of Binding instances. As I see it, this is the simplest solution that provides all the power components may need. In my experience, simpler is usually better.

(2) Give us and the community freedom to experiment with different ways of building/setting-up components. Let the solution described in my comment above (and little bit later here as well) be only one of the possible ways.

(3) As for my proposal: my vision is to conceptually extract Context + Application APIs into two groups:

1. APIs for defining/registering new bindings: ctx.bind(), app.controller(), etc.
2. APIs for consuming/resolving bindings: ctx.get(), @inject, etc.

So far, app.controller() and friends are just sugar methods wrapping ctx.bind(). Let's extract them from the Application class into a new mixin.

As for components, let's implement a base component class that provides bind() API required by controller() but also repository(), dataSource and other helpers contributed by mixins. Under the hood, bind() can push a new binding to this.bindings[] array instead of using Context-based registration.

interface Component {
  // ...
  bindings?: Binding[];
}

interface BindingBuilder {
  bind(key: string): Binding;
}

function AppArtifactRegistrationMixin<T extends Class<any>>(superClass: T) {
  return class extends superClass {
    controller(controllerCtor: ControllerClass, name?: string): Binding {
      name = name || controllerCtor.name;
      return this.bind(`controllers.${name}`)
        .toClass(controllerCtor)
        .tag('controller');
    }
    // etc.
  };
}

class ComponentBindingBuilder implements BindingBuilder {
  bindings: Binding[] = [];

  bind(key: string): Binding {
    const binding = new Binding(key);
    this.bindings.push(binding);
    return binding;
  }
}

class ComponentBase implements Component
  extends AppArtifactRegistrationMixin(ComponentBindingBuilder) {
}

// and also in the Application class:
class Application extends AppArtifactRegistrationMixin(Context) {
  // ...
}

[bajtos]

BTW, we can do something similar today:

export class MyComponent {
     constructor(@inject(CoreBinding.APPLICATION_INSTANCE) app: Application) {
//...

Honestly, this seems a bit hacky to me, I consider this as a (hopefully short-term) workaround until we enhance Component interface to support all kinds of artifacts.

On the other hand, maybe this is a sign that we need to revisit Component design? In the current design, whenever we add a new artifact to Application/RestApplication/etc. (e.g. static assets), we also need to find a way how to enable components to contribute these artifacts too.

In the example you have shown, the components are manipulating the application directly, thus there is no need to enhance the Component API contract to support new features.

I guess it boils down to declarative vs imperative as you wrote yourself.

If we feel that declarative support is something we should support, then I think we should improve our design to make this more obvious/easier to use. For example, the component constructor can receive the Application instance directly in the constructor without any DI.

This raises a question though: do we want to support both declarative and imperative approaches to defining components? Personally, I prefer to choose only one approach, to keep the maintenance costs low. If we support both approaches, then we have more documentation to write and maintain, more questions to answer because people will be confused about which approach to choose, etc.

@strongloop/loopback-maintainers thoughts? (See also my previous comment proposing a solution how to allow extensions to contribute arbitrary bindings.)

[bajtos]

@raymondfeng To move this pull request forward: I think we both agree that it's a good idea to allow components to export bindings: Binding[]. Let's land this feature first, in isolation from the other parts proposed in this pull request, and then discuss how to continue building on top of bindings: Binding[].

[raymondfeng]

@bajtos I reduced the scope of this PR to add bindings support. PTAL.

[bajtos]

Nice 👏

In addition to my comments below, please add some documentation for this new feature. For example, you can add a new section to https://github.com/strongloop/loopback-next/blob/master/docs/site/Creating-components.md. Please check out if there are any other places where to mention this new feature and/or cross-link to the new content you will write.

[bajtos]

A nitpick: I think = BoundValue is not needed, because the specialization of add can be always inferred by the compiler from the specialization of the binding argument.

[bajtos]

@raymondfeng PTAL - is the default value for ValueType needed?

[raymondfeng]

I simplified it as add(binding: Binding<unknown>): this {. The same is also applied to a few other methods.

[bajtos]

I find this name confusing. What does it mean for a component to export a class? Some classes are exported via JavaScript/TypeScript exports, e.g. export class FooBar. Why should be some classes exported via JS/TS and some others exported via Component properties?

Personally, I prefer to remove this API entirely, keep providers for backwards compatibility (possibly even deprecate it) and ask users to leverage bindings API for all bindings.

If you feel it's important to keep this shortcut, then let's find a better name please. For example:classBindings or classesToBind.

[raymondfeng]

I added jsdocs to make it clear.

[bajtos]

I am afraid jsdocs are not good enough. Consider the following code snippet from your pull request:

      class MyComponentWithClasses implements Component {
         classes = {'my-class': MyClass};
       }

The person reading the code (e.g. while reviewing a pull request on GitHub) does not see tscode comments.

[raymondfeng]

@bajtos PTAL.

[bajtos]

The test code looks much better now!

Let's discuss the classes property more, see my comment in the thread above.

Most importantly, we need documentation for these new features.

In addition to my comments below, please add some documentation for this new feature. For example, you can add a new section to https://github.com/strongloop/loopback-next/blob/master/docs/site/Creating-components.md. Please check out if there are any other places where to mention this new feature and/or cross-link to the new content you will write.

[bajtos]

Let's improve this docs please and add an example showing how to leverage this feature (keep it consistent with tsdocs for other Component properties).

An array of bindings to be added to the application context.

[raymondfeng]

I updated to docs and tsdocs.

I still prefer to keep classes to be consistent with providers and controllers as such concepts are relative to context bindings.

[raymondfeng]

@bajtos PTAL

[bajtos]

Two more comments to address, the rest LGTM.

No further review is necessary as long as my comments are addressed (feel free to use a different solution from what I proposed).

[bajtos]

I am not sure if create ... to is grammatically correct. I think create ... in or add ... to would be better.

Suggested change

	create the following bindings to the application context:
	create the following bindings in the application context:

ping @bschrammIBM @dhmlau

[bajtos]

Let's use the newly added sugar API please.

Suggested change

	* const bindingX = new Binding('x').to('Value X');
	* const bindingX = Binding.bind('x').to('Value X');