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

refactor: app.cluster auto bind this #570

Merged
merged 2 commits into from
Mar 18, 2017
Merged

Conversation

gxcsoccer
Copy link
Contributor

@gxcsoccer gxcsoccer commented Mar 14, 2017

Checklist
  • npm test passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

plugin development

Description of change
const cluster = app.cluster;
const client = cluster(XXXClient).create();
// ...

@gxcsoccer gxcsoccer force-pushed the autobind-app-to-cluster branch from 39cdbdc to b9f8c5b Compare March 14, 2017 11:35
// agent worker is leader, app workers are follower
options.isLeader = this.type === 'agent';
options.logger = this.coreLogger;
const client = cluster(clientClass, options);
Copy link
Member

Choose a reason for hiding this comment

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

不是这样的吧

Copy link
Contributor Author

Choose a reason for hiding this comment

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

等下 加个用例

Copy link
Member

Choose a reason for hiding this comment

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

放这里有用么? 下面的 this 还是取不到吧,感觉需要将方法定义在下面,然后 this.cluster = this.cluster.bind(this) ?

Copy link
Member

Choose a reason for hiding this comment

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

他是 arrow function

@codecov
Copy link

codecov bot commented Mar 14, 2017

Codecov Report

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

@@          Coverage Diff          @@
##           master   #570   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          26     26           
  Lines         632    633    +1     
=====================================
+ Hits          632    633    +1
Impacted Files Coverage Δ
lib/egg.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 4687f0f...dd12935. Read the comment docs.

@gxcsoccer gxcsoccer force-pushed the autobind-app-to-cluster branch from 02557ec to a26eeb1 Compare March 15, 2017 01:39
@@ -6,7 +6,8 @@ module.exports = function(app) {
const done = app.readyCallback('register_client', {
isWeakDep: app.config.runMode === 0,
});
app.registryClient = app.cluster(RegistryClient).create();
const cluster = app.cluster;
app.registryClient = cluster(RegistryClient).create();
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@fengmk2 fengmk2 Mar 18, 2017

Choose a reason for hiding this comment

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

options.cluster = app.cluster;
app.registryClient = new RegistryClient(options);

这样才对的

Copy link
Member

Choose a reason for hiding this comment

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

哦,这里的 RegistryClient 是 DataClient

Copy link
Member

@fengmk2 fengmk2 left a comment

Choose a reason for hiding this comment

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

LGTM

@fengmk2 fengmk2 merged commit 984d732 into master Mar 18, 2017
@fengmk2 fengmk2 deleted the autobind-app-to-cluster branch March 18, 2017 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants