Skip to content
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

Merged
merged 4 commits into from Apr 9, 2019

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Oct 26, 2018

Implements:

The PR implements the following features:

Test observer cli

cd loopback-next
git checkout lifecycle
npm run build
cd sandbox
../packages/cli/bin/cli-main.js app life
cd life
../../packages/cli/bin/cli-main.js observer
cd ../..
npm i
cd sandbox/life
npm start

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

@davidxiao
Copy link

when will this PR be merged? :)

@raymondfeng
Copy link
Contributor Author

@bajtos Please review.

Copy link
Member

@bajtos bajtos left a 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 Show resolved Hide resolved
packages/core/src/lifecycle.ts Outdated Show resolved Hide resolved
@@ -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();
Copy link
Member

@bajtos bajtos Nov 5, 2018

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()));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ultimately we'll switch the observer pattern so that components can subscribe to the start/stop events. See #1972.

Do we want to wait unless we settle on #1972?

Copy link
Member

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.

Copy link
Member

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".

packages/core/src/lifecycle.ts Outdated Show resolved Hide resolved
@raymondfeng raymondfeng force-pushed the lifecycle branch 2 times, most recently from d083838 to 5ede815 Compare November 27, 2018 18:51
@raymondfeng
Copy link
Contributor Author

@bajtos PTAL

@raymondfeng raymondfeng force-pushed the lifecycle branch 2 times, most recently from 5afd0a7 to 469f594 Compare November 27, 2018 20:15
@raymondfeng raymondfeng mentioned this pull request Nov 27, 2018
@raymondfeng raymondfeng force-pushed the lifecycle branch 2 times, most recently from 64b56cc to 7048e1a Compare November 27, 2018 20:46
@raymondfeng raymondfeng changed the title feat(core): introduce basic life cycle support for components feat(core): introduce basic life cycle support for LoopBack applications Nov 27, 2018
packages/core/src/keys.ts Outdated Show resolved Hide resolved
packages/core/src/keys.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@b-admike b-admike left a 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 Show resolved Hide resolved
* @param keyNamespace Binding key prefix
* @param tag Tag name
*/
private _findByNamespaceOrTag(keyNamespace: string, tag?: string) {
Copy link
Contributor

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?

Copy link
Member

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

packages/boot/src/booters/lifecyle-script.booter.ts Outdated Show resolved Hide resolved
@raymondfeng raymondfeng requested a review from bajtos April 4, 2019 16:15
@raymondfeng raymondfeng force-pushed the lifecycle branch 2 times, most recently from 2a91482 to c64551a Compare April 5, 2019 15:57
@bajtos
Copy link
Member

bajtos commented Apr 8, 2019

Does it mean that if the lifecycle class is decorated with @Bind and sets the type to something else than CoreTags.LIFE_CYCLE_OBSERVER, then the result of calling app.lifeCycleObserver is a binding with a type that's not LIFE_CYCLE_OBSERVER? Similarly for namespace. Are we okay with that? I find it surprising to be honest.

Please note that type and namespace options in createBindingFromClass() overrides @bind.

Well in that case, what are the arguments against setting the tag & namespace via the asLifeCycleObserver template?

What I am looking for: an easy way how to bind a life-cycle observer without using createBindingFromClass or app.lifeCycleObserver.

Copy link
Contributor

@hacksparrow hacksparrow left a 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.

@raymondfeng
Copy link
Contributor Author

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 :-)

Copy link
Member

@bajtos bajtos left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants