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

fix: app.keys getter must have a setter either #3891

Merged
merged 2 commits into from
Aug 21, 2019
Merged

Conversation

dead-horse
Copy link
Member

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

@@ -271,6 +271,10 @@ class Application extends EggApplication {
return this[KEYS];
}

set keys(_) {
Copy link
Member Author

Choose a reason for hiding this comment

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

get/set 最好还是成对出现,特别是覆盖属性的时候,避免重构踩坑。

Copy link
Member Author

Choose a reason for hiding this comment

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

koajs/koa@287e589

koa 也改进了一下

@codecov
Copy link

codecov bot commented Aug 19, 2019

Codecov Report

Merging #3891 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #3891   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          34      34           
  Lines         945     945           
======================================
  Hits          945     945
Impacted Files Coverage Δ
lib/application.js 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 24c388b...9b5e77b. Read the comment docs.

@leoDreamer
Copy link

leoDreamer commented Aug 21, 2019

大佬多久和呀,现在不锁koa版本都不能打镜像,不好意思没看到koa已经2.8.1了,之前锁了版本在操作

@atian25
Copy link
Member

atian25 commented Aug 21, 2019

koa 不是回滚了么

@dead-horse
Copy link
Member Author

koa 不是回滚了么

koa 是绕过了,但是其实本不应该 koa 关心这个事情的,是 egg 的实现有问题。

@dead-horse dead-horse merged commit a9d0cf5 into master Aug 21, 2019
@dead-horse dead-horse deleted the fix-keys-setter branch August 21, 2019 06:40
popomore pushed a commit that referenced this pull request Aug 21, 2019
fix: app.keys getter must have a setter either (#3891)
@atian25
Copy link
Member

atian25 commented Aug 26, 2019

@dead-horse 这个发版本了么?

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.

3 participants