-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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: @loopback/boot #858
feat: @loopback/boot #858
Conversation
2e05cce
to
e586b3d
Compare
packages/core/src/application.ts
Outdated
this.options.boot.projectRoot = resolve(this.options.boot.projectRoot); | ||
|
||
// Bind Boot Config for Booters | ||
this.bind(CoreBindings.BOOT_CONFIG).to(this.options.boot); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be better to create a child context of app
and use it to run boot()
.
packages/core/src/application.ts
Outdated
* A Booter class interface | ||
*/ | ||
export interface Booter { | ||
config?(): void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use verbs for the phases/methods? For example, configure
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for using verbs for phases/method names, that's the usual convention AFAIK.
// This file is licensed under the MIT License. | ||
// License text available at https://opensource.org/licenses/MIT | ||
|
||
import {CoreBindings, Application, Booter, BootOptions} from '@loopback/core'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Booter
and BootOptions
interfaces should be defined in boot
package.
* a list of skipped files. Other files that were read will still be bound | ||
* to the Application instance. | ||
*/ | ||
async boot() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to find a better name for this phase, such as load
.
packages/core/src/application.ts
Outdated
* @param {BootOptions} bootOptions Options for boot. Bound for Booters to | ||
* receive via Dependency Injection. | ||
*/ | ||
async boot(bootOptions?: BootOptions) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Majority of this logic should be refactored into a BootStrapper
class in boot
package as the extension point for all booters. app.boot
is just a sugar method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also as I mentioned earlier, the boot configuration is an internal business of the Application
class that should not be leaking to Application's public API. The boot
method should not be accepting any assembly-related config.
packages/core/src/application.ts
Outdated
for (const inst of booterInsts) { | ||
if (inst[phase]) { | ||
await inst[phase](); | ||
console.log(`${inst.constructor.name} phase: ${phase} complete.`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use debug
module instead of console.log
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this new version, I find it a lot better than the previous proposal 👍
There are some problems from our previous discussions that are still not addresses, please see my comments below.
packages/boot/.gitignore
Outdated
@@ -0,0 +1,3 @@ | |||
*.tgz | |||
dist* | |||
package |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use top-level .gitgnore
from loopback-next monorepo and remove this file?
packages/boot/.travis.yml
Outdated
@@ -0,0 +1,5 @@ | |||
sudo: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, can we use top-level .travis.yml
from loopback-next monorepo and remove this file?
packages/boot/README.md
Outdated
|
||
```ts | ||
import {ControllerBooter} from '@loopback/boot'; | ||
app.booter(ControllerBooter); // register booter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. How are we going to register all booters, will we need another metabooter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow. A booter is registered by app.booter
. A normal Application developer will likely not be writing their own Booters. We can/should add support for a ScriptBooter
however at some point down the road to boot all scripts in a scripts folder similar to how it is done today.
packages/boot/README.md
Outdated
**Via Application Config** | ||
```ts | ||
new Application({ | ||
boot: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👎
As I commented before, we should not be mixing assembly configuration with application configuration, see #742. In particular, users of Application
class should not be able to modify how the Application is assembled (booted). It's the responsibility of the Application class to know where to find its assembly bits like controllers.
packages/boot/README.md
Outdated
**Via BootOptions** | ||
```ts | ||
app.boot({ | ||
boot: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API feels clunky to me, why are we nesting configuration of app.boot
inside boot
property? We already know that the argument of app.boot
is specifying boot-related config.
The previous comment applies here too - users of Application class, including the callers of app.boot
should not have access to boot configuration.
projectRoot = | ||
process.cwd().indexOf('packages') > -1 | ||
? `${dist}/test/fixtures/booterApp` | ||
: `packages/boot/${dist}/test/fixtures/booterApp`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I consider it as a code smell that this code needs to know about what is our compilation target and what is the layout of dist
directories used by the compiler.
I am proposing to use a simpler solution based on __dirname
or require.resolve
, for example:
projectRoot = path.resolve(__dirname, '..', 'fixtures', 'booterApp');
// or
projectRoot = path.dirname(require.resolve('../fixtures/booterApp/application.ts'));
(This requires that fixtures are copied or transpiled as part of our build. The first alternative can be achieved by adding --allowJs
(possibly together with --checkJs
) to tsc
compiler options. The second alternative requires us to change the dist files from .js
to .ts
. I personally prefer the second option, as it's closer to how our users are going to use the bootstrapper. IIUC, this part is already solved.)
packages/core/src/application.ts
Outdated
* app.booters([ControllerBooter, RepositoryBooter]); | ||
* ``` | ||
*/ | ||
booters<T extends Booter>(booterArr: Constructor<T>[]): Binding[] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I strongly dislike when we have two entities with almost the same name, differing only in the s
suffix. Such difference is very easy to overlook when reading code. Please use a more distinctive names, or perhaps allow app.booter
to accept both a single booter or an array of booters.
packages/core/src/application.ts
Outdated
* @param {BootOptions} bootOptions Options for boot. Bound for Booters to | ||
* receive via Dependency Injection. | ||
*/ | ||
async boot(bootOptions?: BootOptions) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also as I mentioned earlier, the boot configuration is an internal business of the Application
class that should not be leaking to Application's public API. The boot
method should not be accepting any assembly-related config.
packages/core/src/application.ts
Outdated
* A Booter class interface | ||
*/ | ||
export interface Booter { | ||
config?(): void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for using verbs for phases/method names, that's the usual convention AFAIK.
`${resolve(projectRoot, 'controllers/hello.controller.js')}`, | ||
`${resolve(projectRoot, 'controllers/two.controller.js')}`, | ||
`${resolve(projectRoot, 'controllers/another.ext.ctrl.js')}`, | ||
`${resolve(projectRoot, 'controllers/nested/nested.controller.js')}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned before, having an external fixture with multiple artifacts is an anti-pattern that yields test suite that's difficult to maintain over time.
For example, in this particular test, the expected list is unnecessary repetitive. There is no need to specify all of empty
, hello
and two
with the same extension .controller.js
, one entry is enough to validate the logic.
A better solution is to create a set of helpers that make it super easy to prepare a test app directory with exactly the right (and minimal!) set of artifacts needed by the test. This makes it also much much easier to understand which parts of the fixture are relevant to the particular (usually failing) test.
packages/boot/README.md
Outdated
|
||
A Booter is a Class that can be bound to an Application and is called | ||
to perform a task before the Application is started. A Booter may have multiple | ||
phases to complete it's task. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: it's
-> its
packages/boot/package.json
Outdated
"version": "4.0.0-alpha.1", | ||
"description": "A collection of Booters for LoopBack 4 Applications", | ||
"engines": { | ||
"node": ">=6" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Echoing Miroslav's comments from Kyu's PR. Best to drop support of node v6 in this new package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding on @bajtos comment on Kyu's PR was that we support Node 6 in new packages to avoid CI failure till such time that we can create a new PR to drop Node 6 across all packages. (I'm going to do that soon ... because it's an annoyance to support it when we are looking to drop it).
|
||
/** | ||
* This phase is responsible for configuring this Booter. It converts | ||
* options and assigns default values for missing values so ther phases |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: ther
-> that other
packages/core/src/application.ts
Outdated
|
||
// Find Bindings and get instance | ||
const bindings = this.findByTag(CoreBindings.BOOTERS_TAG); | ||
let booterInsts = bindings.map(binding => this.getSync(binding.key)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would let booterInsts = await bindings.map(binding => this.get(binding.key));
be better here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did that but unfortunately that doesn't work. I was ending up with an array of Promises
instead of resolved instances. I also tried bindings.map(async binding => await this.get(binding.key))
but was again getting Promises not instances (I think it has to do with the fact that this is an arrow function).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor nitpicks. Once major concerns from @bajtos and @raymondfeng are addressed, I'll review again hopefully more in detail
packages/boot/README.md
Outdated
The options for this are passed in a `controllers` object on `boot`. | ||
|
||
Available Options on the `boot.controllers` are as follows: | ||
|Options|Type|Default|Description| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Broken table here: probably needs a space in between the captions and the |
s
packages/boot/index.js
Outdated
|
||
const nodeMajorVersion = +process.versions.node.split('.')[0]; | ||
module.exports = | ||
nodeMajorVersion >= 7 ? require('./dist/src') : require('./dist6/src'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
module.exports = require('./dist/src')
since we're getting rid of node 6 support
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that we will support Node 6 till we can create a PR to drop it all together from all packages because otherwise we may start seeing CI failures.
Follow up based on a review comment here: #858 (comment) 36 This removes package level .gitignore and .travis.yml files in favour of top-level files.
Follow up based on a review comment here: #858 (comment) This removes package level .gitignore and .travis.yml files in favour of top-level files.
Follow up based on a review comment here: #858 (comment) This removes package level .gitignore and .travis.yml files in favour of top-level files.
Hi @bajtos
|
I'm not sure why AppVeyor builds are failing. The expected and actual error messages are identical. 😞 |
Fair enough 👍 I think I would slightly prefer if you could create and land that pull request adding "sandbox" functions before finishing this pull request. Your proposed approach is fine with me as long as the follow-up work is done ASAP - I don't want it to be delayed ad infinitum, as it often happens with clean-ups. |
This makes sense to a certain degree. Unfortunately it also means that we cannot version the bootstrapper and the core runtime independently any more :( For example, if we decide to rename one of the boot phases (booter methods) in the future, we must release a major version of both We have been already bitten by a built-in I would rather avoid repeating the same mistake in loopback-next.
I think the simplest solution for now is to remove Example usage (EDITED on 2018-01-19 17:31 CEST): import {Application} from '@loopback/core';
import {boot} from '@loopback/boot';
new Application({components: [BootComponent]});
boot(app, {
projectRoot: '',
controllers: {...}
}); This will allow us to remove To be honest, I think we should revisit the current design and consider moving the registration of individual booters to a different class than Example usage: import {Application} from '@loopback/core';
import {ControllerBooter, BootRunnert} from '@loopback/boot';
const app = new Application();
const booter = new BootComponent(app);
booter.register(ControllerBooter);
booter.run({
projectRoot: __dirname,
controllers: {
dirs: ['controllers'],
extensions: ['.controller.js'],
nested: true
} @raymondfeng @kjdelisle could you please chime in? |
packages/boot/README.md
Outdated
```ts | ||
import {Application} from '@loopback/core'; | ||
import {ControllerBooter, BootComponent} from '@loopback/boot'; | ||
const app = new Application({components:[BootComponent]}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
packages/boot/README.md
Outdated
#### Examples | ||
**Via BootOptions** | ||
```ts | ||
new Application({components: [BootComponent]}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, please use app.component
instead.
@virkt25 Since you are adding a new package, please make sure that all steps described in add a new package and new package checklist are implemented as part of this pull request. See #1013 and #1015 for more details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I like this new design a lot!
I am no longer able to review all changes in full details as I was re-reading this patch too many times. Please get somebody with fresh eyes to give this pull request a closer look.
packages/boot/src/boot.mixin.ts
Outdated
// Project Root and Boot Options need to be bound so we can resolve an | ||
// instance of Bootstrapper. | ||
this.bind(BootBindings.PROJECT_ROOT).to(this.projectRoot); | ||
this.bind(BootBindings.BOOT_OPTIONS).to(this.bootOptions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we create these bindings inside the constructor?
Have you considered using a dynamic binding, to ensure the bindings and the properties are always in sync?
this.bind(...).toDynamicValue(() => this.projectRoot);
this.bind(...).toDynamicValue(() => this.bootOptions);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea.
packages/boot/src/boot.mixin.ts
Outdated
* app.booter(MyBooter, MyOtherBooter) | ||
* ``` | ||
*/ | ||
booter(...booterCls: Constructor<Booter>[]): Binding[] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this method is accepting multiple arguments, I think it should have a plural name booters
.
packages/boot/src/boot.mixin.ts
Outdated
*/ | ||
booter(...booterCls: Constructor<Booter>[]): Binding[] { | ||
// tslint:disable-next-line:no-any | ||
return booterCls.map(cls => _bindBooter(<Context>(<any>this), cls)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
map
is used to transform an array of X values to an array of Y values. Please use forEach
instead.
I find this typecast very worrying: <Context>(<any>this)
. Isn't there any way how to tell TypeScript that BootMixin
can be applied only on classes inheriting from Context
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
map
is used because we're transforming an array of Constructor<Booter>
classes to an array of Binding
. Using forEach
will mean manually adding bindings to an array to return.
I had tried constraining the Mixin type to Application
but it caused issues with TypeScript ... issues relating to requiring me to import every definition used by Application into the file where we use the Mixin ... OR ... issues with being able to chain mixins. I'll add comments documenting this as per @raymondfeng's suggestion for future reference and maybe at some point there will be a way to fix this.
*/ | ||
mountComponentBooters(component: Constructor<{}>) { | ||
const componentKey = `components.${component.name}`; | ||
const compInstance = this.getSync(componentKey); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pattern when we have mountComponent{something}
calling this.getSync
to obtain the component instance is starting to repeat in too many places. IMO, we should refactor app.component
and introduce app.mountComponent
that can be used to access things provided by component instances.
// in application
public component(componentCtor: Constructor<Component>, name?: string) {
name = name || componentCtor.name;
const componentKey = `components.${name}`;
this.bind(componentKey)
.toClass(componentCtor)
.inScope(BindingScope.SINGLETON)
.tag('component');
// Assuming components can be synchronously instantiated
const instance = this.getSync(componentKey);
this._mountComponent(instance);
}
protected _mountComponent() {
mountComponent(this, instance);
}
// in extensions/component mixins:
// - do not change public component() method
// - change mountComponent instead
protected _mountComponent(component: Component & {booters: /*add typedef*/}) {
super._mountComponent(component);
if (component.booters) {
this.booter(...compInstance.booters);
}
}
Thoughts? Feel free to leave this improvement out of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a great idea! I've created #1017 to track this and keep it out of this PR. Feel free to edit that issue if I missed anything.
/** | ||
* Filter Object for Bootstrapper | ||
*/ | ||
filter?: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think the filter
should left out of this initial implementation and we should spend more time researching what typical use cases for filter
will be and how to support them in the easiest way.
It's not a blocker, this PR can be landed with the filter
option in place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll mark this as a feature that may change. I'd like to leave it in for future discussion. It can't be used via the Mixin approach anyways for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My worry is that we'll never have that discussion and there'll be a vestigial organ hanging off this code.
Anything that isn't something we're dead set on providing shouldn't be in the code base.
👎
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'll be more effort to remove this so I'm leaving it in and I'll create a follow up task for the discussion around this topic. In the meantime this is something we will definitely need to provide one way or another for APIC / etc. later on post-MVP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue to discuss this further: #1021
export class ControllerBooterApp extends BootMixin(RestApplication) { | ||
constructor(options?: ApplicationConfig) { | ||
super(options); | ||
this.projectRoot = __dirname; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 👍
} | ||
|
||
async start() { | ||
await super.start(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method can be removed, since it's effectively a no-op now. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
|
||
it('throws an error given a non-existent file', async () => { | ||
const files = [resolve(SANDBOX_PATH, 'fake.artifact.js')]; | ||
expect(loadClassesFromFiles(files)).to.eventually.throw(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please narrow down the check to verify that an expected error was thrown. We don't want this test to pass when e.g. TypeError: undefined is not a function
was reported by the loader.
// Customize @loopback/boot Booter Conventions here | ||
this.bootOptions = { | ||
controllers: { | ||
// Customize ControllerBooter Conventiones here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please spell out all default configuration options here? It will make it easier for users to immediately see which options are available for tweaking.
packages/boot/package.json
Outdated
"api-docs", | ||
"src" | ||
], | ||
"repository": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the following so that lerna
publishes it as a public package:
"publishConfig": {
"access": "public"
},
packages/boot/src/boot.mixin.ts
Outdated
* - Adds `mountComponentBooters` which binds Booters to the application from `component.booters[]` | ||
*/ | ||
// tslint:disable-next-line:no-any | ||
export function BootMixin<T extends Constructor<any>>(superClass: T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be tricky things to deal with if the base is Constructor<Application>
. We should at lease document why here.
a65a192
to
8734d45
Compare
A few commit messages violate our conventions:
|
8576b37
to
6a8ee32
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not happy with how much scope creep has occurred here, and the idea of committing some features based on potential future usage bothers me.
That being said, if you're certain we're going to need it and its existence requires upfront testing then I'll live with it.
/** | ||
* Filter Object for Bootstrapper | ||
*/ | ||
filter?: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My worry is that we'll never have that discussion and there'll be a vestigial organ hanging off this code.
Anything that isn't something we're dead set on providing shouldn't be in the code base.
👎
99a88e5
to
1a01290
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏
1a01290
to
8ef54a0
Compare
Squash this to the minimum number of distinct, self-contained commits and then merge it when ready. |
8ef54a0
to
e55b757
Compare
🎉 🎉 🎉 🎉 🎉 🎉 |
Initial implementation of a Phase based Booter in Application as well as a
@loopback/boot
package to containbooters
.implements #780
Checklist
npm test
passes on your machinepackages/cli
were updatedpackages/example-*
were updated