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(singleton): support async create function #2382

Merged
merged 6 commits into from
Apr 14, 2018
Merged

Conversation

dead-horse
Copy link
Member

@dead-horse dead-horse commented Apr 13, 2018

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
  • support config.client to be async function
  • support create method to be async function
  • add singleton.createInstanceAsync when create method is async function

@@ -48,10 +52,23 @@ class Singleton {
}

createInstance(config) {
// async creator only support createInstanceAsync
assert(!is.asyncFunction(this.create),
Copy link
Member Author

Choose a reason for hiding this comment

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

有一个可以讨论的地方,其实可以判断 this.create 来决定 createInstance 方法是返回创建后的实例还是返回一个 promise,这样就不需要 createInstanceAsync 方法了,但是 createInstance 方法就不统一了,也不好。所以我倾向于新增一个 createInstanceAsync 方法来应对这种特殊场景。

Copy link
Member

Choose a reason for hiding this comment

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

主要还是兼容问题,如果没有问题我倾向于 createInstance 就是一个 async function

Copy link
Member Author

Choose a reason for hiding this comment

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

就是因为有兼容问题才这样的,createInstance 不能直接返回一个 async function

lib/egg.js Outdated
*/
addSingleton(name, create) {
const options = {};
options.name = name;
options.create = create;
options.app = this;
const singleton = new Singleton(options);
singleton.init();
const done = this.readyCallback(`singleton:${name}`);
Copy link
Member

Choose a reason for hiding this comment

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

use beforeStart?

Copy link
Member Author

Choose a reason for hiding this comment

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

改了

@@ -48,10 +52,23 @@ class Singleton {
}

createInstance(config) {
// async creator only support createInstanceAsync
assert(!is.asyncFunction(this.create),
Copy link
Member

Choose a reason for hiding this comment

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

主要还是兼容问题,如果没有问题我倾向于 createInstance 就是一个 async function

@codecov
Copy link

codecov bot commented Apr 13, 2018

Codecov Report

Merging #2382 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2382      +/-   ##
==========================================
+ Coverage   99.61%   99.61%   +<.01%     
==========================================
  Files          29       29              
  Lines         773      783      +10     
==========================================
+ Hits          770      780      +10     
  Misses          3        3
Impacted Files Coverage Δ
lib/egg.js 100% <100%> (ø) ⬆️
lib/core/singleton.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 a5b6731...02e30aa. Read the comment docs.

@dead-horse
Copy link
Member Author

合并了,后面补一下文档,这个就不 pick 到 1.x 了

@dead-horse dead-horse merged commit 3a489b6 into master Apr 14, 2018
@dead-horse dead-horse deleted the async-singleton branch April 14, 2018 02:19
popomore pushed a commit that referenced this pull request Apr 14, 2018
feat(singleton): support async create function (#2382)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants