[DO NOT REVIEW] feat: implement convention based boot

[virkt25]

Description

Initial implementation of boot package.

Related issues

• connect to [Boot] Automatic configuration of controllers in Application #780
• connect to [Boot] Automatic configuration of repositories in Application  #781

Checklist

• New tests added or existing tests modified to cover all changes
  • WIP - RepositoryMixin boot() override test
• Code conforms with the style guide

[kjdelisle]

I'd switch the trigger to test:rebuild or something similar.

[kjdelisle]

Solid initial implementation with respect to all of the artifact handling, only minor nits with respect to default values there.

However, I think we can do better with respect to the way we handle boot mixins. See https://www.typescriptlang.org/docs/handbook/mixins.html for an example of array-based mixin loading.

If we can make the UX nicer than having to nest a dozen mixin calls on an extended class, then I'd call that a big win. If there's a good reason not to, then I'll dismiss that portion of the review.

[kjdelisle]

Change the case of Booting to lowercase.

[kjdelisle]

I'm conflicted about this approach. On the one hand, it gives us finer control over the boot order (which is something people have complained about with LB3), but it requires users to be careful about how they use mixins, and requires an awful lot of mixin chaining.

It could end up looking like this:

export class MyApplication extends ThirdScript(SecondScript(FirstScript(BootMixin(RestApplication)))) {
  // etc...
}

It's not a tremendously painful experience, but I think it'd be nice if we added some sugar to simplify this. If you check out the TypeScript handbook page on Mixins, it shows a pattern for applying an array of mixins without requiring users to extend things in this way.

[raymondfeng]

Again, I don't think Mixin is the right approach here. It has the same issue as Express app.use as the order of execution is controlled by the order how mixins are chained. I would rather use the extension point/extension pattern as we do in LoopBack 3.x.

Please note that a single boot() method is not going to work. There are multiple steps needed to discover/load/transform/resolve/start/stop various artifacts which can be cross-referenced. We need to allow each extensions to participate the boot process at different phases.

I strongly recommend that we follow a similar approach at https://github.com/strongloop/loopback-boot/tree/master. The main idea is as follows:

1. The @loopback/boot will have an extension point for all booters as separate extensions. The extension point can find corresponding extensions from the context.

2. Each booter can be registered using context bindings, such as booters.controller, booters.model, and booters.datasource.

3. There will be a BootLifeCycle interface to define optional steps, such as:

export interface BootLifeCycle {
  discover?(...);
  load?(...);
  transform?(...);
  resolve?(...);
  start?(...);
  stop?(...);
}

A booter can choose to implement what methods to implement.

4. app.boot will give control to the boot extension point, which in turn coordinates the registered booters to boot artifacts in the order of life cycle events.

5. app.boot have options to control what booters should be involved and what life cycle phases should be triggered.

[virkt25]

Thanks for the comments Raymond. Looking for some clarification.

1. My understanding of loopback-boot right now is that it runs scripts found in a boot folder alphabetically / in a given order by requiring them. We can accomplish the same using the Mixin by running scripts in a boot folder. This task was just about discovering and binding artifacts.

2. I sort of see the value of having different functions for discover/load/transform/etc. but if the purpose of boot is to find and load artifacts than I can imagine a single function being able to do that as a extension developer can write the boot function to do what they want.

3. I'm not sure I understand the purpose of certain phases that are proposed such as resolve/start/stop. Resolve in particular is something that I think is better handled by Dependency Injection.

With @kjdelisle 's change, you can control Mixin order by passing them in via an array.

Just trying to understand the value add of having phases for boot vs. just booting by discovering artifacts via Mixin followed by running scripts in a boot folder like current loopback-boot.

[raymondfeng]

My understanding of loopback-boot right now is that it runs scripts found in a boot folder alphabetically / in a given order by requiring them. We can accomplish the same using the Mixin by running scripts in a boot folder. This task was just about discovering and binding artifacts.

No. Boot script is just one of the artifact types we support in LoopBack 3.x. There are different plugins for model definitions, data sources, middleware, components, and boot scripts. Each plugin is responsible for loading/compiling/executing its own kind of artifact.

I sort of see the value of having different functions for discover/load/transform/etc. but if the purpose of boot is to find and load artifacts than I can imagine a single function being able to do that as a extension developer can write the boot function to do what they want.

One pass is usually not good enough to handle artifacts that have references to other ones. For example, model Customer has a hasMany relation to model Order and Order has a belongsTo relation to Customer. A single boot() for models cannot build the runtime representation of the relations between the two models .

I'm not sure I understand the purpose of certain phases that are proposed such as resolve/start/stop. Resolve in particular is something that I think is better handled by Dependency Injection.

Resolve - connect multiple artifacts, for example, set up the target model reference for a relation.
Start - start an artifact, for example, a server instance or a database connection
Stop - stop an artifact, for example, a server instance or a database connection

[virkt25]

I see. But shouldn't references be resolved by Dependency Injection when needed if boot sets up bindings properly which is what it should be doing ...?

And I see the mixins folder on loopback-boot. So it controls loading of artifacts by calling the start() for each mixin?

I'm also not sure what it means to start / stop an artifact ... or how stop will even be called?

[virkt25]

Yea sorry I meant plugins not mixins.

[kjdelisle]

@raymondfeng Your proposed changes are out of scope for the task. If this is something we want, then we should create a new task, groom it and poker it.

[raymondfeng]

IMO, the approach taken by the PR is architecturally flawed. I'm not sure if we really want to land it as is. The boot module is fundamental to the declarative support for LoopBack 4. I think we need to start with the right path from the beginning.

I don't mind starting with a single boot() implemented by individual booters, but mixin is not suitable for this use case.

[bajtos]

Great discussion!

Let me clarify few possible confusions I anticipate from the discussion above before I post my opinions.

In LoopBack 3.x, there are two versions of loopback-boot. 2.x is what almost everybody uses today, because it's the latest version published to npmjs.org and used by our CLI tooling when scaffolding a new project. It has rather simple and less flexible design, the order of individual boot steps is hard coded and there are no extension points for components. Then there is the upcoming new version 3.0 living in the master branch containing a rewrite by @raymondfeng and @davidcheung, which provides much more flexible design with extension points for components. I believed this version was never published (see strongloop/loopback-boot#241), but as I checked the master branch today, there are apparently some 3.x versions published to npmjs.org. Because the release process of the new semver-major version was not done properly, these 3.x versions are effectively hidden by 2.x updates. What a mess! But let's not digress - I just wanted to clarify the state of loopback-boot to prevent confusion created by people (e.g. from our community) reading the discussion above.

My understanding of loopback-boot right now is that it runs scripts found in a boot folder alphabetically / in a given order by requiring them. We can accomplish the same using the Mixin by running scripts in a boot folder. This task was just about discovering and binding artifacts.

This is definitely the case in loopback-boot 2.x. I don't know how the new 3.x version handles boot scripts.

And I see the mixins folder on loopback-boot. So it controls loading of artifacts by calling the start() for each mixin?

Even though this has been clarified that @virkt25 meant plugins, not mixins`, I'd like to note that in LoopBack 3.x, we have a concept of Model Mixins that allow users to apply a mixin to a model class, and at least loopback-boot 2.x does load and execute such mixin scripts.

---

[@kjdelisle] @raymondfeng Your proposed changes are out of scope for the task. If this is something we want, then we should create a new task, groom it and poker it.
[@raymondfeng] IMO, the approach taken by the PR is architecturally flawed. I'm not sure if we really want to land it as is. The boot module is fundamental to the declarative support for LoopBack 4. I think we need to start with the right path from the beginning.
I don't mind starting with a single boot() implemented by individual booters, but mixin is not suitable for this use case.

I agree with both of you.

IMO, we should not aim for a fully flexible bootstrapper in this first iteration targeting our MVP milestone. For MVP, it's ok to have a hard-coded bootstrapper with no (or very little) extension points.

At the same time, I agree that mixin composition is not a good architecture for our bootstrapper and therefore we should not pursue it even for MVP, because upgrading from mixin composition to a different architecture would be too disruptive.

TBH, I find Raymond's BootLifeCycle with pre-defined steps as suboptimal too. Assuming each of the steps like discover and load are run fully for all components before the next step is executed (i.e. run discover for all components and only then run load for all components), then how are we going to handle the situation when ComponentA needs the output of load step from ComponentB before it can discover its own artifacts? On the other hand, if entire BootLifeCycle is run for each component before the next component is booted (i.e. run discover and load for ComponentA, then for ComponentB), then how are we going to handle the situation when ComponentA needs to run some task after ComponentB has loaded & transformed all its artifacts, but before ComponentB attempts to resolve them?

I am proposing to explore our "Sequence" design pattern we are using for composing request handling middleware and see if and how it can be applied to the boot process too. I see two benefits of such approach:

• By reusing the same concept for both request handling logic and bootstrapping, users can apply their existing knowledge in multiple places. The framework should be easier to learn as a result.

• Sequence makes the flow control explicit, easy to understand and modify.

Right now, the boot process (both master (3.x) and 2.x branches of loopback-boot for LoopBack 3.x) is very opaque. The current 2.x branch does not allow any customization of boot steps - see the hard-coded algorithm in executor.js. The new version in master (which hasn't been released for public use yet - see strongloop/loopback-boot#241) seems to promise more flexibility in that matter, but my knowledge of that new codebase is not sufficient to tell how well we are delivering on that promise.

Let me post a mock-up boot sequence to illustrate my idea:

type EnvironmentVariables = {[name: string]: string};

interface BootOptions {
  projectRoot: string;
  env?: EnvironmentVariables;
}

interface AppBootSequence<App> {
  boot(app: App, options: BootOptions);
}

class MyBootSequence implements AppBootSequence<MyApplication> {
   /* constructor receiving dependencies - I am leaving that out for brevity */

  public boot(app: MyApplication, options: BootOptions) {
    this.setupDataSources(app, options);
    this.setupRepositoriesAndModels(app, options);
    this.resolveModelRelations(app, options);
    this.setupControllers(app, options);
    this.start(app, env);
  }
}

// setupDataSources step provided by @loopback/repository or similar
setupDataSources(app, options) {
  const controllersDir = path.resolve(
    options.projectRoot,
    'controllers' || options.controllersDir);

  const controllerExts = options.controllerExts || 'controller.js';

  bootClassArtifacts(app, controllersDir, controllerExts, a => app.controller(a));
}

// usage in Application
// this code should be scaffolded by our CLI tooling
// together with MyBootSequence template
class MyApplication {
  constructor(/*...*/) {
    this.bind('bootstrapper').toClass(MyBootSequence)
  }

  async boot(env: EnvironmentVariables) {
    const options: BootOptions = {
      projectRoot: __dirname,
      env: env,
    }
    const sequence = await this.get('bootstrapper');
    await sequence.boot(app, options);
  }
}

Since this design is similar with REST Sequence Handler, I would expect it should be reasonably easy to leverage the extension point #657 and action composition #713 features that @raymondfeng is working on, to get an even more flexible solution.

Thoughts?

[bajtos]

Let me expand a bit on why I am against mixin composition.

Consider ComponentA and ComponentB, where ComponentA needs to run it's boot function only after ComponentB has already booted. This requires ComponentB to call super.boot() before running its own boot steps and the application developer to compose mixins in the correct order. As a result, neither the ComponentB nor the application developer are fully in control of the order in which individual boot steps are invoked and the correct order of boot steps requires cooperation of multiple parts of the codebase. I see that as a sign of Low Cohesion & High Coupling, which typically makes code more difficult to understand and maintain.

(I admit my solution proposed above does not address this problem entirely, because in the Sequence design, individual sequence steps have no control of the order in which they are executed. OTOH, the action composition technique proposed by Raymond should address that part in mostly backwards-compatible way.)

[kjdelisle]

Please constrain the superclass to the Application type, as a hint to extension devs:

export function BootMixin(
  superClass: typeof Application,
) { //... }

[kjdelisle]

function TestMixin(superClass: typeof Application)

[kjdelisle]

Please export this default value as a constant, too.

[bajtos]

I think the default value should be .controller.js (leading with a dot)?

[kjdelisle]

Please import these values from the constants you exported above (see previous comments).

[kjdelisle]

Please import these values from the constants you exported above (see previous comment).

[kjdelisle]

Please export this value as a constant for others.

[bajtos]

dist/src will not work for JavaScript projects that don't use any transpilation.

I am proposing to keep the approach for loopback-boot where we don't make assumptions about overall project layout and allow the user to have one or more applications in a single project.

As I understand (or envision?) our current conventions, the directories with application's artifacts are always in the same directory as the main application file:

# TypeScript
dist/src/application.js
dist/src/controllers/hello.controller.js

# Vanilla JavaScript
lib/application.js
lib/controllers/hello.controller.js

I am proposing to pass the __dirname of application.js as the project root to @loopback/boot. This way we don't need to know how anything about the transpilation setup, and also the usage of the boot module will become simpler:

// somewhere inside src/application.ts

const options: BootOptions = {
  projectRoot: __dirname,
  // ...
};

[kjdelisle]

Use constant exports.

[kjdelisle]

These should also be constant exports.

[shimks]

LGTM. I want to question the decision of putting the specific artifact booting function inside the main boot file (the controller one specifically). I personally think it'd be better to put them in their own files along with their own tests. But maybe there are downsides to my suggested approach that I'm not seeing, so take with a grain of salt.

[shimks]

2x automatically

[shimks]

needs an end tag for the code above

[shimks]

I think what the test is for should be more descriptive (testing for custom boot feat in this one, testing reversed order of mixins in the other, etc.), but maybe not since writing all of them would be too much; maybe just mention the important ones?

[shimks]

repeated definition in here and the integration one. They should probably be imported from a utils.test file or something similar. Same thing for other helper functions

[shimks]

Add some fringe case tests such as a.ts.js

[shimks]

Is this within scope?

[raymondfeng]

It should be Copyright (c) IBM Corp. 2017-2018. All Rights Reserved. now :-)

[raymondfeng]

Pls see my comment.

[bajtos]

I am worried about packaging bootstrapper with repository runtime. If we decide to make a breaking change in our boostrapper, we will have to release a new semver-major version of the repository package too (which may be more expensive/difficult to do). It will also force users of the repository to always use the latest boot version (and project conventions), therefore making the cost of upgrading to a newer semver-major version of the repository package more expensive.

Is there a way how to move bootstrapping of repositories to the boot package and export it as an optional step, where either the user can opt-in/opt-out, or else the bootstrapper detects whether the target app has repository method and use that to decide whether to boot repositories or not?

Ideally, this boot step would belong to its own package, e.g. @loopback/repository-boot, similarly to how #740 is introducing @loopback/repository-rest, but I am not sure if that's not an overkill, to have such a small package?

[bajtos]

I have reviewed the implementation, see my comments below. I'll take a look at the tests later (probably not sooner than tomorrow), sorry for the delays.

[bajtos]

I am against adding boot configuration to ApplicationConfig because of the reasons outlined in #742:

Right now, Application constructor accepts a config/options object that mixes two very different concerns:

1. The functionality/features the application should implement - what controllers it contains, what repositories are registered, etc. ("the assembly instructions")
2. The configuration - what port to listen on, what database servers to connect to, etc.

IMO, the consumers of an application must not change its functionality and features (with the exception of enabling/disabling explicit feature flags) and therefore the config/options argument should contain only the configuration (port numbers, connection strings, feature flags).

Just think about it - how is the code loading an application supposed to know what directories the application stores its controller source files?

IMO, the configuration of file paths should be provided by the application class. The boot function should be pretty much self-contained, with the exception of arguments holding environment-specific configuration, e.g. environment variables or a flag distinguishing between dev/debug-like and production-like modes.

[bajtos]

dist/src will not work for JavaScript projects that don't use any transpilation.

I am proposing to keep the approach for loopback-boot where we don't make assumptions about overall project layout and allow the user to have one or more applications in a single project.

As I understand (or envision?) our current conventions, the directories with application's artifacts are always in the same directory as the main application file:

# TypeScript
dist/src/application.js
dist/src/controllers/hello.controller.js

# Vanilla JavaScript
lib/application.js
lib/controllers/hello.controller.js

I am proposing to pass the __dirname of application.js as the project root to @loopback/boot. This way we don't need to know how anything about the transpilation setup, and also the usage of the boot module will become simpler:

// somewhere inside src/application.ts

const options: BootOptions = {
  projectRoot: __dirname,
  // ...
};

[bajtos]

Is it a good idea to boot controllers before booting things they depend on, e.g. repositories? I know this particular example is not valid, because controllers can be registered before their dependencies are, thanks to our DI/IoC design. But what if users decide to opt-out of our DI?

I am not saying the current implementation is wrong, I'd just like us to consider possible ramifications.

[bajtos]

I think it's a bad idea to couple boot with start. Consider the case of CLI tools like lb export-api-def or lb-ng, that need to load the full application including all repositories & controllers to inspect the shape of the public API, but that do not want to start the application.

As I see it, this issue is uncovering a deeper design flaw: it's possible to create an application instance that's not fully initialized (does not have all artifacts loaded and registered) and that requires further actions (method calls) before it can be used. This is an anti-pattern in OOP, unfortunately I am not able to find a good reference explaining the reasoning.

Unfortunately we cannot call async boot method directly from the constructor, which makes things more complicated. A possible solution is to use a flavor of Factory pattern:

class MyApplication {
  // The constructor is protected - only the class and subclasses
  // can create new instances
  protected constructor(/*...*/) {
    // ...
  }

  static async create(/*...*/): Promise<MyApplication> {
    const app = new MyApplication(/*...*/);
    await app.boot(/*...*/);
    return app;
  }

[bajtos]

I think the default value should be .controller.js (leading with a dot)?

[bajtos]

We need to take more care of error handling and carefully consider which errors are safe to hide to a debug log and which must be propagated back to the caller.

For example, when there is no controllers directory, then it makes sense to me to return an empty array and leave a message in the debug log.

However, if the controllers directory is there, but we were not able to read its contents, then that's a serious error that should abort the boot process.

[bajtos]

Will this correctly handle symlinks and possibly other edge cases? IMO we should be using glob module for all filesystem traversal, it's the de facto standard tool for this in Node.js.

[bajtos]

Use String.prototype.endsWith and Array.prototype.some:

return exts.some(e => item.endsWith(e));

[bajtos]

Let's not reinvent the wheel and use glob module please.

const pattern = 
  // start with one of the directories
  `@(${dirs.join('|'))/` +
  // then find any files in that directory
  nested ? '**/*' : '*' +
  // that have one of the allowed extensions
  `@(${exts.join('|'))`;

return glob.sync(pattern);

The pattern may not work out of the box.

If the pattern is too difficult to understand or to get right, then at least use glob instead of readFolder, and don't use Array.prototype.push - either replace dirs.forEach with dirs.map, or use files = files.concat, which allows you to get rid of flatten. I personally prefer dirs.map + flatten.

[bajtos]

Have you considered using a template argument here to avoid any and preserve type checks inside the function?

I think the following should work:

export function flatten<T>(arr: Array<Array<T> | T>): Array<T> {
  return arr.reduce(
    (flat, item) =>
      flat.concat(Array.isArray(item) ? flatten(item) : item),
    []);
}

[raymondfeng]

To summarize my view, there are a few key requirements:

1. We should be able to encapsulate booting logic for different artifacts in their classes, such as ControllerBooter, ModelBooter, DataSourceBooter, RepositoryBooter, and ComponentBooter.
2. We should be able to handle booting logic in steps to deal with dependencies, which can be even circular, for example, in relations, Customer has many Orders and Order belongs to a Customer.
3. We should be able to plug in new booters to support new artifact types, for example, a JSONSchemaBooter to load json schemas and build LB4 models out of them.
4. We should be able to coordinate the booters so that they can collaborate.
5. We should be able to filter what booters/actions to be triggered during boot so that we can customize the boot for different cases.

My proposal was to use a matrix (booter, phase) for the booting process to allow extensibility on both dimensions (number of booters and lifecycle phases). The extreme case of the matrix is to flatten it to an one-dimension array by having one phase but more booters.

As the starting point, the coordinator (extension point) can be hard-coded to wire and invoke all booters (extensions). Of course, it will not have the ability to support 3 and 5. We can then create a more flexible coordinator to dynamically visit booters in steps to form the booting process.

I see subtle but important difference between the sequence of actions for req/res processing and booting. IMO, the req/res processing cares more about round-trip and lean toward a chain of actions but booters are more one-way forward to enrich the metadata knowledge base and booting seems to be more fit with visitor pattern.

[virkt25]

I'll be creating a new version following the pattern and requirements proposed by @raymondfeng in a new branch. I'm leaving this PR open for reference till the new one is up. @bajtos please don't spend additional time reviewing this PR.

I'll also try to incorporate the general feedback I've had on the PR in terms of coding in the new PR.

[bajtos]

@virkt25 sounds good! Thank you for changing the pull request title, it helped :)

I feel that @raymondfeng's proposal is a grand vision of where we want to eventually arrive, but at the same time it requires a lot of work, more than we want to invest for the MVP. When working on the new version, please try to simplify and hard-code as many things as possible, to keep the initial pull request small and easy to review. Once the initial PR is landed, we can iteratively/incrementally improve it, as time and our priorities allow.

[virkt25]

Closing in favour of #858