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: improve d.ts with ts support #2306

Merged
merged 20 commits into from
Apr 4, 2018
Merged

chore: improve d.ts with ts support #2306

merged 20 commits into from
Apr 4, 2018

Conversation

whxaxes
Copy link
Member

@whxaxes whxaxes commented Apr 1, 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

@whxaxes whxaxes requested review from atian25 and shepherdwind and removed request for atian25 April 1, 2018 07:56
Copy link
Member

@atian25 atian25 left a comment

Choose a reason for hiding this comment

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

好像有一个测试 fixture 里面的用法改改?

@whxaxes
Copy link
Member Author

whxaxes commented Apr 1, 2018

@atian25 app-ts 那个 fixtures 里就这么一句

export default {
  keys: 'foo',
}

没用到 EggAppConfig

@atian25
Copy link
Member

atian25 commented Apr 1, 2018

嗯,那就只是 example 那边错?

@whxaxes
Copy link
Member Author

whxaxes commented Apr 1, 2018

嗯,就 example 那边之后改一下就好了

index.d.ts Outdated
@@ -165,6 +165,15 @@ declare module 'egg' {

export type LoggerLevel = 'DEBUG' | 'INFO' | 'WARN' | 'ERROR' | 'NONE';

export interface EggAppInfo {
Copy link
Member

Choose a reason for hiding this comment

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

顶部加个. jsdoc?说明下这个用在哪里的,如给个 config 的示例

Copy link
Member Author

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 1, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2306   +/-   ##
=======================================
  Coverage   99.59%   99.59%           
=======================================
  Files          29       29           
  Lines         749      749           
=======================================
  Hits          746      746           
  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 7fba689...745cb27. Read the comment docs.

@atian25 atian25 mentioned this pull request Apr 1, 2018
28 tasks
index.d.ts Outdated
* ```
*/
export interface EggAppInfo {
pkg: string; // package.json
Copy link
Member

Choose a reason for hiding this comment

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

这里错了, pkg 是 json

Copy link
Member 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.

rebase

Copy link
Member Author

Choose a reason for hiding this comment

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

改了

* }
* ```
*/
export interface EggAppInfo {
Copy link
Contributor

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.

Copy link
Member

Choose a reason for hiding this comment

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

@shepherdwind 之前你们 example 那边写的 config 理解有问题。

export default (appInfo: EggAppInfo, appConfig?: EggAppConfig) => {} 这才是 config.{env}.js 的签名

Copy link
Member

Choose a reason for hiding this comment

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

不过 appConfig 这个没几个人知道,哈哈

@atian25
Copy link
Member

atian25 commented Apr 3, 2018

  1. 添加下 plugin

  2. 这个要改下,info 的第一个参数是支持 number 或 object 啥的,直接 any 好了。

  export interface Logger {
    info(info: string, ...args: any[]): void;
    warn(info: string, ...args: any[]): void;
    debug(info: string, ...args: any[]): void;
    error(info: string | Error, ...args: any[]): void;
  }

Copy link
Member

@atian25 atian25 left a comment

Choose a reason for hiding this comment

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

顺手改下 logger 那里

index.d.ts Outdated
env?: EggEnvType;
path?: string;
package?: string;
enabled?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

enable

index.d.ts Outdated
/**
* build-in plugin list
*/
interface EggPluginList {
Copy link
Member

Choose a reason for hiding this comment

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

就叫 EggPlugin 吧

@atian25 atian25 changed the title chore: add EggAppInfo in d.ts chore: improve d.ts with ts support Apr 4, 2018
index.d.ts Outdated
@@ -348,11 +371,31 @@ declare module 'egg' {

siteFile: any;
Copy link
Member

Choose a reason for hiding this comment

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

* ```
*/
export interface EggAppInfo {
pkg: any; // package.json
Copy link
Member

Choose a reason for hiding this comment

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

是个 object

Copy link
Member Author

Choose a reason for hiding this comment

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

看到 config 下的就是 any,这种 package info,特意写成 { [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.

any 可以是 Array,String 啥的吧?不过这个无所谓了。

index.d.ts Outdated
cache: boolean;
defaultExtension: string;
defaultViewEngine: string;
mapping: any;
Copy link
Member

Choose a reason for hiding this comment

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

object, key value 都是 string

index.d.ts Outdated
watcher: any;
watcher: {
type: string;
eventSources: {
Copy link
Member

Choose a reason for hiding this comment

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

这个 key 可以干掉,或者里面允许 [key: string]

Copy link
Member

Choose a reason for hiding this comment

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

感觉这些是不是提交到 egg-watcher 那边,然后 egg 里面 import 就好了?不要都写在 egg 里面

Copy link
Member Author

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.

先在这改吧,改完我们看看是不是再一个 PR 干掉,然后才一起发布。

index.d.ts Outdated
};

// egg env type
type EggEnvType = 'local' | 'unittest' | 'prod';
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 Author

@whxaxes whxaxes Apr 4, 2018

Choose a reason for hiding this comment

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

这个覆盖就是 declare merging....这个不需要写在注释里把,应该是要写在如何写插件或者框架的 ts 那里吧?

Copy link
Member Author

Choose a reason for hiding this comment

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

如果这个都要写,那 interface 那些不也一样要写了么...

Copy link
Member

Choose a reason for hiding this comment

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

行吧,那在 ts 文档那边写就好了,或者是在 d.ts 的顶部写一次。

index.d.ts Outdated
@@ -540,6 +594,8 @@ declare module 'egg' {
controller: IController;

Controller: Controller;

middlewares: IMiddleware;
Copy link
Member

Choose a reason for hiding this comment

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

middleware

Copy link
Member Author

Choose a reason for hiding this comment

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

egg-core 里是 middlewares 哦

Copy link
Member Author

Choose a reason for hiding this comment

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

image

Copy link
Member

Choose a reason for hiding this comment

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

你往上看看,有一个 defineProperty

Copy link
Member Author

Choose a reason for hiding this comment

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

哦,看到了,我改一下

index.d.ts Outdated

static: any;
static: {
prefix: string;
Copy link
Member

Choose a reason for hiding this comment

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

除了这些还有其他参数的,https://github.com/eggjs/egg-static#configuration
加个允许未知 key 吧

Copy link
Member Author

Choose a reason for hiding this comment

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

好,我加个 &PlainObject

* // { view: { defaultEngines: string } } => { view?: { defaultEngines?: string } }
* type EggConfig = PowerPartial<EggAppConfig>
*/
type PowerPartial<T> = {
Copy link

Choose a reason for hiding this comment

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

这个 PowerPartial 的实现,用递归写法好点吧 ?

type PowerPartial<T> = {
    [U in keyof T]? : T[U] extends {}
      ? PowerPartial<T[U]> 
      : T[U];
  };

Copy link
Member Author

Choose a reason for hiding this comment

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

可以,我改一下

Copy link
Member Author

@whxaxes whxaxes Apr 4, 2018

Choose a reason for hiding this comment

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

会不会有性能问题。。。这样一直递归下去,如果是按照此前的,就至少能控制在 3 层

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

Choose a reason for hiding this comment

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

额,你这样讲也有可能,不过这个只是对编译期,应该还好吧~~看看翰文他们什么意见。

Copy link

@beliefgp beliefgp Apr 4, 2018

Choose a reason for hiding this comment

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

interface Test {
  a: {
    a1: {
      a2: {
        a3: Test;
      };
    },
  };
  b: string;
}
type TestP = PowerPartial<Test>;
const t: TestP = {
      a: {
        a1: {
          a2: {
            a3: {
              a: {
              },
            },
          },
        },
      },
      b: 'b',
    };

试了下,编译提示都没有什么问题,也没有卡顿,ts应该有这种内部保护机制吧……

Copy link
Member Author

Choose a reason for hiding this comment

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

不是对象,只是 interface 的话,是可能有的有这种互相引用的情况的,不过我刚才试了一下,发现没问题,那我改成你这种把

Copy link
Member

@atian25 atian25 Apr 4, 2018

Choose a reason for hiding this comment

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

先不改吧,反正 config 一般也就三层

Copy link

Choose a reason for hiding this comment

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

我能说我现在就有4层的么……😆

Copy link
Member Author

Choose a reason for hiding this comment

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

OK,改了

@@ -3,8 +3,23 @@ import * as KoaApplication from 'koa';
import * as KoaRouter from 'koa-router';
import { RequestOptions } from 'urllib';
import { Readable } from 'stream';
import 'egg-onerror';
Copy link
Contributor

Choose a reason for hiding this comment

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

插件全部都引入么?但是如果用户不开启也会有相关类型提示了,会不会太多了。

Copy link
Member Author

Choose a reason for hiding this comment

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

这些都是 egg 内置的插件,其实是希望之后,能把配置这些分散到各个插件去实现

Copy link
Member Author

Choose a reason for hiding this comment

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

这些插件都是开启的吧,目前这些插件也还没有 d.ts,现在先引入,之后补了插件的 d.ts 之后,这个也可以直接生效了

Copy link
Member

Choose a reason for hiding this comment

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

我让他先写入的,后面 egg 里面的一些如 config.development 的定义,都下沉到插件那边去。
现在先 import 了,避免后面又要发 egg 版本

@atian25 atian25 merged commit 4061427 into master Apr 4, 2018
@atian25 atian25 deleted the ts-app-info branch April 4, 2018 10:50
@atian25 atian25 mentioned this pull request Apr 4, 2018
4 tasks
popomore pushed a commit that referenced this pull request Apr 4, 2018
chore: improve d.ts with ts support (#2306)
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