feat(service-proxy): add service mixin #1649

bajtos · 2018-08-27T09:02:59Z

Implement "ServiceMixin" for applications. This mixin enhances component
registration so that service providers exported by a component are
automatically registered for dependency injection; and adds a new sugar
API for registering service providers manually:

app.serviceProvider(MyServiceProvicer);

The method name "serviceProvider" was chosen deliberately to make it
clear that we are binding a Provider, not a class constructor. Compare
this to app.repository(MyRepo) that accepts a class construct. In the
future, we may add app.service(MyService) method if there is enough
user demand.

This pull request is the first step towards automated service registration via boot, see #1439.

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 · 2018-08-27T09:06:36Z

docs/site/Calling-other-APIs-and-Web-Services.md

-
-**NOTE**: Once we start to support declarative datasources with
-`@loopback/boot`, the datasource configuration files can be dropped into
-`src/datasources` to be discovered and bound automatically.


This doc fix is not strictly related to the changes made by my pull request, the section should have been removed when the datasource booter was added. Since it's just a small change, I didn't feel like creating a new commit or a new pull request.

raymondfeng · 2018-08-27T14:44:03Z

packages/service-proxy/src/mixins/service.mixin.ts

+/**
+ * Interface for classes with `new` operator.
+ */
+export interface Class<T> {


Should we somehow leverage the Class/Constructor type from https://github.com/strongloop/loopback-next/blob/master/packages/context/src/value-promise.ts#L13 or refactor it into a common package?

Having a shared definition of Class and Constructor types would be definitely nice.

However, I am not sure if such small typedefs are enough to justify the cost of creating a new package?

Also please note that in RepositoryMixin (which I based my work on), Class is coming from a different package - see https://github.com/strongloop/loopback-next/blob/f68a45ced9a392286d484b72cc690a6599aeb0c0/packages/repository/src/common-types.ts#L11-L19

Can we leave this improvement out of scope of this pull request please?

Sure. +1 to have a separate PR.

raymondfeng · 2018-08-27T14:47:46Z

packages/service-proxy/src/mixins/service.mixin.ts

+     *
+     * @param component The component to mount services of
+     */
+    mountComponentRepository(component: Class<{}>) {


Shouldn't it be named as mountComponentServiceProviders?

BTW, the mountComponentRepository in RepositoryMixin should be named as mountComponentRepositories too. (plural)

Good point, I'll apply both suggestions.

raymondfeng · 2018-08-27T14:49:34Z

packages/service-proxy/src/mixins/service.mixin.ts

+  // tslint:disable-next-line:no-any
+  serviceProvider<S>(provider: Class<Provider<S>>): void;
+  component(component: Class<{}>): void;
+  mountComponentRepository(component: Class<{}>): void;


It should be mountComponentServiceProviders too.

Actually, I prefer to call this method mountComponentServices. In the future, if we add support for app.service(serviceCtor: Class), then I'd like mountComponentServices to handle both service classes and providers.

raymondfeng · 2018-08-27T14:49:50Z

packages/service-proxy/src/mixins/service.mixin.ts

+/**
+ * Interface for an Application mixed in with ServiceMixin
+ */
+export interface AppWithServices extends Application {


ApplicationWithServices?

raymondfeng · 2018-08-27T14:50:21Z

packages/service-proxy/src/mixins/service.mixin.ts

+   *
+   * @param component The component to mount services of
+   */
+  mountComponentRepository(component: Class<{}>) {}


mountComponentServiceProviders

raymondfeng · 2018-08-27T14:51:46Z

packages/service-proxy/test/unit/mixin/service.mixin.unit.ts

+
+// tslint:disable:no-any
+
+describe('RepositoryMixin', () => {


It should be ServiceProviderMixin.

See #1649 (comment), I prefer to use ServiceMixin instead of ServiceProviderMixin.

In my mind, the mixin is adding APIs for working with services. The fact that services are registered via Providers is just an implementation detail to me.

raymondfeng

Nice abstraction. Please address my comments.

bajtos · 2018-08-27T15:29:50Z

@raymondfeng comments addressed in 767c54c, LGTY now?

marioestradarosa · 2018-08-27T17:33:49Z

Nice work @bajtos . From the app developer perspective in the current way of registering the services and the proposed way of registering the provider and then injecting the service in the controller I can see a better code design and the addition of the ServiceMixin automatically by the boolean switch is great!.

Can you highlight if there is a gain in performance in this way?. And finally I believe the next step would be to have the provider discovery , to avoid the registration of the provider, imagine an app with a lot of providers ?

marioestradarosa

Excelent! .

virkt25

Awesome effort 👏 Just some minor comments.

virkt25 · 2018-08-28T02:13:40Z

packages/cli/generators/app/templates/src/application.ts.ejs

+  if (project.enableRepository) {
+    classWithOptionalMixins = `RepositoryMixin(${classWithOptionalMixins})`;
+  }
+  if (project.enableServices) {


Can we look to compute classWithOptionalMixins within the generator instead of the template to keep the template simple?

virkt25 · 2018-08-28T02:15:06Z

packages/cli/generators/project/templates/package.json.ejs

    "@loopback/repository": "<%= project.dependencies['@loopback/repository'] -%>",
-    "@loopback/rest": "<%= project.dependencies['@loopback/rest'] -%>"
+    "@loopback/rest": "<%= project.dependencies['@loopback/rest'] -%>",
+    "@loopback/service-proxy": "<%= project.dependencies['@loopback/service-proxy'] -%>"


Should this be limited to an application with serviceProxy option enabled? Then again we are loading the repository package for an application regardless ... I'll leave it up to you to decide.

virkt25 · 2018-08-28T02:24:49Z

packages/service-proxy/src/mixins/service.mixin.ts

+     * app.serviceProvider(GeocoderServiceProvider);
+     * ```
+     */
+    serviceProvider<S>(provider: Class<Provider<S>>): void {


Shouldn't we just call this service()?

Cross-posting from the pull request description:

The method name "serviceProvider" was chosen deliberately to make it
clear that we are binding a Provider, not a class constructor. Compare
this to app.repository(MyRepo) that accepts a class construct. In the
future, we may add app.service(MyService) method if there is enough
user demand.

bajtos · 2018-08-28T11:10:51Z

@marioestradarosa thank you for chiming in.

Can you highlight if there is a gain in performance in this way?

I am not aware of any performance gains. My goal here is to make it easier for LB4 app developers to register their services (service providers).

And finally I believe the next step would be to have the provider discovery , to avoid the registration of the provider, imagine an app with a lot of providers ?

Exactly! My next step is to implement a Booter in @loopback/boot package that will scan project files, discover service providers and register them via app.serviceProvider.

See #1439 for a bigger picture.

bajtos · 2018-08-28T12:02:45Z

@virkt25 thank you for pointing out opportunities to make the code cleaner! I addressed your first two comments in 6b7fa83

Implement "ServiceMixin" for applications. This mixin enhances component registration so that service providers exported by a component are automatically registered for dependency injection; and adds a new sugar API for registering service providers manually: app.serviceProvider(MyServiceProvicer); The method name "serviceProvider" was chosen deliberately to make it clear that we are binding a Provider, not a class constructor. Compare this to `app.repository(MyRepo)` that accepts a class construct. In the future, we may add `app.service(MyService)` method if there is enough user demand.

bajtos requested a review from raymondfeng as a code owner August 27, 2018 09:02

bajtos self-assigned this Aug 27, 2018

bajtos requested review from marioestradarosa, shimks and virkt25 August 27, 2018 09:03

bajtos added the feature label Aug 27, 2018

bajtos added this to the August Milestone milestone Aug 27, 2018

bajtos force-pushed the feat/register-services branch from e96a5d1 to fd1e05a Compare August 27, 2018 09:04

bajtos commented Aug 27, 2018

View reviewed changes

bajtos mentioned this pull request Aug 27, 2018

Sugar API for registering services + automated registration via boot #1439

Closed

8 tasks

raymondfeng reviewed Aug 27, 2018

View reviewed changes

raymondfeng requested changes Aug 27, 2018

View reviewed changes

raymondfeng approved these changes Aug 27, 2018

View reviewed changes

marioestradarosa approved these changes Aug 27, 2018

View reviewed changes

virkt25 reviewed Aug 28, 2018

View reviewed changes

bajtos force-pushed the feat/register-services branch from 767c54c to 6b7fa83 Compare August 28, 2018 12:02

bajtos force-pushed the feat/register-services branch from 6b7fa83 to 42ca652 Compare August 28, 2018 12:03

bajtos merged commit fb01931 into master Aug 28, 2018

bajtos deleted the feat/register-services branch August 28, 2018 12:54

bajtos mentioned this pull request Aug 28, 2018

feat(boot): implement Service booter #1652

Merged

7 tasks

bajtos mentioned this pull request Aug 31, 2018

fix(cli): rename repository/service feature flags #1661

Merged

7 tasks

bajtos mentioned this pull request Sep 13, 2018

docs: todo-tutorial extend app with ServiceMixin #1703

Merged

7 tasks

simonbrunel mentioned this pull request Apr 20, 2021

fix(cli): invalid package.plain.json.ejs template #7416

Merged

8 tasks


		// tslint:disable:no-any

		describe('RepositoryMixin', () => {

feat(service-proxy): add service mixin #1649

feat(service-proxy): add service mixin #1649

Uh oh!

Conversation

bajtos commented Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raymondfeng Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raymondfeng Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raymondfeng left a comment

Choose a reason for hiding this comment

Uh oh!

bajtos commented Aug 27, 2018

Uh oh!

marioestradarosa commented Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marioestradarosa left a comment

Choose a reason for hiding this comment

Uh oh!

virkt25 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Aug 28, 2018

Uh oh!

bajtos commented Aug 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bajtos commented Aug 27, 2018 •

edited

Loading

raymondfeng Aug 27, 2018 •

edited

Loading

raymondfeng Aug 27, 2018 •

edited

Loading

marioestradarosa commented Aug 27, 2018 •

edited

Loading