-
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(core): introduce basic life cycle support for LoopBack applications #1928
Conversation
9baeb74
to
e277a32
Compare
e277a32
to
bd846c8
Compare
when will this PR be merged? :) |
@bajtos Please review. |
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.
Let's discuss few high-level areas first.
packages/core/src/application.ts
Outdated
@@ -130,6 +131,9 @@ export class Application extends Context { | |||
* @memberof Application | |||
*/ | |||
public async start(): Promise<void> { | |||
await this._forEachComponent(c => { | |||
if (isLifeCycleAction(c)) return c.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.
In your proposed design, a component class has to implement LifeCycleActions
interface if it wants to be notified about start/stop events.
Have you considered a different approach where the component "exports" possibly multiple LifeCycleAction classes using the same pattern we have for controllers and providers?
This is the good old Composition over inheritance advice.
// in a component
class MyComponent implements Component {
onLifeCycle: [MyLifeCycleListener];
}
class MyLifeCycleListener {
constructor(/* receive dependencies */) {}
async start() {
// ...;
}
async stop() {
// ...;
}
}
// inside `mountComponent`
component.onLifeCycle.forEach(cls => app.registerLifeCycleListener(cls));
// inside app.start()
await Promise.all(this._lifeCycleListeners.map(l => l.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.
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 that even with the observer pattern from #1972, we should still allow components to export lifecycle observers in an indirect way, i.e. letting mountComponent
to handle the actual registration.
It is not clear to me how #1972 would affect the design of lifecycle events and whether the solution based purely on low-level observer APIs would be enough easy to use.
My preferred approach is to use composition instead of inheritance, allow components to export an array of life-cycle listener/observe classes and get this pull request landed.
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.
Later on, when #1972 is implemented, we can look into ways how to leverage the observer pattern as a low-level implementation detail to power "LifeCycleObserver".
aaa24f1
to
6fc3f86
Compare
6fc3f86
to
2f613ec
Compare
d083838
to
5ede815
Compare
@bajtos PTAL |
5afd0a7
to
469f594
Compare
64b56cc
to
7048e1a
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.
LGTM 💯 , left some minor comments.
packages/core/src/application.ts
Outdated
* @param keyNamespace Binding key prefix | ||
* @param tag Tag name | ||
*/ | ||
private _findByNamespaceOrTag(keyNamespace: string, tag?: string) { |
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 wonder if this utility function can be moved to @loopback/context
and be useful in other places?
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.
Great idea, +1 from me
41522f3
to
5d442d2
Compare
2a91482
to
c64551a
Compare
Well in that case, what are the arguments against setting the tag & namespace via the What I am looking for: an easy way how to bind a life-cycle observer without using |
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 was a discussion where it was mentioned that eventually we'll be using the observer pattern for application events.
Will the API of lifeCycleObserver
be affected by it? If yes, we should mention it beforehand so as to prepare our users of the future change. If only the underlying implementation is affected with no change in the API, we need not.
The observer pattern can be used to implement life cycle observers but the proposed apis can stay. If we have to change, we can always release a new major version. Release often, release fast :-) |
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.
LGTM. Please clean up the git history before landing.
Implements:
The PR implements the following features:
lb4 observer
command to generate life cycle scripts - See https://github.com/strongloop/loopback-next/blob/lifecycle/docs/site/Life-cycle-observer-generator.mdTest
observer
cliChecklist
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated