refact: use cluster-client #2

shaoshuai0102 · 2017-01-23T12:03:46Z

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

shaoshuai0102 · 2017-01-23T12:05:39Z

ci 需要修复 cluster-client 和 egg-mock 两个模块，我本地 link 临时改的

shaoshuai0102 · 2017-01-23T12:05:56Z

然后 unwatch 没法实现，因为 cluster-client 还不支持 unsub

luckydrq · 2017-01-23T12:07:44Z

agent.js

-  agent.watcher.ready(() => {
-    logger.info('[egg:watcher:agent] watcher start success');
-    done();
+  agent.beforeStart(function* () {


现在不推荐用 readyCallback 了？

是的，原因是如果用 readyCallback 很容易引导模块使用 callback 模式，大家希望引导模块开发到 aa 或者 generator

那 readyCallback 这个api准备废弃么？

@luckydrq 看看这个 eggjs/egg#227

@luckydrq 老代码没有升级的，继续使用 readyCallback，重构的代码都直接使用 generator 写 init 好了

popomore · 2017-01-23T13:18:32Z

agent.js


  try {
    const EventSource = require('./lib/event-sources/' + type);
-    agent.watcher.useEventSource(new EventSource(agent));
+    watcher.useEventSource(new EventSource(agent));


为啥这个要加到外面？

在拓展 watcher eventSource 类型的插件中需要调用：

// agent.js if (config.watcher.type === 'custom') { agent.watcher.useEventSource(new CustomEventSource(agent)); }

提供额外方法的这样设计是为了让 watcher 插件不用依赖扩展的 custmwatcher 插件。可以看下内部的 watcher-alipay在拓展 watcher eventSource 类型的插件中需要调用：

// agent.js if (config.watcher.type === 'custom') { agent.watcher.useEventSource(new CustomEventSource(agent)); }

提供额外方法的这样设计是为了让 watcher 插件不用依赖扩展的 custmwatcher 插件。可以看下内部的 watcher-alipay

那这个 API 只在 agent 开放？

通过原来的方式 load 不好么，如果切换到 custom 的 type，就自动调用 config.customEventSource。

现在支持多个 eventSource 么

原来就是用的 useEventSource API。你指的原来的方式是？

通过 config 的方式是可以完成的。只是要变更 API，我是觉得收益不大，可以先不改这个

要做到尽量保持 app/agent 的一致性。

那我借着这次 egg 1.0 把 API 给改了吧

popomore · 2017-01-23T13:21:17Z

lib/watcher.js

+    // cluster-client stops invoking any method if client is not ready
+    // so set ready first. The app start logic depends on eventSource
+    // if needed to finish initialization before app starting.
+    this.ready(true);


不是应该加载完 eventSource 后再 ready 么，调用方法是什么意思？

注释里写了啊，cluster-client 的要求，如果没有 ready 暴露的方法不执行。内部使用了 client.ready 进行 invoke。所以需要 eventSource 自己进行 ready 时序控制，我在测试用例里有

这个好难理解，还没有 get 到。

这个是 cluster-client 的约束

不过 watcher 启动好像也不需要等待

popomore · 2017-01-23T13:22:16Z

package.json

    "ci": "npm run lint && npm run cov",
-    "autod": "autod"
+    "autod": "autod -P autod-egg"


这个应该不用 autod-egg 吧

popomore · 2017-01-23T13:23:03Z

test/development.test.js

-      });
+      .expect('agent watch success');
+
+    yield done => setTimeout(done, 100);


用 ko-sleep 吧

这个没事吧，无成本包装嘛

popomore · 2017-01-24T01:10:16Z

统一吧，thunk 之后不好替换 Shawn <notifications@github.com>于2017年1月24日周二08:19写道：

…

***@***.**** commented on this pull request. ------------------------------ In test/development.test.js <#2>: > - .expect(200) - .expect(res => { - const lastCount = count; - count = parseInt(res.text); - count.should.equal(lastCount); - }) - .end(done); - }, 100); - }, 100); - }); - }); - }, 100); - }); + .expect('agent watch success'); + + yield done => setTimeout(done, 100); 这个没事吧，无成本包装嘛 — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#2>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAWA1UMfJIavk7GmkNjqOKv8lPGAbOA0ks5rVUN0gaJpZM4Lq9vt> .

fengmk2 · 2017-01-24T07:16:06Z

app.js

-    },
-
-  }, app.config.watcher);
+module.exports = function(app) {


fengmk2 · 2017-01-24T07:17:58Z

app.js

+
+  app.beforeStart(function* () {
+    yield watcher.ready();
+    logger.info('[watcher:app] watcher start success');


跟下面的统一 [egg-watcher:app]

fengmk2 · 2017-01-24T07:20:28Z

lib/watcher.js

    this.on(path, callback);
  }

+  /*
+  // TODO wait unsubscribe implementation of cluster-client


@shaoshuai0102 可以去 https://github.com/node-modules/cluster-client 提交 unsubscribe 的实现

fengmk2 · 2017-01-24T07:21:16Z

test/watcher.test.js

@@ -18,20 +18,27 @@ describe('test/watcher.test.js', () => {
    });
    app.ready(() => {
      const content = fs.readFileSync(__dirname + '/fixtures/apps/watcher-type-default/logs/watcher-type-default/egg-agent.log').toString();
-      content.should.containEql('defaultEventSource watcher will NOT take effect');
+      assert(content.indexOf('defaultEventSource watcher will NOT take effect') > -1);


content.includes(str)

codecov-io · 2017-01-25T06:58:22Z

Current coverage is 80.72% (diff: 91.66%)

Merging #2 into master will increase coverage by 23.18%

@@             master         #2   diff @@
==========================================
  Files             6          8     +2   
  Lines            73         83    +10   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits             42         67    +25   
+ Misses           31         16    -15   
  Partials          0          0

Powered by Codecov. Last update 2b57dab...729ee61

popomore · 2017-01-25T07:13:25Z

.travis.yml

@@ -1,7 +1,6 @@
 sudo: false
 language: node_js
 node_js:
-  - '4'
  - '6'


再加个 7

popomore · 2017-01-25T07:19:34Z

app.js

+  const watcher = app.watcher = app.cluster(Watcher)
+      .delegate('watch', 'subscribe')
+      .create(config)
+      .on('error', e => logger.error(e));


感觉可以让 app.cluster 来处理 eggjs/egg#304 (comment)

如果还有其他的自定义事件，cluster-client 做不来，而且职责不清了。

popomore · 2017-01-25T07:25:35Z

其他看了下，没意见。

shaoshuai0102 · 2017-01-25T07:46:16Z

别先合并，我改下通过配置的方式，而不是 API 方式拓展

shaoshuai0102 · 2017-01-25T08:01:51Z

@popomore

popomore · 2017-01-25T08:16:32Z

config/config.default.js

 /**
 * watcher options
 * @member Config#watcher
 * @property {string} type - event source type
 */
 exports.watcher = {
  type: 'default', // default event source
+  eventSources: {


有了 eventSources 是不是就不需要 type 了

//config.default.js exports.watcher = { eventSource: path.join(__dirname, '../lib/event-sources/default'), }; //config.local.js exports.watcher = { eventSource: path.join(__dirname, '../lib/event-sources/development'), };

shaoshuai0102 · 2017-01-25T08:47:29Z

插件配置 eventsource 使用者使用 type 进行切换。这样 chair 这种业务框架可以集成多种，然后开发者自己配 type 就好了。否则需要通过开关插件来做

…

在 2017年1月25日，下午4:16，Haoliang Gao ***@***.***> 写道： @popomore commented on this pull request. In config/config.default.js: > /** * watcher options * @member Config#watcher * @Property {string} type - event source type */ exports.watcher = { type: 'default', // default event source + eventSources: { 有了 eventSources 是不是就不需要 type 了 //config.default.js exports.watcher = { eventSource: path.join(__dirname, '../lib/event-sources/default'), }; //config.local.js exports.watcher = { eventSource: path.join(__dirname, '../lib/event-sources/development'), }; — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

popomore · 2017-01-25T09:07:18Z

+1

popomore · 2017-01-25T09:13:22Z

egg-watcher@2.0.0

popomore · 2017-01-25T09:14:17Z

忘记加链接了 eggjs/egg#286

[skip ci] ## 1.0.0 (2024-12-18) ### ⚠ BREAKING CHANGES * drop Node.js < 18.19.0 support part of eggjs/egg#3644 eggjs/egg#5257  ## Summary by CodeRabbit - **New Features** - Introduced new configuration files for managing watcher settings in different environments (default, local, unittest). - Added a new `Boot` class to manage application lifecycle and watcher initialization. - Implemented `Watcher` class for monitoring file changes with event handling. - Added `DevelopmentEventSource` and `DefaultEventSource` classes for specific event source management. - **Bug Fixes** - Enhanced path handling in various modules to ensure correct file watching functionality. - **Documentation** - Updated `README.md` with project name change and improved structure. - **Tests** - Introduced new unit tests for watcher functionality and refactored existing test files to improve clarity and structure. - **Chores** - Removed deprecated configuration files and streamlined project structure. - Updated TypeScript configuration for stricter type-checking.  ### Features * [BREAK CHANGE] use cluster-client ([#2](#2)) ([90a8b47](90a8b47)) * add event source event logs ([#9](#9)) ([b351795](b351795)) * pass custom options ([#4](#4)) ([cf9fcac](cf9fcac)) * support cjs and esm both by tshy ([#14](#14)) ([c80fea0](c80fea0)) ### Bug Fixes * should support watch one file multiple times ([#6](#6)) ([6d84e21](6d84e21)) * spell error on watcher.js ([#13](#13)) ([9ab2eed](9ab2eed))

shaoshuai0102 requested a review from popomore January 23, 2017 12:03

shaoshuai0102 assigned popomore Jan 23, 2017

luckydrq reviewed Jan 23, 2017

View reviewed changes

popomore reviewed Jan 23, 2017

View reviewed changes

package.json

"ci": "npm run lint && npm run cov",

"autod": "autod"

"autod": "autod -P autod-egg"

Copy link

Member

popomore Jan 23, 2017

Choose a reason for hiding this comment

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

这个应该不用 autod-egg 吧

popomore reviewed Jan 23, 2017

View reviewed changes

shaoshuai0102 force-pushed the refact-use-cluster-client branch 2 times, most recently from e506404 to d12eedc Compare January 24, 2017 01:03

fengmk2 reviewed Jan 24, 2017

View reviewed changes

app.js

},

}, app.config.watcher);

module.exports = function(app) {

Copy link

Member

fengmk2 Jan 24, 2017

Choose a reason for hiding this comment

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

app => {

fengmk2 reviewed Jan 24, 2017

View reviewed changes

shaoshuai0102 force-pushed the refact-use-cluster-client branch 2 times, most recently from 7779abe to 99355e5 Compare January 24, 2017 07:22

refact: use cluster-client

8f9b916

shaoshuai0102 force-pushed the refact-use-cluster-client branch 2 times, most recently from cec191e to c339426 Compare January 25, 2017 06:58

popomore reviewed Jan 25, 2017

View reviewed changes

chore: only support node 6+

729ee61

shaoshuai0102 force-pushed the refact-use-cluster-client branch from c339426 to 729ee61 Compare January 25, 2017 07:31

fengmk2 approved these changes Jan 25, 2017

View reviewed changes

refact: [BREAK CHANGE] remove useEventSource API

d1f910f

popomore reviewed Jan 25, 2017

View reviewed changes

popomore approved these changes Jan 25, 2017

View reviewed changes

popomore merged commit 90a8b47 into master Jan 25, 2017

popomore deleted the refact-use-cluster-client branch January 25, 2017 09:08

refact: use cluster-client #2

refact: use cluster-client #2

Conversation

shaoshuai0102 commented Jan 23, 2017

Checklist

Affected core subsystem(s)

Description of change

shaoshuai0102 commented Jan 23, 2017

shaoshuai0102 commented Jan 23, 2017

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 commented Jan 24, 2017 via email

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

codecov-io commented Jan 25, 2017 • edited Loading

Current coverage is 80.72% (diff: 91.66%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

popomore commented Jan 25, 2017

shaoshuai0102 commented Jan 25, 2017

shaoshuai0102 commented Jan 25, 2017

Choose a reason for hiding this comment

shaoshuai0102 commented Jan 25, 2017 via email

popomore commented Jan 25, 2017

popomore commented Jan 25, 2017

popomore commented Jan 25, 2017

codecov-io commented Jan 25, 2017 •

edited

Loading