refactor: classify #23

atian25 · 2017-09-28T08:21:55Z

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

重构，使用类的写法。
支持 cronOptions，配置时区等。close [egg-schedule] cron-style 建议支持自定义 timezone egg#1274
自定义 schedule 类型改为 agent.schedule.use(type, Strategy) 方式
暴露 agent.schedule / agent.ScheduleStrategy
messenger 的 action 统一为 egg-schedule，把 key 放到 data 里面传递，减少事件注册个数。
schedule key 去掉前缀，直接为 fullpath
[RFC] 订阅模型 egg#1468

popomore · 2017-09-28T08:54:34Z

agent.js

+  agent.messenger.once('egg-ready', () => {
+    // Compatible
+    for (const type of Object.keys(handlers)) {
+      schedule.use(type, handler2Class(type, handlers[type]));


这里可以加个 depd

atian25 · 2017-09-28T08:58:52Z

lib/schedule.js

+      }
+
+      const handler = new Strategy(this.agent, key, schedule);
+      this.handler.set(key, handler);


这里还没太想好 stop() 怎么用，目前的 key 是全路径，这个不好传递进来。
初步想法是 exports.key 的方式给用户去定义，我们检测不允许冲突。

没想好之前，handler 这里可能先不存了，要干掉。
因为如果存的话，就要考虑到 Strategy 的退出机制，那边退出后这边也要跟着移除。

atian25 · 2017-09-28T09:01:47Z

lib/strategy/base.js

+    try {
+      do {
+        // TODO: try
+        nextTick = interval.next().getTime();


这里要 try 下，支持 cronOptions 后，会有 endDate 导致的主动退出

popomore · 2017-09-28T10:17:10Z

lib/schedule.js

+    this.agent = agent;
+    this.handler = new Map();
+    this.strategy = new Map();
+    this.use('worker', WorkerStrategy);


这个也写在 agent.js 里吧

popomore · 2017-09-28T10:18:20Z

lib/schedule.js

+const AllStrategy = require('./strategy/all');
+const Strategy = require('./strategy/base');
+
+module.exports = class Schedule {


用 sdk-base？实现 _init？这样不需要手动调用 start 了。

感觉继承 sdk-base 好像也没啥用处，就是 nextTick 自动调用 init，因为 handler 是在 ready 里面才注册的，这样的话就要把 Schedule 的实例化也放进来，感觉继承也没啥好处

popomore · 2017-09-28T10:20:53Z

lib/schedule.js

+      }
+
+      const handler = new Strategy(this.agent, key, schedule);
+      this.handler.set(key, handler);


这个 handler 存着有啥用？

上面有提到，#23 (comment)

实现 close 和 beforeClose 的时候要移除掉?

之前是想在 agent 里面可以支持 close 掉指定的任务。这个不知道有没有场景？

如果没有的话，可以考虑不存 handler，只在 Strategy 内部，判断到 agent 的那个状态位后，自己退出循环（目前是跳过，不是退出）

popomore · 2017-09-28T10:27:58Z

lib/schedule.js

+  }
+
+  start() {
+    this.agent.disableSchedule = false;


这个可以写在 agent.js 里

agent.disableSchedule = false; agent.on('close', () => { agent.disableSchedule = true; });

其他都是只读。

popomore · 2017-09-28T10:34:46Z

lib/schedule.js

+
+      const Strategy = this.strategy.get(type);
+      if (!Strategy) {
+        const err = new Error(`schedule type [${type}] is not defined`);


is not supported?

popomore · 2017-09-28T10:55:14Z

lib/strategy/base.js

+const ms = require('humanize-ms');
+const safetimers = require('safe-timers');
+
+module.exports = class BaseStrategy {


BaseStrategy 承载一个功能就是保留 sender

原来的扩展

agent[SCHEDULE_HANDLER].cluster = (schedule, sender) => { listenXXX(schedule.event, () => { sender.one(); }); };

期望的扩展

class ClusterStrategy extends BaseStrategy { constructor(scheduleInfo) { super(scheduleInfo); // 这里实现触发时机 listenXXX(scheduleInfo.event, () => { this.sendOne(); }); } } agent.use('cluster', ClusterStrategy);

感觉不需要传递 agent，入参是 scheduleInfo，有 sendOne 和 sendAll 方法。

sender 目前的设计，是每次都生成一个，bind key 的，感觉有点别扭。

传入 agent 的话，Strategy 里面可以调用 agent.config 和 agent.logger。

Strategy 还是简单点

popomore · 2017-09-28T11:54:30Z

lib/strategy/base.js

+const ms = require('humanize-ms');
+const safetimers = require('safe-timers');
+
+module.exports = class BaseStrategy {


然后在搞个本地的基类实现现有的功能

class LocalStrategy extends BaseStrategy { start(listener) { startXXX(listener); } close() { // 关闭 interval } }); class AllLocalStrategy extends LocalStrategy { constructor(scheduleInfo) { super(scheduleInfo); this.start(() => { this.sendAll(); }); } } class WorkerLocalStrategy extends LocalStrategy { constructor(scheduleInfo) { super(scheduleInfo); this.start(() => { this.sendOne(); }); } }

嗯，这个可以再拆下。

popomore · 2017-09-28T11:57:56Z

应该暴露 app.ScheduleStrategy

codecov · 2017-09-28T13:51:31Z

Codecov Report

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

@@          Coverage Diff          @@
##           master    #23   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      9    +6     
  Lines         111    168   +57     
=====================================
+ Hits          111    168   +57

Impacted Files	Coverage Δ
lib/load_schedule.js	`100% <ø> (ø)`	⬆️
lib/strategy/all.js	`100% <100%> (ø)`
lib/timer.js	`100% <100%> (ø)`
lib/strategy/base.js	`100% <100%> (ø)`
lib/schedule.js	`100% <100%> (ø)`
app.js	`100% <100%> (ø)`	⬆️
agent.js	`100% <100%> (ø)`	⬆️
app/extend/agent.js	`100% <100%> (ø)`
lib/strategy/worker.js	`100% <100%> (ø)`
... and 5 more

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 729f0f3...4fd1f03. Read the comment docs.

popomore · 2017-09-28T14:27:34Z

lib/strategy/base.js

+'use strict';
+
+module.exports = class BaseStrategy {
+  constructor(agent, key, scheduleInfo) {


传一个对象就好了，不然继承的时候不好写

是指三个作为一个对象传入还是?

你看下 base 和 timer，没有 agent 的话不行。

popomore · 2017-09-28T14:28:57Z

lib/strategy/base.js

+  constructor(agent, key, scheduleInfo) {
+    this.agent = agent;
+    this.key = key;
+    this.scheduleInfo = scheduleInfo;


要不就叫 schedule 好了，和定义的名字一样

popomore · 2017-09-28T14:30:08Z

lib/strategy/local.js

+      throw new Error('[egg-schedule] schedule.interval or schedule.cron must be present');
+    }
+
+    const send = () => {


send 做参数传入不好？

这个timer 也可以做个单例，然后在子类 timer.start(key, cb)，并且管理所有的 timer，可以直接关闭 timer 不需要在这里实现 close

嗯，跟这个一起的，明早再想想，没电了现在。其他几个 review 改了，刚提交了一份最新了。

改为参数传入了

单例的那个明天再看看

popomore · 2017-09-28T14:55:58Z

lib/strategy/base.js

+    this.agent.messenger.send(this.key);
+  }
+
+  start() {


这里不需要实现 start

基类里面不写个空函数? 毕竟 schedule 那边会调用的

popomore · 2017-09-28T15:03:51Z

lib/strategy/worker.js

+module.exports = class WorkerStrategy extends Strategy {
+  constructor(...args) {
+    super(...args);
+    this.type = 'worker';


这个不用了吧

好，在外面 use 时有设置 prototype.type 了

popomore · 2017-09-28T15:08:57Z

lib/strategy/worker.js

@@ -0,0 +1,14 @@
+'use strict';
+
+const Strategy = require('./timer');


这个文件在哪

这个timer 也可以做个单例，然后在子类 timer.start(key, cb)，并且管理所有的 timer，可以直接关闭 timer 不需要在这里实现 close

timer 文件没看到？刷新下。

我在想是不是把所有的 Strategy 的实例，都在自己的构造函数里面存到 static 去？
然后 Strategy 提供一个 static close()

这样的好处：

schedule 那边就不用搞个 handler map 了。

在 cron 里面触发 endDate 后，把自己 close 掉，就可以自己回收了，否则不好通知 schdule。

觉得怎么样？

endDate 是哪里出发的

我期望 strategy 不需要 start 方法了，都在构造函数做就好了，基本都是注册一个 sender

默认的就是通过 timer 注册一个 sender

嗯，timer 的改了提交了。

popomore · 2017-09-28T15:38:48Z

agent.js

-  };
+  const handlers = agent[SCHEDULE_HANDLER] = {};
+  agent.schedule = new Schedule(agent);
+  agent.ScheduleStrategy = BaseStrategy;


这个写在 extend，没有插件顺序问题

popomore · 2017-09-28T15:42:59Z

agent.js

-      err.message = `[egg-schedule] parse cron instruction(${schedule.cron}) error: ${err.message}`;
-      throw err;
+  agent.messenger.once('egg-ready', () => {
+    agent.schedule.use('worker', WorkerStrategy);


现在定义可以放在外面了

兼容的那几个不行吧？放外面的话，执行顺序是怎么样的？

popomore · 2017-09-28T15:47:34Z

agent.js

+    agent.schedule.use('all', AllStrategy);
+    //TODO: compatible, will remove at next major
+    const keys = Object.keys(handlers);
+    if (keys.length) agent.deprecate('should use `schedule.use()` instead of `agent[Symbol.for(\'egg#scheduleHandler\')]` to register handler.');


放到循环里提示？

popomore · 2017-09-28T15:49:59Z

lib/schedule.js

+
+  use(type, clz) {
+    assert(typeof clz.prototype.start === 'function', `schedule type [${type}] should implement \`start\` method`)
+    clz.prototype.type = type;


这个 type 不要了，class 都知道自己是谁了，还需要这个标识？

嗯，可以干掉了。

popomore · 2017-09-28T15:53:30Z

lib/schedule.js

+  }
+
+  close() {
+    this.closed = true;


实际化设 false ?

popomore · 2017-09-28T15:54:35Z

lib/schedule.js

+    for (const handler of this[HANDLER].values()) {
+      handler.close();
+    }
+    this.agent.coreLogger.info('[egg-schedule] close tasks.');


写到 for 循环？把 key 打出来

popomore · 2017-09-28T15:57:33Z

lib/strategy/timer.js

+    }
+
+    if (interval) {
+      this.interval = this.safeInterval(send, ms(interval));


这里每个配置不需要 return 么？

popomore · 2017-09-28T16:01:42Z

lib/strategy/worker.js

@@ -0,0 +1,14 @@
+'use strict';
+
+const Strategy = require('./timer');


endDate 是哪里出发的

popomore · 2017-09-28T16:21:22Z

lib/timer.js

+      this.startCron(key, interval, listener);
+    }, nextTick - now);
+
+    this.timer.set(key, tid);


每个递归都覆盖？

嗯，因为是 setTimeout ，每次都是一个新的了，要记录最后一个

popomore · 2017-09-28T16:22:57Z

lib/strategy/worker.js

+module.exports = class WorkerStrategy extends Strategy {
+  constructor(...args) {
+    super(...args);
+    this.timer = new Timer();


我的意思是用单例，不用多实例

this.agent.scheduleTimer.start(key, schedule, () => this.sendOne());

然后 start 和 close 都可以删掉

这样就是把 timer 提到最外面了，现在这样也就两个实例。

提的话，我更倾向于提到 agent.schedule.timer 就好了

主要是这个 timer 就是内置的 strategy 用的，把他当作插件来考虑看看。

popomore · 2017-09-28T16:27:01Z

agent.js

-    agent.disableSchedule = true;
-    return;
+  agent.beforeClose(() => {
+    return agent.schedule.close();


agent.scheduleTimer.close

popomore · 2017-09-29T01:36:07Z

lib/schedule.js

+  }
+
+  start() {
+    this.closed = false;


这里是不是不用设了

如果手动调用一次 close，然后再调用 start 这种场景呢？

popomore · 2017-09-29T01:44:23Z

agent.js


-  if (schedule.interval) {


@dead-horse 这里原来就可以设置多个的吗

popomore · 2017-09-29T01:45:33Z

agent.js

+        });
+      }
+      close() {
+        this.agent.logger.warn(`schedule type [${type}] stop is not implemented yet`);


没有 close 就不要实现了。

popomore · 2017-09-29T09:22:14Z

test/fixtures/customType/agent.js

@@ -6,4 +6,14 @@ module.exports = function(agent) {
  agent[SCHEDULE_HANDLER].custom = function(schedule, sender) {
    setInterval(sender.one, schedule.interval);
  };
+
+  class ClusterStrategy extends agent.ScheduleStrategy {


分为两个例子吧

popomore · 2017-09-29T09:31:45Z

agent.js

-    } catch (err) {
-      err.message = `[egg-schedule] parse cron instruction(${schedule.cron}) error: ${err.message}`;
-      throw err;
+  agent.messenger.once('egg-ready', () => {


加一下注释，需要等插件注册 strategy

加了，在下一行

popomore · 2017-09-29T09:34:46Z

lib/timer.js

+      throw new Error('[egg-schedule] schedule.interval or schedule.cron must be present');
+    }
+
+    if (interval) {


这里我的意思是如果一个 schedule 同时配置了 interval cron immediate 呢？应该匹配到就返回。

嗯，这个要看之前是怎么样的了，cc @dead-horse

之前的逻辑好像就是这样的，都配置了就都执行

嗯，这个就保持原来逻辑

popomore · 2017-09-29T09:35:23Z

其他 +1，剩下的 @dead-horse 看看，估计要等国庆后了。

popomore · 2017-09-29T09:36:10Z

CI 好像不是很稳

popomore · 2017-10-09T01:58:56Z

@dead-horse 看看

dead-horse · 2017-10-11T05:47:18Z

+1

atian25 · 2017-10-11T05:59:04Z

2.5.0

refactor: classify

24fbf8a