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: lifecycle base class #3581

Merged
merged 1 commit into from
Apr 16, 2019
Merged

feat: lifecycle base class #3581

merged 1 commit into from
Apr 16, 2019

Conversation

killagu
Copy link
Contributor

@killagu killagu commented Apr 1, 2019

Checklist
  • npm test passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)
Description of change

Fixes:

@killagu killagu requested a review from atian25 April 1, 2019 08:46
index.js Outdated Show resolved Hide resolved
@atian25
Copy link
Member

atian25 commented Apr 1, 2019

PR title 应该是 feat 了,实现了 #3454 这个 RFC

index.js Outdated Show resolved Hide resolved
return this[WORKER];
}

get agent() {
Copy link
Member

Choose a reason for hiding this comment

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

这样实现的话,this.app 和 this.agent 都可以取到值?是不是可以根据 worker.type 判断一下,避免用户误用?

Copy link
Member

Choose a reason for hiding this comment

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

直接在 constructor 里面根据类型来赋值吧?而不是 assert 报错。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

根据类型来赋值,开发者取到 undefined,有个断言提醒会不会好一点?

Copy link
Member

Choose a reason for hiding this comment

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

也行

@codecov
Copy link

codecov bot commented Apr 3, 2019

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #3581   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          32      34    +2     
  Lines         921     988   +67     
======================================
+ Hits          921     988   +67
Impacted Files Coverage Δ
lib/agent.js 100% <ø> (ø) ⬆️
lib/egg.js 100% <100%> (ø) ⬆️
lib/core/base_hook_class.js 100% <100%> (ø)
agent.js 100% <100%> (ø)
index.js 100% <100%> (ø) ⬆️
app/extend/request.js 100% <0%> (ø) ⬆️
app/extend/response.js 100% <0%> (ø) ⬆️
config/config.default.js 100% <0%> (ø) ⬆️
lib/core/base_context_logger.js 100% <0%> (ø) ⬆️
... and 1 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 4b13a1f...56918d4. Read the comment docs.

lib/core/base_hook_class.js Outdated Show resolved Hide resolved
@atian25 atian25 changed the title fix: fix agent logger config not work feat Apr 3, 2019
@atian25 atian25 changed the title feat feat: lifecycle base class Apr 3, 2019
index.d.ts Outdated Show resolved Hide resolved
@killagu killagu force-pushed the fix/agent-logger branch 3 times, most recently from b7af099 to 16abfd1 Compare April 8, 2019 13:00
@atian25
Copy link
Member

atian25 commented Apr 11, 2019

覆盖率那个看看?然后可以合并了。

@killagu
Copy link
Contributor Author

killagu commented Apr 13, 2019

@atian25 可以合了

@atian25 atian25 merged commit 8bb7c7e into master Apr 16, 2019
@atian25 atian25 deleted the fix/agent-logger branch April 16, 2019 01:45
popomore pushed a commit that referenced this pull request Apr 16, 2019
feat: add BaseHookClass (#3581)

Refs: #3454
Fixes: #3548

class EggAgentHook extends BaseHookClass {
configDidLoad() {
this.agent._wrapMessenger();
Copy link
Member

Choose a reason for hiding this comment

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

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