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(typings): change export interface to class definition #2293

Merged
merged 19 commits into from
Apr 3, 2018

Conversation

waitingsong
Copy link
Contributor

issue: #2292

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

issue link

@atian25 atian25 requested review from whxaxes and shepherdwind and removed request for whxaxes March 29, 2018 12:51
@codecov
Copy link

codecov bot commented Mar 29, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2293   +/-   ##
=======================================
  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 1611079...e16f76d. Read the comment docs.

@waitingsong
Copy link
Contributor Author

for this usage:

//  lib/agent.ts
import * as path from 'path'
import {Agent as EggAgent} from 'egg'

export const EGG_PATH = Symbol.for('egg#eggPath');

export default class Agent extends EggAgent {
  get [EGG_PATH]() {
    return path.dirname(__dirname);
  }
}

@waitingsong
Copy link
Contributor Author

waitingsong commented Mar 29, 2018

使用 样例代码库framework 测试成功

index.d.ts Outdated
* Singleton instance in Agent Worker, extend {@link EggApplication}
*/
class Agent extends EggApplication {
_wrapMessenger(): void
Copy link
Member

Choose a reason for hiding this comment

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

这个 _wrapMessenger 是非暴露的方法把,我觉得不需要加进来

index.d.ts Outdated
https: boolean // https or not
key: string //ssl key
cert: string // ssl cert
typescript: boolean
Copy link
Member

Choose a reason for hiding this comment

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

typescript 这个入参,我记得我们当初定的时候,不是给用户用的吧?@atian25 ,应该不需要加进来?

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 Outdated
typescript: boolean
}

export function startCluster(options: ClusterOptions, callback: () => any): void
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 Outdated
key: string //ssl key
cert: string // ssl cert
typescript: boolean
framework: string; // specify framework that can be absolute path or npm package
Copy link
Member

Choose a reason for hiding this comment

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

这样的注释,在 vscode 那边能看到提示么?
是不是要用 jsdoc 的方式来做注释?

Copy link
Contributor Author

@waitingsong waitingsong Mar 30, 2018

Choose a reason for hiding this comment

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

如果是 vsc ,鼠标 ctrl+hover时 可以显示注释内容的

@waitingsong
Copy link
Contributor Author

2018-03-30_200728

@waitingsong
Copy link
Contributor Author

waitingsong commented Mar 30, 2018

如果改成这种格式,悬浮提示时参数显示数量更少

2018-03-30_201242

2018-03-30_201421

plugins, port, https, key, cert, typescript
index.d.ts Outdated
// typescript?: boolean;
}

export function startCluster(options: ClusterOptions, callback: () => any): void
Copy link
Member

Choose a reason for hiding this comment

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

void 后面也顺手加个 ;

index.d.ts Outdated
}

export interface ClusterOptions {
framework: string; // specify framework that can be absolute path or npm package
Copy link
Member

Choose a reason for hiding this comment

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

framework / baseDir / workers 都是可选的

@atian25 atian25 merged commit e0e7ed1 into eggjs:master Apr 3, 2018
@waitingsong waitingsong deleted the ts branch April 3, 2018 10:59
popomore pushed a commit that referenced this pull request Apr 3, 2018
chore(typings): change export interface to class definition (#2293)
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