feat: impl boot methods #171

killagu · 2018-07-03T12:15:25Z

Add Lifecycle class, control EggCore lifecycle.
After config, extend, app.js, angent.js loaded, loader should call initBoots.
After all files loaded, loader should call triggerDidLoad.

Refs: eggjs/egg#2520

Checklist

npm test passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

Description of change

killagu · 2018-07-03T12:18:04Z

codecov · 2018-07-03T12:22:27Z

Codecov Report

Merging #171 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #171    +/-   ##
======================================
  Coverage     100%   100%            
======================================
  Files          18     20     +2     
  Lines         958   1068   +110     
======================================
+ Hits          958   1068   +110

Impacted Files	Coverage Δ
lib/egg.js	`100% <100%> (ø)`	⬆️
lib/loader/mixin/custom.js	`100% <100%> (ø)`	⬆️
lib/loader/mixin/boot.js	`100% <100%> (ø)`
lib/utils/index.js	`100% <100%> (ø)`	⬆️
lib/loader/egg_loader.js	`100% <100%> (ø)`	⬆️
lib/lifecycle.js	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae38fa4...f74f6f0. Read the comment docs.

popomore · 2018-07-03T14:55:07Z

lib/egg.js

+    }
+  }
+
+  ready(flagOrFunction) {


这个直接用 get-ready 吧

这里没法直接用 get-ready 的，就是上次讲过的第二个 ready 对象需要延迟init

popomore · 2018-07-03T15:07:08Z

lib/egg.js

+    /**
+     * @member {BootClass} EggCore#BootClass
+     */
+    this.BootClass = BootClass;


感觉没必要继承这个类

popomore · 2018-07-03T15:44:09Z

test/fixtures/boot-configDidLoad-error/app.js

+      app.bootLog.push('didReady');
+    }
+
+    async beforeClose() {


这个我们要调整吗，didClose

popomore · 2018-07-03T15:55:29Z

lib/loader/mixin/custom.js

    this.timing.end('Load app.js');
+    this.app[Symbol.for('EggCore#triggerConfigDidLoad')]();


这个应该在 loadconfig 后

loadCustomApp 是在 loadConfig 之后的。

popomore · 2018-07-03T16:09:28Z

逻辑可以在整理下，这里 app 上比较乱，建议放到一个类里面去处理所有逻辑，比如 lifecycle

Boot 类要提前加载，所以要把 customApp 拆分两步，require 和执行
实例化 lifecycle 类，提前 require 所有的 app.js，实例化所有的类。
在 loadConfig 后 trigger didLoadConfig
在 load 后 trigger didLoad
在 didLoad 后 trigger willReady
以下后现在类似

在 lifecycle 管理生命周期，lifecycle ready 后触发 app ready。app 现有的 api 可以直接调用这个类的 api 来注册

fengmk2 · 2018-07-11T13:43:33Z

关注，puml 图也提交到 README 中，或者 egg 文档中。

killagu · 2018-07-11T14:13:57Z

ping @popomore 把生命周期的控制抽取到 Lifecycle 这个类里面了。

popomore

先提这些

popomore · 2018-07-11T15:48:26Z

lib/lifecycle.js

+   */
+  initBoots() {
+    this.boots = this.app[BOOT_FILES]
+      .map(t => (is.function(t) && !is.class(t) ? t(this.app) : t))


这里为啥还要执行下？应该就筛选出类

loadCustom 里面不再执行了，统一只执行了 requireFile ，所以这里执行了一下。兼容旧的方法的写法。

还是分开吧，这里只处理 boot 类，那里保持原样。

popomore · 2018-07-11T16:00:25Z

lib/lifecycle.js

+    this.triggerConfigDidLoad();
+  }
+
+  beforeStart(scope) {


这里改成 registerDidLoad 吧，和 trigger 对应。

app 的 beforeStart 还是调用这个。

加一个 legacyReadyCallback 的 getter 调用 loadReady 的 readyCallback，然后 app 的 readyCallback 调用这个。

popomore · 2018-07-11T16:07:23Z

lib/lifecycle.js

+    return this.closePromise;
+  }
+
+  ready(flagOrFunction) {


这里还是直接用 get-ready 吧

this.loadReady.ready(() => {
this.triggerWillReady()
})

this.bootReady.ready(() => {
this.ready(true)
});

这样把整个流程串起来

如果错误就直接调用 this.ready(err)

app.ready 这个方法中传入的函数实际调用的是 bootReady，而 bootReady 是在 loadReady ready 之后才实例化的。所以 lifecycle 的 ready 方法不能直接由 ready 对象代理。

app.ready 应该是应用完成 ready 后，不是注册到 bootReady

popomore · 2018-07-11T16:11:09Z

lib/lifecycle.js

+    });
+  }
+
+  beforeClose(fn) {


这个也改成 registerDidClose 吧

popomore · 2018-07-11T16:15:02Z

lib/lifecycle.js

+    this.closeSet.add(fn);
+  }
+
+  close() {


这里改成 async 吧，然后判断下是否执行了两次

这个判断我加一下。不过是 break change 了，旧版本是可以 close 多次的。

之前也不行只是返回相同的 promise，看看测试看

现在这里重复执行也是返回相同的 promise，判断不是抛出重复 close 异常的意思吗？

改成 async 会更加简洁，现在还要有个闭包

popomore · 2018-07-11T16:21:07Z

lib/lifecycle.js

+      .filter(t => t.didReady)
+      .map(t => t.didReady.bind(t));
+    const readyCbs = [
+      ...this.readyCallbacks,


这里只需要处理 boot 类的就好了，直接进入到 lifecycle 的 ready 阶段了，开发者自己坚挺 app.ready 不需要我们处理了

popomore · 2018-07-11T16:24:58Z

lib/egg.js

@@ -123,6 +122,7 @@ class EggCore extends KoaApplication {
      logger: this.console,
      serverScope: options.serverScope,
    });
+    this[BOOT_FILES] = [];


这个有用？

popomore · 2018-07-11T16:26:25Z

lib/loader/mixin/custom.js

+        continue;
+      }
+      const file = this.requireFile(filePath);
+      this.app[BOOT_FILES].push(file);


直接用 lifecycle 的 api?

这个阶段应该在 didConfigLoad 之后了，我绝对 loader 最开始可以先加载 boot 文件，然后到这里才执行函数。

dead-horse · 2018-07-11T17:19:00Z

lib/egg.js

+      app: this,
+      logger: this.console,
+    });
+    this.lifecycle.on('error', this.emit.bind(this, 'error'));


用 error => this.emit('error', error) 的写法吧，可以不用 bind 了

popomore · 2018-07-12T15:50:32Z

lib/lifecycle.js

+    return this.app.timing;
+  }
+
+  get legacyReadyCallback() {


直接用函数呗，bind 比较丑

popomore · 2018-07-12T15:58:48Z

lib/lifecycle.js

+   */
+  initBoots() {
+    this.boots = this.bootFiles.map(t => new t(this.app));
+    this[REGISTER_BEFORE_CLOSE]();


是不是可以在这里把所有的都给注册一遍

for (const boot of this.boots) { if (boot.didLoad) { this.didLoadSet.add(boot.didLoad); } if (boot.willReady) { this.willReadySet.add(boot.willReady); } // ...所有生命周期 }

这个没什么必要吧，单独把 function 存起来又要用 bind 了。

popomore · 2018-07-12T16:03:50Z

lib/lifecycle.js

+    this.closeSet.add(fn);
+  }
+
+  close() {


改成 async 会更加简洁，现在还要有个闭包

popomore · 2018-07-12T16:04:52Z

lib/lifecycle.js

+    this.loadReady.ready(error => {
+      debug('didLoad done');
+      if (error) {
+        this.triggerDidReady(error);


如果 error 这里就不用触发了吧

triggerDidReady 同样是通过 this.ready(err => this.triggerDidReady(err)) 触发的

popomore · 2018-07-12T16:07:54Z

lib/lifecycle.js

+      });
+  }
+
+  [INIT_BOOT_READY]() {


这个和 INIT_LOAD_READY 应该是类似的，这里有些差异，比如 readyTimeout 和 stat

popomore · 2018-07-13T03:13:29Z

lib/lifecycle.js

+  /**
+   * init boots and trigger config did config
+   */
+  initBoots() {


这个在哪里调用的

由 loader 在 loadCustomApp/loadCustomAgent 结束后调用

popomore · 2018-07-16T15:22:33Z

initBoots 的位置还是不对，应该在 customApp 前面，比如在 configDidLoad 修改中间件，app.js 里面应该可以取到最新的。

killagu · 2018-07-17T13:49:17Z

@popomore 改对了，测试用例里面也加入了这种情况。

popomore · 2018-08-20T10:33:28Z

lib/lifecycle.js

+  }
+
+  [INIT_BOOT_READY]() {
+    if (!this.bootReady) {


这里会调用两次?

popomore · 2018-08-20T10:42:11Z

lib/lifecycle.js

+        this[REGISTER_READY_CALLBACK](didLoad, this.loadReady, 'Did Load');
+      }
+    }
+    this.loadReady.ready(err => {


这个是不是可以移到 INIT_LOAD_READY 去，定义和 trigger 分开。

popomore · 2018-08-20T10:46:57Z

lib/lifecycle.js

+
+  [INIT_BOOT_READY]() {
+    if (!this.bootReady) {
+      this.bootReady = new Ready({ timeout: this.readyTimeout });


是不是把 https://github.com/node-modules/ready-callback/blob/master/lib/ready.js#L33 去掉，就可以提前创建实例了？

popomore · 2018-08-20T11:00:23Z

lib/loader/mixin/custom.js

  },

+  loadBootFile(fileName) {


我又想了下，是不是增加一个 config.initBoot() 的方法，用来加载 app.js/agent.js，这个方法放到 config.loadPlugin 之后。

popomore · 2018-08-20T11:42:32Z

lib/lifecycle.js

+            await boot.didReady(err);
+          }
+        } catch (e) {
+          this.app.emit('error', e);


这个不用 this.emit?

popomore · 2018-08-20T11:42:50Z

lib/lifecycle.js

+    (async () => {
+      for (const boot of this.boots) {
+        try {
+          if (boot.didReady) {


if 放到外面去

popomore · 2018-08-20T11:49:06Z

lib/egg.js

-    }).on('ready_timeout', id => {
-      this.console.warn('[egg:core:ready_timeout] %s seconds later %s was still unable to finish.', eggReadyTimeoutEnv / 1000, id);
-    });
+  boot() {


这个 API 是不是可以不需要了，triggerDidLoad 应该在 app.js 里面就触发了？

fengmk2 · 2018-08-21T01:51:56Z

egg 文档也改起来，加上这个章节 👍

killagu · 2018-08-23T04:14:46Z

我又想了下，是不是增加一个 config.initBoot() 的方法，用来加载 app.js/agent.js，这个方法放到 config.loadPlugin 之后。

这个 app.js/agent.js 我想想还是不拿出来了，app.js/agent.js 加载是有特殊性的，appWrokerLoader 只加载 app.js，agentWrokerLoader 只加载 agent.js。现在是通过方法的方式来区分的，而在 EggLoader 自身是区分不出是 appWorkerLoader 还是 agentWorkerLoader 的，不能做到只加载需要的文件。

killagu · 2018-08-25T03:15:53Z

ping @popomore

atian25 · 2018-08-27T03:08:06Z

lib/egg.js

+   * @see https://github.com/node-modules/ready
+   * @since 1.0.0
+   * @param {boolean|Error|Function} flagOrFunction -
+   * @return {Promise|null} return promise when argument is undefined


啥时会返回 null ？

应该推荐使用 await app.ready() 的方式而不是 callback 的方式？

使用 ready(() => {}) 的时候是返回 null 的。这里是保证接口的兼容性。

atian25 · 2018-08-27T03:11:32Z

lib/egg.js

-      this.console.warn('[egg:core:ready_timeout] %s seconds later %s was still unable to finish.', eggReadyTimeoutEnv / 1000, id);
-    });
+  async close() {
+    if (this.closePromise) return this.closePromise;


closePromise 是内部变量？用 Symbol 或 _ ？

这个可以有。

atian25 · 2018-08-27T03:13:48Z

lib/lifecycle.js

+
+const utils = require('./utils');
+
+class Lifecycle extends EventEmitter {


要不要继承 sdk-base 啥的？ @popomore

这个 Lifecycle 和 SDKBase 还不太一样，SDKBase 一般是异步 init 的时候用一下。

atian25 · 2018-08-27T03:16:39Z

lib/lifecycle.js

+    this.timing.start('Application Start');
+    // get app timeout from env or use default timeout 10 second
+    const eggReadyTimeoutEnv = Number.parseInt(process.env.EGG_READY_TIMEOUT_ENV || 10000);
+    assert(


这句应该在上面的 parseInt 前面？

Number.isInteger 用 is-type-of ？

parseInt 之后才能使用 Number.isInteger

这个 Number.isInteger 还用 is-type-of 就没什么必要了吧。

atian25 · 2018-08-27T03:17:57Z

lib/lifecycle.js

+
+    this[INIT_READY]();
+    this
+      .on('ready_stat', data => {


ready_done / ready_suc 会不会好点？

这个是 ready-callback emit 过来的事件了，不好改了。

atian25 · 2018-08-27T03:19:34Z

lib/lifecycle.js

+  init() {
+    assert(this[INIT] === false, 'lifecycle have been init');
+    this[INIT] = true;
+    this.boots = this.bootFiles.map(t => new t(this.app));


这个 t 是一个 class ？上面的 addBootFile 是一个 file

是一个 class，在 customLoader 里面有这样一个判断 is.class(bootFile)

那 addBootFile 这个命名会不会有点误导？ addBootHook(Clz) 这样是不是好点？

是有点误导，改了。

atian25 · 2018-08-27T03:21:05Z

lib/lifecycle.js

+
+  registerBeforeClose(fn) {
+    assert(is.function(fn), 'argument should be function');
+    assert(this.isClose === false, 'app has been closed');


isClosed ? 不过要看看之前的风格

之前就是 isClose 😂

不过还是改一下吧。改一下合理一点

1. add `Boot` Class, developer can inherit from it. 2. add `startBoot`, application like egg should call `startBoot` after all files loaded. Refs: eggjs/egg#2520

fengmk2 · 2018-09-04T13:34:16Z

@killagu @popomore @atian25 是否可以合并了？

atian25 · 2018-09-05T00:48:39Z

+1

popomore · 2018-09-06T09:28:56Z

[email protected]

initial-wu · 2018-09-29T04:46:06Z

lifecycle 内部 beforeClose 钩子保存在一个 Set 里面是出于什么考虑？私以为用数组貌似也可以？

killagu · 2018-09-29T06:08:04Z

@initial-wu 可以防止重复添加。

initial-wu · 2018-09-29T07:47:43Z

@initial-wu 可以防止重复添加。

明白。btw，考虑调整一下这里的写法吗 #171 (comment)

killagu · 2018-09-29T07:52:04Z

没明白你说的意思。 get legacyReadyCallback 这里已经改掉了。

initial-wu · 2018-09-29T10:32:58Z

没明白你说的意思。 get legacyReadyCallback 这里已经改掉了。

这个评论的锚点应该已经变了，@popomore 可能是想说改成这样？

killagu · 2018-09-29T14:56:53Z

之前说的就是你截图的地方，这里不用在封装一遍了，会多一层调用栈。

1. Add `Lifecycle` class, control EggCore lifecycle. 2. After config, extend, app.js, angent.js loaded, loader should call `initBoots`. 3. After all files loaded, loader should call `triggerDidLoad`. Refs: eggjs/egg#2520

killagu requested review from popomore and dead-horse July 3, 2018 12:15

killagu changed the title ~~feat: impl boot methods~~ [WIP] feat: impl boot methods Jul 3, 2018

popomore reviewed Jul 3, 2018

View reviewed changes

killagu force-pushed the impl-boot branch from ad50db8 to 83edc34 Compare July 11, 2018 14:04

popomore reviewed Jul 11, 2018

View reviewed changes

dead-horse reviewed Jul 11, 2018

View reviewed changes

killagu force-pushed the impl-boot branch from 83edc34 to a96ad4a Compare July 12, 2018 03:37

popomore reviewed Jul 13, 2018

View reviewed changes

killagu force-pushed the impl-boot branch 2 times, most recently from a273973 to 451d41f Compare July 19, 2018 02:37

popomore reviewed Aug 20, 2018

View reviewed changes

lib/lifecycle.js Outdated

}

[INIT_BOOT_READY]() {

if (!this.bootReady) {

Copy link

Member

popomore Aug 20, 2018

Choose a reason for hiding this comment

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

这里会调用两次?

popomore approved these changes Aug 20, 2018

View reviewed changes

popomore reviewed Aug 20, 2018

View reviewed changes

killagu changed the title ~~[WIP] feat: impl boot methods~~ feat: impl boot methods Aug 23, 2018

killagu force-pushed the impl-boot branch from b8a8220 to 3b40f7c Compare August 23, 2018 05:43

atian25 reviewed Aug 27, 2018

View reviewed changes

killagu added 2 commits August 30, 2018 10:50

feat: impl boot methods

74d2137

1. add `Boot` Class, developer can inherit from it. 2. add `startBoot`, application like egg should call `startBoot` after all files loaded. Refs: eggjs/egg#2520

f

f74f6f0

killagu force-pushed the impl-boot branch from 719a201 to f74f6f0 Compare August 30, 2018 02:50

popomore approved these changes Sep 6, 2018

View reviewed changes

popomore merged commit 9d2f2fc into master Sep 6, 2018

popomore deleted the impl-boot branch September 6, 2018 05:49

		this.timing.end('Load app.js');
		this.app[Symbol.for('EggCore#triggerConfigDidLoad')]();


		const utils = require('./utils');

		class Lifecycle extends EventEmitter {

feat: impl boot methods #171

feat: impl boot methods #171

Conversation

killagu commented Jul 3, 2018 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

killagu commented Jul 3, 2018

codecov bot commented Jul 3, 2018 • edited Loading

Codecov Report

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

popomore commented Jul 3, 2018

fengmk2 commented Jul 11, 2018

killagu commented Jul 11, 2018

popomore left a 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

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

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

popomore Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

popomore commented Jul 16, 2018

killagu commented Jul 17, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fengmk2 commented Aug 21, 2018

killagu commented Aug 23, 2018

killagu commented Aug 25, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

killagu commented Jul 3, 2018 •

edited

Loading

codecov bot commented Jul 3, 2018 •

edited

Loading

popomore Jul 12, 2018 •

edited

Loading