feat(boot): implement Service booter #1652

bajtos · 2018-08-28T15:05:34Z

Implement automatic discovery and registration of Service providers - see #1439 for the full context and #1649 for the first step on that journey.

This pull request has three parts/commits, the first two are preparations making the actual implementation easier to make and troubleshoot.

feat(testlab): expose "sandbox.path" property (4e86b08)

Allow TestSandbox consumers to access the path of the sandbox directory.

Clean up the code by moving "validateInst" check into "path" getter,
thus avoiding the need to explicitly call "validateInst" in every
public API method.

feat(boot): add debug logs for better troubleshooting (f88e776)

Make it easier to troubleshoot the situation when a booter is not
recognizing artifact files and/or classes exported by those files.

To make debug logs easy to read, absolute paths are converted to
project-relative paths in debug logs. This change required a refactoring
of Booter design, where "projectRoot" becomes a required constructor
argument.

While making these changes, I changed "options" to be a required
constructor argument too, and chaged both "options" and "projectRoot"

feat(boot): implement Service booter

The main change.

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 · 2018-08-28T15:17:09Z

packages/boot/src/booters/service.booter.ts

+};
+
+function isServiceProvider(cls: Constructor<{}>): cls is ServiceProviderClass {
+  return /Provider$/.test(cls.name);


I think we should check the existence of value().

Sounds good, I add that check to the existing name check.

(IMO, checking just for value is not enough, as there may be other classes/interfaces that contain value method too.)

There should be documentation stating that the Booter is expecting Provider to be a part of the class name.

raymondfeng · 2018-08-28T15:17:58Z

packages/boot/test/fixtures/service-class.artifact.ts

+  greet(whom: string = 'world') {
+    return Promise.resolve(`Hello ${whom}`);
+  }
+}


Are we going to bind plain service class during boot? I don't see the logic.

Not yet. The logic here is to ensure that the booter is picking only Providers and excludes regular classes, there is an integration test for that.

https://github.com/strongloop/loopback-next/blob/9098ad3ade0b226e90291c0f3365b290bd508a66/packages/boot/test/integration/service.booter.integration.ts#L23-L26

Suggestions are welcome on how to make this more obvious to people reading this code.

Maybe it's a sign that I should enhance ServiceMixin and the booter to handle both app.serviceProvider(GeocoderServiceProvider) and app.service(GreeterService) cases, even though app.service may not be used by any real LB4 app? IDK.

For now, let's just add a comment to GreeterService saying it will be excluded from discovery as we only allow providers so far.

virkt25 · 2018-08-28T22:54:01Z

packages/boot/src/booters/base-artifact.booter.ts

    this.discovered = await discoverFiles(this.glob, this.projectRoot);
+
+    if (debug.enabled) {


Why have a check here? Doesn't debug only log if the debugger is enabled?

Because JSON.stringify is expensive. The parameters will be calculated before invoking debug itself.

virkt25 · 2018-08-28T22:55:02Z

packages/boot/src/booters/booter-utils.ts

 import {promisify} from 'util';
 const glob = promisify(require('glob'));

+const debug = debugFactory('loopback:boot:base-artifact-booter');


Should this be loopback:boot:booter-utils?

virkt25 · 2018-08-28T23:00:48Z

packages/boot/src/booters/service.booter.ts

+};
+
+function isServiceProvider(cls: Constructor<{}>): cls is ServiceProviderClass {
+  return /Provider$/.test(cls.name);


There should be documentation stating that the Booter is expecting Provider to be a part of the class name.

Allow TestSandbox consumers to access the path of the sandbox directory. Clean up the code by moving "validateInst" check into "path" getter, thus avoiding the need to explicitly call "validateInst" in every public API method.

Make it easier to troubleshoot the situation when a booter is not recognizing artifact files and/or classes exported by those files. To make debug logs easy to read, absolute paths are converted to project-relative paths in debug logs. This change required a refactoring of Booter design, where "projectRoot" becomes a required constructor argument. While making these changes, I changed "options" to be a required constructor argument too, and chaged both "options" and "projectRoot" to readonly properties.

- address review comments - use ServiceBooter in example apps - update tutorial pages - update docs

bajtos · 2018-08-30T08:42:01Z

I have addressed your review comments and finished updates of example apps and docs - see 36a8eac.

The pull request is ready for the final review and landing.

@raymondfeng @virkt25 PTAL again.

raymondfeng · 2018-08-30T14:10:24Z

docs/site/Booting-an-Application.md

+
+**IMPORTANT:** For a class to be recognized by `ServiceBooter` as a service
+provider, its name must end with `Provider` suffix and its prototype must have a
+`value()` method.


To me, this is a sign that we should have something like #992 to make it possible to explicit.

Makes sense 👍

+1 for #992, it's certainly needed.

raymondfeng · 2018-08-30T14:11:35Z

docs/site/soap-calculator-tutorial-scaffolding.md

@@ -12,12 +12,14 @@ Let's start by creating the initial application by running the following
 command:

 ```sh
-lb4 app soap-calculator --enableRepository
+lb4 app soap-calculator --enableRepository --enableServices


Hmm, now we have enableRepository vs enableServices. The naming is inconsistent.

Yeah. Should we rename --enableRepository to --enableRepositories? I can do that in a follow-up pull request.

bajtos self-assigned this Aug 28, 2018

bajtos requested review from raymondfeng, virkt25 and marioestradarosa August 28, 2018 15:05

bajtos mentioned this pull request Aug 28, 2018

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

Closed

8 tasks

raymondfeng reviewed Aug 28, 2018

View reviewed changes

virkt25 reviewed Aug 28, 2018

View reviewed changes

feat(testlab): expose "sandbox.path" property

d888308

Allow TestSandbox consumers to access the path of the sandbox directory. Clean up the code by moving "validateInst" check into "path" getter, thus avoiding the need to explicitly call "validateInst" in every public API method.

bajtos force-pushed the feat/service-booter branch from 9098ad3 to 10eb11b Compare August 30, 2018 08:38

bajtos added 3 commits August 30, 2018 10:39

feat(boot): implement Service booter

9b8c8b7

fixup! finish the changes

cc0e108

- address review comments - use ServiceBooter in example apps - update tutorial pages - update docs

bajtos force-pushed the feat/service-booter branch from 10eb11b to cc0e108 Compare August 30, 2018 08:40

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

raymondfeng reviewed Aug 30, 2018

View reviewed changes

raymondfeng approved these changes Aug 30, 2018

View reviewed changes

virkt25 approved these changes Aug 30, 2018

View reviewed changes

raymondfeng merged commit 9cab3a9 into master Aug 30, 2018

raymondfeng deleted the feat/service-booter branch August 30, 2018 17:27

bajtos mentioned this pull request Aug 31, 2018

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

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(boot): implement Service booter #1652

feat(boot): implement Service booter #1652

bajtos commented Aug 28, 2018 •

edited

Loading

raymondfeng Aug 28, 2018

bajtos Aug 28, 2018

virkt25 Aug 28, 2018

raymondfeng Aug 28, 2018

bajtos Aug 28, 2018

raymondfeng Aug 28, 2018

virkt25 Aug 28, 2018

raymondfeng Aug 29, 2018

virkt25 Aug 28, 2018

virkt25 Aug 28, 2018

bajtos commented Aug 30, 2018

raymondfeng Aug 30, 2018

bajtos Aug 30, 2018

virkt25 Aug 30, 2018

raymondfeng Aug 30, 2018

bajtos Aug 30, 2018

		this.discovered = await discoverFiles(this.glob, this.projectRoot);

		if (debug.enabled) {

feat(boot): implement Service booter #1652

feat(boot): implement Service booter #1652

Conversation

bajtos commented Aug 28, 2018 • edited Loading

Checklist

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

bajtos commented Aug 30, 2018

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

bajtos commented Aug 28, 2018 •

edited

Loading