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: add tsd #1027

Merged
merged 4 commits into from
Jun 20, 2017
Merged

feat: add tsd #1027

merged 4 commits into from
Jun 20, 2017

Conversation

shepherdwind
Copy link
Contributor

新增 tsd 文件,依赖 https://github.com/eggjs/egg-type

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

@shepherdwind
Copy link
Contributor Author

用例和依赖都在 https://github.com/eggjs/egg-type/blob/master/package.json#L7 这个仓库里面。

index.d.ts Outdated
@@ -0,0 +1,41 @@
import * as EggType from 'egg-type';
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.

独立的话更新起来更方便,不需要发 egg 版本了,只需要发这个 egg-type 即可。

Copy link
Member

Choose a reason for hiding this comment

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

一般增加 api 都需要改地址egg,不然 egg 的测试就过不了。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我想的是,这个 tsd 类似于文档,egg 的文档和 egg 仓库在一起,但如果文档发布需要 egg 升级就不大合理了。

现在的 tsd 定义还只有一个 demo ,并没有在内部真正用上,后面修改可能会比较多。这种情况如果要频繁升级 egg 的版本就不大合理了。

ts 本身功能也在升级,后面也许接口自动 merge 支持了,我们的写法也会有所变化。因此,我认为初期最好独立一个仓库,这样后面发布 tsd 包会比较方便,不需要依赖 egg 的版本。

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.

所以这里就有问题,没办法维护的。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

egg-type 依赖的 egg 只是一些基本的接口定义。这个 egg 里面 index.d.ts 里面只有很少一部分定义,这一部分基本不变的,后续接口变动只修改 egg-type 就可以了,测试也不会再有依赖的。

Copy link
Member

Choose a reason for hiding this comment

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

我的观点还是哪里的代码就在哪里定义 ts,两者只要有一个地方变更就会不同步。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

那我弄过来吧,egg-type 不用也行,我主要考虑后续升级上可能没那么方便,不过 tsd 一般很少改动,影响也不大。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

还要改啥?我都弄过来了,egg-type 包不依赖了

@@ -0,0 +1,34 @@
import { Controller, Service, Application } from '..';
Copy link
Member

Choose a reason for hiding this comment

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

测试放 fixture 吧,建一个应用,按标准 egg 应用写,然后用 coffee 运行命令行测试

Copy link
Contributor 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 Jun 10, 2017

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1027   +/-   ##
=======================================
  Coverage   98.96%   98.96%           
=======================================
  Files          29       29           
  Lines         676      676           
=======================================
  Hits          669      669           
  Misses          7        7

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 a4ba2a2...f816f0c. Read the comment docs.

@@ -74,7 +80,8 @@
"config",
"bin",
"lib",
"index.js"
"index.js",
"index.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.

d.ts 的 entry 配置要配么

Copy link
Contributor Author

Choose a reason for hiding this comment

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

d.ts 的 entry 配置 是什么意思?

ts 默认会找包里面的 index.d.ts ,找不到找 @types/egg

Copy link
Member

Choose a reason for hiding this comment

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

就是如果不放 index.d.ts 是否可以指定,类似 main

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html 可以配置 types 字段,要配一个么?默认是 index.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.

配吧

Copy link
Contributor Author

Choose a reason for hiding this comment

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

加好了

import { Controller } from '../../../../../../';

// add user controller and service
declare module '../../../../../../' {
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
Contributor Author

Choose a reason for hiding this comment

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

嗯,这个是有点长了

// add user controller and service
declare module '../../../../../../' {
interface IController {
foo: FooController;
Copy link
Member

Choose a reason for hiding this comment

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

class 不是不会提升吗,ts 支持的?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

放上面下面都可以,这里是静态分析过程,所以放上面也行,不过统一放下面可能会好点

*/
service: IService;

constructor(ctx: Context);
Copy link
Member

Choose a reason for hiding this comment

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

这个不错

const baseDir = path.join(__dirname, '../fixtures/apps/app-ts');

describe('test/ts/index.test.js', () => {
it('tsc ok', function* () {
Copy link
Member

Choose a reason for hiding this comment

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

这个可以改成 before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@shepherdwind shepherdwind force-pushed the add-tsd branch 3 times, most recently from ebea601 to 3c3b142 Compare June 19, 2017 08:05
@shepherdwind
Copy link
Contributor Author

用例要跑半个小时啊

package.json Outdated
"@types/accepts": "^1.3.2",
"@types/koa": "^2.0.39",
"@types/koa-router": "^7.0.22",
"@types/node": "^7.0.29",
Copy link
Member

@fengmk2 fengmk2 Jun 19, 2017

Choose a reason for hiding this comment

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

如果使用 egg 的应用也依赖了 @types/node 怎么办?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个我去掉吧,不依赖 node 也可以的

@shepherdwind
Copy link
Contributor Author

8.1 下面 link 会挂

Unhandled rejection RangeError: Maximum call stack size exceededpreinstall: no script for preinstall, continuing
    at exports.create (/usr/local/lib/node_modules/.npm_npminstall/npm/3.10.8/npm/lib/install/node.js:31:40)
    at /usr/local/lib/node_modules/.npm_npminstall/npm/3.10.8/npm/lib/install/node.js:36:14
    at Array.forEach (native)
    at exports.create (/usr/local/lib/node_modules/.npm_npminstall/npm/3.10.8/npm/lib/install/node.js:33:25)
    at /usr/local/lib/node_modules/.npm_npminstall/npm/3.10.8/npm/lib/install/node.js:36:14
    at Array.forEach (native)
    at exports.create (/usr/local/lib/node_modules/.npm_npminstall/npm/3.10.8/npm/lib/install/node.js:33:25)
    at normalizeTree (/usr/local/lib/node_modules/.npm_npminstall/npm/3.10.8/npm/lib/install.js:377:5)
    at Array.forEach (native)
    at normalizeTree (/usr/local/lib/node_modules/.npm_npminstall/npm/3.10.8/npm/lib/install.js:379:19)
    at Array.forEach (native)
    at normalizeTree (/usr/local/lib/node_modules/.npm_npminstall/npm/3.10.8/npm/lib/install.js:379:19

@shepherdwind
Copy link
Contributor Author


describe('test/ts/index.test.js', () => {
before(function* () {
yield runscript('tsc && npm link ../../../../', { cwd: baseDir });
Copy link
Member

Choose a reason for hiding this comment

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

if (process.env.CI) {
  yield runscript('tsc && npmlink ../../../../', { cwd: baseDir });
} else {
  yield runscript('tsc && npm link ../../../../', { cwd: baseDir });
}

@shepherdwind 这样改,使用 npmlink

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我试下

@shepherdwind
Copy link
Contributor Author

这个可以合并不,tsd 有人发 mr 了 DefinitelyTyped/DefinitelyTyped#17318

@popomore
Copy link
Member

先合并

@popomore popomore merged commit 2b1644e into master Jun 20, 2017
@popomore popomore deleted the add-tsd branch June 20, 2017 15:11
@fwgood
Copy link

fwgood commented Oct 18, 2017

@shepherdwind
Copy link
Contributor Author

@fwgood 类型内置到 egg 里面了,所以那个仓库也不需要了。

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