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

chore: change request.body and ctx.config type #2145

Merged
merged 1 commit into from
Feb 28, 2018
Merged

Conversation

shepherdwind
Copy link
Contributor

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

@codecov-io
Copy link

codecov-io commented Feb 28, 2018

Codecov Report

Merging #2145 into typings will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           typings    #2145   +/-   ##
========================================
  Coverage    99.59%   99.59%           
========================================
  Files           29       29           
  Lines          743      743           
========================================
  Hits           740      740           
  Misses           3        3

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 bd7100a...00f7cf9. Read the comment docs.

@@ -39,8 +39,6 @@ export interface Logger {
error(info: string | Error, ...args: any[]): void;
}

export type RequestArrayBody = any[];
export type RequestObjectBody = { [key: string]: any };
Copy link
Member

Choose a reason for hiding this comment

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

这 2 个干掉的话会不会 break ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个没必要用了吧,只有 request.body 可能用上

index.d.ts Outdated
@@ -22,7 +22,7 @@ declare class BaseContextClass { // tslint:disable-line
/**
* Application config object
*/
config: EggAppConfig;
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.

额,现在这样很难用,自定义的时候 ctx.config.xxx 很多时候都没有自定义。上次 @popomore 他们群里讨论决定改成 any 的。

Copy link
Member

Choose a reason for hiding this comment

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

可以吧,我的想法是,插件那边提供的 config,都提供 merge,应用层的,也自己写 merge

Copy link
Member

Choose a reason for hiding this comment

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

是否可以写成

config: { [key: string]: any } & EggAppConfig

或者干脆给 EggAppConfig 加上 [key: string]: any

Copy link
Contributor Author

Choose a reason for hiding this comment

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

上次讨论是觉得那样,需要写成 class 模式,有些不好,而且麻烦,然后建议都改成 any ,简单粗暴。

Copy link
Member

Choose a reason for hiding this comment

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

简单粗暴但丢失了 TS 的优点,加个 [key: string]: any 就好了吧。

Copy link
Member

Choose a reason for hiding this comment

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

插件和应用层不写,那也只是不提示他们而已,至少 egg 内置的要显示吧

Copy link
Member

Choose a reason for hiding this comment

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

建议给 EggAppConfig 加上 [key: string]: any

Copy link
Contributor Author

Choose a reason for hiding this comment

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

嗯,改了。一开始没想到,这样确实比较好。

@atian25 atian25 requested a review from whxaxes February 28, 2018 05:42
@atian25 atian25 changed the title chore: change request.body and ctx.config type to any chore: change request.body and ctx.config type Feb 28, 2018
@atian25 atian25 merged commit 565adc2 into typings Feb 28, 2018
@atian25 atian25 deleted the change-type branch February 28, 2018 06:42
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