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(core): improve component bindings #1924

Merged
merged 4 commits into from Nov 9, 2018

Conversation

Projects
None yet
2 participants
@raymondfeng
Member

raymondfeng commented Oct 26, 2018

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

@raymondfeng raymondfeng requested a review from bajtos as a code owner Oct 26, 2018

@raymondfeng raymondfeng changed the title from Component bindings to Improve component bindings Oct 26, 2018

@raymondfeng raymondfeng force-pushed the component-bindings branch from 589a5ab to 4700f7a Oct 26, 2018

@raymondfeng raymondfeng changed the title from Improve component bindings to feat(core): improve component bindings Oct 26, 2018

@raymondfeng raymondfeng force-pushed the component-bindings branch 2 times, most recently from 4a3b252 to c95f689 Oct 26, 2018

@raymondfeng raymondfeng referenced this pull request Nov 1, 2018

Closed

[WIP] feat: local API Explorer #1957

0 of 7 tasks complete
@bajtos

This comment has been minimized.

Member

bajtos commented Nov 2, 2018

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

This comment has been minimized.

Member

bajtos commented Nov 2, 2018

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

This comment has been minimized.

Member

raymondfeng commented Nov 2, 2018

@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

This comment has been minimized.

Member

raymondfeng commented Nov 2, 2018

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

This comment has been minimized.

Member

bajtos commented Nov 5, 2018

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

This comment has been minimized.

Member

bajtos commented Nov 5, 2018

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

This comment has been minimized.

Member

bajtos commented Nov 5, 2018

@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 raymondfeng force-pushed the component-bindings branch from c95f689 to c9d33ac Nov 5, 2018

@raymondfeng

This comment has been minimized.

Member

raymondfeng commented Nov 5, 2018

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

* same key, an error will be thrown.
* @param binding The configured binding to be added
*/
add<ValueType = BoundValue>(binding: Binding<ValueType>): this {

This comment has been minimized.

@bajtos

bajtos Nov 6, 2018

Member

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.

This comment has been minimized.

@bajtos

bajtos Nov 8, 2018

Member

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

This comment has been minimized.

@raymondfeng

raymondfeng Nov 8, 2018

Member

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

Show resolved Hide resolved packages/context/src/context.ts
@@ -27,13 +31,21 @@ export interface Component {
* A map of name/class pairs for binding providers
*/
providers?: ProviderMap;
classes?: ClassMap;

This comment has been minimized.

@bajtos

bajtos Nov 6, 2018

Member

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.

This comment has been minimized.

@raymondfeng

raymondfeng Nov 6, 2018

Member

I added jsdocs to make it clear.

This comment has been minimized.

@bajtos

bajtos Nov 8, 2018

Member

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.

Show resolved Hide resolved packages/core/src/component.ts Outdated
Show resolved Hide resolved packages/core/test/unit/application.unit.ts Outdated
Show resolved Hide resolved packages/core/test/unit/application.unit.ts Outdated

@raymondfeng raymondfeng force-pushed the component-bindings branch 2 times, most recently from e17ba58 to 76c5557 Nov 6, 2018

@raymondfeng

This comment has been minimized.

Member

raymondfeng commented Nov 6, 2018

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

/**
* A map of name/class pairs for servers
*/
servers?: {
[name: string]: Constructor<Server>;
};
/**
* An array of bindings

This comment has been minimized.

@bajtos

bajtos Nov 8, 2018

Member

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 raymondfeng force-pushed the component-bindings branch from 76c5557 to d70b2a7 Nov 8, 2018

@raymondfeng

This comment has been minimized.

Member

raymondfeng commented Nov 8, 2018

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 raymondfeng force-pushed the component-bindings branch from 6eafae3 to 4fbbb39 Nov 8, 2018

@raymondfeng

This comment has been minimized.

Member

raymondfeng commented Nov 9, 2018

@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).

The example `MyComponent` above will add `MyController` to application's API and
create a new binding `my-value` that will be resolved using `MyValueProvider`.
create the following bindings to the application context:

This comment has been minimized.

@bajtos

bajtos Nov 9, 2018

Member

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

Suggested change Beta
create the following bindings to the application context:
create the following bindings in the application context:

ping @bschrammIBM @dhmlau

/**
* An array of bindings to be aded to the application context. For example,
* ```ts
* const bindingX = new Binding('x').to('Value X');

This comment has been minimized.

@bajtos

bajtos Nov 9, 2018

Member

Let's use the newly added sugar API please.

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

@raymondfeng raymondfeng force-pushed the component-bindings branch from 624df0c to f95737c Nov 9, 2018

@raymondfeng raymondfeng force-pushed the component-bindings branch from f95737c to cc96229 Nov 9, 2018

@bajtos

bajtos approved these changes Nov 9, 2018

@raymondfeng raymondfeng merged commit 824e9f4 into master Nov 9, 2018

3 of 4 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - package.json (dhmlau) No manifest changes detected

@raymondfeng raymondfeng deleted the component-bindings branch Nov 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment